Harden SENTINEL commands and config rewrite against control-character injection by eifrah-aws · Pull Request #3847 · valkey-io/valkey

eifrah-aws · 2026-05-27T13:39:17Z

Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values.

Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

coderabbitai · 2026-05-27T13:39:46Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 43d9f912-222f-4fb9-9a09-791ed5b2ff5e

📥 Commits

Reviewing files that changed from the base of the PR and between 3e6e97e and 4423e1f.

📒 Files selected for processing (1)

src/sentinel.c

📝 Walkthrough

Walkthrough

Adds control-character validation helpers and applies them to SENTINEL CONFIG SET, MONITOR, and SET to reject ASCII control bytes (≤ 0x1F or 0x7F). Updates sentinel config rewrite to use sdscatrepr for several string fields. Adds runtime tests for argument rejection and config rewrite escaping.

Changes

Control character validation in SENTINEL

Layer / File(s)	Summary
Control character detection and validation `src/sentinel.c`	Adds `containsControlChars()` and `sentinelValidateArgs()` that scan argument bytes for ASCII control characters (≤ 0x1F) or DEL (0x7F) and return an error reply when found.
Wire validation into SENTINEL commands `src/sentinel.c`	Invoke `sentinelValidateArgs()` in `SENTINEL CONFIG SET`, `SENTINEL MONITOR`, and `SENTINEL SET` handlers, aborting command handling on validation failure.
Config rewrite uses sdscatrepr for special fields `src/sentinel.c`	`rewriteConfigSentinelOption` now uses `sdscatrepr` when serializing `notification-script`, `client-reconfig-script`, `auth-pass`, `known-sentinel` runid, `rename-command` names, and global `sentinel-user`/`sentinel-pass`.
Migrate sentinel string fields to sds `src/sentinel.c`	Change multiple sentinel state fields (runid, auth_pass, auth_user, notification_script, client_reconfig_script, sentinel_auth_pass, sentinel_auth_user) from `char *` to `sds`.
Test control character rejection and rewrite escaping `tests/sentinel/tests/03-runtime-reconf.tcl`	Adds tests asserting `SENTINEL SET`, `SENTINEL MONITOR`, and `SENTINEL CONFIG SET` reject control characters and verifies config rewrite escapes special characters via `sdscatrepr`.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main security enhancement: hardening SENTINEL commands against control-character injection.
Description check	✅ Passed	The description is relevant and provides context about the control-character validation mechanism being added to prevent config injection.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/sentinel/tests/03-runtime-reconf.tcl (1)

202-217: ⚡ Quick win

Add one regression case for control chars in <primaryname>.

This test covers option/value tokens well; adding a <primaryname> control-char case would close the argument-surface coverage.

🧪 Suggested addition

 test "SENTINEL SET rejects values containing control characters" {
@@
     # Control char in option name is also rejected
     assert_error "ERR Invalid argument*" {S 0 SENTINEL SET mymaster "bad\noption" value}
+    # Control char in primary name is also rejected
+    assert_error "ERR Invalid argument*" {S 0 SENTINEL SET "mymaster\nbad" quorum 2}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/sentinel/tests/03-runtime-reconf.tcl` around lines 202 - 217, Add a
regression case that verifies control characters in the <primaryname> argument
are rejected: inside the test "SENTINEL SET rejects values containing control
characters" add an assert_error call using the same pattern as the other checks
but targeting the primary name token (e.g. assert_error "ERR Invalid argument*"
{S 0 SENTINEL SET "bad\nname" auth-pass "val"}), so the S 0 SENTINEL SET
invocation rejects a primaryname containing a newline, carriage return, null,
tab, or DEL similarly to the existing auth-user/auth-pass checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/sentinel.c`:
- Around line 4238-4245: The loop that rejects control characters skips the
primary name because it starts at j = 3; update the SENTINEL SET validation to
include c->argv[2] (the <primaryname>) by starting the scan at j = 2 (or
otherwise explicitly check objectGetVal(c->argv[2]) with containsControlChars),
and keep the same error path using addReplyError when control characters are
found so <primaryname> cannot bypass the guard.

---

Nitpick comments:
In `@tests/sentinel/tests/03-runtime-reconf.tcl`:
- Around line 202-217: Add a regression case that verifies control characters in
the <primaryname> argument are rejected: inside the test "SENTINEL SET rejects
values containing control characters" add an assert_error call using the same
pattern as the other checks but targeting the primary name token (e.g.
assert_error "ERR Invalid argument*" {S 0 SENTINEL SET "bad\nname" auth-pass
"val"}), so the S 0 SENTINEL SET invocation rejects a primaryname containing a
newline, carriage return, null, tab, or DEL similarly to the existing
auth-user/auth-pass checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3e11e46d-c8f8-48af-9cb8-23427bf3a32c

📥 Commits

Reviewing files that changed from the base of the PR and between 8ee3fd7 and 8dcef32.

📒 Files selected for processing (2)

src/sentinel.c
tests/sentinel/tests/03-runtime-reconf.tcl

codecov · 2026-05-27T14:04:54Z

Codecov Report

❌ Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.66%. Comparing base (7abe49a) to head (4423e1f).

Files with missing lines	Patch %	Lines
src/sentinel.c	0.00%	32 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #3847   +/-   ##
=========================================
  Coverage     76.65%   76.66%           
=========================================
  Files           162      162           
  Lines         80694    80717   +23     
=========================================
+ Hits          61857    61881   +24     
+ Misses        18837    18836    -1

Files with missing lines	Coverage Δ
src/sentinel.c	`0.00% <0.00%> (ø)`

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ranshid

Thank you @eifrah-aws !

I think we are missing some extra sentinal command flows.

sentinelMonitorCommand and sentinelConfigSetCommand both will require the same handling of containsControlChars.
sentinelHandleConfiguration At startup, loadServerConfig reads each line of the conf file and tokenizes it with sdssplitargs(line, &argc). sdssplitargs honors quoted strings and escape sequences, so since sentinelHandleConfiguration NO content validation — whatever bytes sdssplitargs produced go straight into the in-memory field.
createSentinelValkeyInstance - reached from hello-message processing for peer sentinels (so name/runid of a peer comes off the wire from another Sentinel)

I guess aside for the commands processing, we could mitigate some of the existing and future cases by adding another layer of defence in rewriteConfigSentinelOption

Consolidate Sentinel argument sanitization into a shared helper and apply it to commands that persist user-provided values. This keeps the existing CRLF injection protection while making the validation consistent across SENTINEL SET, SENTINEL CONFIG SET, and SENTINEL MONITOR. The new helper scans all arguments from a given offset and rejects any value containing control characters before command-specific parsing continues. * Sentinel command validation * Config and monitor argument handling Signed-off-by: Eran Ifrah <eifrah@amazon.com>

ranshid

Title/description still say "Validate SENTINEL SET arguments," but the change now also covers SENTINEL MONITOR and SENTINEL CONFIG SET — worth updating so the scope (and changelog) reflects reality. Two inline notes on the validation surface below.

ranshid

Follow-up on the config-write sink: this can be hardened here as defense-in-depth.

Use sdscatrepr() to quote user-controlled string fields when writing the sentinel config file, preventing CRLF injection at the write sink regardless of ingress path (command, conf-load, or peer HELLO). Fields hardened: auth-pass, auth-user, sentinel-user, sentinel-pass, notification-script, client-reconfig-script, known-sentinel runid, and rename-command names. The existing announce-ip field already used sdscatrepr. Round-trips cleanly since sdssplitargs unquotes on load. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/sentinel.c`:
- Around line 2059-2060: Replace calls to strlen(...) used as the length
argument to sdscatrepr with sdslen(...) for all SDS-backed fields inside the
sdscatrepr usages; specifically update the sdscatrepr invocations that pass
primary->notification_script, primary->client_reconfig_script,
primary->auth_pass, primary->auth_user, ri->runid, sentinel.sentinel_auth_user
and sentinel.sentinel_auth_pass so they call sdslen(field) instead of
strlen(field). Locate the sdscatprintf/sdscatrepr sequences around the
sdscatrepr calls and only change the length argument to sdslen to avoid
truncation on embedded NULs while keeping the rest of the formatting logic
intact.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e07c3bf2-3dc6-4296-a0d5-b685b1877985

📥 Commits

Reviewing files that changed from the base of the PR and between bfd3438 and 3e6e97e.

📒 Files selected for processing (2)

src/sentinel.c
tests/sentinel/tests/03-runtime-reconf.tcl

🚧 Files skipped from review as they are similar to previous changes (1)

tests/sentinel/tests/03-runtime-reconf.tcl

Change runid, auth_pass, auth_user, notification_script, client_reconfig_script, sentinel_auth_pass, and sentinel_auth_user from char* to sds. Replace strlen() with sdslen() in sdscatrepr calls. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

ranshid

LGTM! thank you @eifrah-aws

… injection (#3847) Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

… injection (valkey-io#3847) Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

zuiderkwast · 2026-06-02T13:56:27Z

Problem when backporting this to 8.1: objectGetVal is not defined. When backporting, we need a minimal definition like #define objectGetVal(_o) ((_o)->ptr).

… injection (#3847) Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

… injection (valkey-io#3847) Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

… injection (#3847) Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

# Backport sweep for 9.1 Automated cherry-picks from PRs marked "To be backported". ## Applied | Source PR | Title | Detail | |---|---|---| | #3743 | Fix buffered_reply assert in HFE commands with module keyspace notifications | cherry-picked in a prior sweep | | #3766 | Fix flaky block_keyspace_notification test for HGETDEL notify race | cherry-picked in a prior sweep | | #3800 | Fix heap-use-after-free in ACL LOAD when client free is deferred | cherry-picked in a prior sweep | | #3723 | Fix double-finish and RESP reply violation in cluster slot migration | cherry-picked in a prior sweep | | #3872 | Redacting customer information when hide_user_data_from_log is true in rdb.c, networking.c, debug.c and t_hash | cherry-picked in a prior sweep | | #3846 | Fix use-after-free in VM_RegisterClusterMessageReceiver | cherry-picked in a prior sweep | | #3806 | Add ALL_DBS flag to CLUSTER FLUSHSLOT for database-level ACL | cherry-picked in a prior sweep | | #3847 | Harden SENTINEL commands and config rewrite against control-character injection | | | #3801 | Validate every DB clause in COPY against ACL db= permissions | | | #3851 | Replace AUTOMATION_PAT with Valkeyrie Bot GitHub App token | | | #3848 | Fix cluster AUX-field control-character and delimiter injection | | | #3544 | Revert "IO-Threads redesign cleanup work (#3544)" | cherry-picked in a prior sweep | | #3888 | Report exact dbid for COPY in ACL LOG when db= access is denied | conflicts resolved by Claude Code | | #3942 | Fix shard_id format specifier in UPDATE message log | | | #3941 | Avoid random() % 0 undefined behaviour when cluster-node-timeout < 30 | | --- *Generated by valkey-ci-agent using Claude Code.* --------- Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: chx9 <lovelypiska@outlook.com> Signed-off-by: zackcam <zackcam@amazon.com> Signed-off-by: Eran Ifrah <eifrah@amazon.com> Signed-off-by: Jules Lasarte <lasartej@amazon.com> Signed-off-by: jjuleslasarte <jules.lasarte@gmail.com> Signed-off-by: akash kumar <akumdev@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: lovelypiska <lovelypiska@outlook.com> Co-authored-by: zackcam <zackcam@amazon.com> Co-authored-by: eifrah-aws <eifrah@amazon.com> Co-authored-by: jjuleslasarte <jules.lasarte@gmail.com> Co-authored-by: Jules Lasarte <lasartej@amazon.com> Co-authored-by: Akash Kumar <45854686+akashkgit@users.noreply.github.com>

… injection (#3847) Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

Validate SENTINEL SET arguments

8dcef32

Reject any SENTINEL SET argument containing control characters (0x00-0x1F, 0x7F) to prevent config injection via newline-embedded values. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

github-actions Bot assigned eifrah-aws May 27, 2026

coderabbitai Bot reviewed May 27, 2026

View reviewed changes

Comment thread src/sentinel.c Outdated

eifrah-aws mentioned this pull request May 27, 2026

Fix use after free #3836

Closed

ranshid reviewed May 27, 2026

View reviewed changes

sarthakaggarwal97 reviewed May 27, 2026

View reviewed changes

Comment thread src/sentinel.c Outdated

eifrah-aws added 2 commits May 28, 2026 12:49

Merge branch 'unstable' into security-fix-sentinel-crlf-injection

bfd3438

eifrah-aws requested a review from ranshid May 28, 2026 10:02

lucasyonge approved these changes May 29, 2026

View reviewed changes

lucasyonge added this to Valkey 9.1, Valkey 8.1, Valkey 8.0, Valkey 9.0 and Valkey 7.2 May 29, 2026

lucasyonge moved this to To be backported in Valkey 7.2 May 29, 2026

lucasyonge moved this to To be backported in Valkey 8.0 May 29, 2026

lucasyonge moved this to To be backported in Valkey 8.1 May 29, 2026

lucasyonge moved this to To be backported in Valkey 9.0 May 29, 2026

lucasyonge moved this to To be backported in Valkey 9.1 May 29, 2026

ranshid reviewed May 31, 2026

View reviewed changes

Comment thread src/sentinel.c

Comment thread src/sentinel.c

ranshid reviewed May 31, 2026

View reviewed changes

Comment thread src/sentinel.c

Merge branch 'unstable' into security-fix-sentinel-crlf-injection

385fe77

eifrah-aws changed the title ~~Validate SENTINEL SET arguments~~ Harden SENTINEL commands and config rewrite against control-character injection Jun 1, 2026

coderabbitai Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/sentinel.c Outdated

eifrah-aws requested a review from ranshid June 1, 2026 10:26

ranshid reviewed Jun 1, 2026

View reviewed changes

Comment thread src/sentinel.c Outdated

ranshid mentioned this pull request Jun 1, 2026

Fix cluster AUX-field control-character and delimiter injection #3848

Merged

ranshid approved these changes Jun 1, 2026

View reviewed changes

ranshid merged commit e71299d into valkey-io:unstable Jun 1, 2026
63 checks passed

valkeyrie-ops Bot mentioned this pull request Jun 2, 2026

[backport] Backport sweep for 9.1 #3774

Merged

valkeyrie-ops Bot mentioned this pull request May 31, 2026

[backport] Backport sweep for 7.2 #3738

Merged

zuiderkwast moved this from To be backported to 8.1.8 (WIP) in Valkey 8.1 Jun 2, 2026

zuiderkwast mentioned this pull request Jun 2, 2026

[backport] Backport sweep for 8.1 #3737

Merged

eifrah-aws deleted the security-fix-sentinel-crlf-injection branch June 4, 2026 06:44

valkeyrie-ops Bot mentioned this pull request Jun 4, 2026

[backport] Backport sweep for 8.0 #3736

Merged

sarthakaggarwal97 moved this from To be backported to Done in Valkey 9.1 Jun 15, 2026

valkeyrie-ops Bot moved this from To be backported to Done in Valkey 7.2 Jun 17, 2026

valkeyrie-ops Bot moved this from To be backported to Done in Valkey 8.0 Jun 18, 2026

valkeyrie-ops Bot mentioned this pull request Jun 19, 2026

[backport] Backport sweep for 9.0 #3879

Open

Uh oh!

Conversation

eifrah-aws commented May 27, 2026

Uh oh!

coderabbitai Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranshid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zuiderkwast commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented May 27, 2026 •

edited

Loading

codecov Bot commented May 27, 2026 •

edited

Loading