fix(logging): neutralize control characters in RedactingFormatter (fixes #61409)#61450
fix(logging): neutralize control characters in RedactingFormatter (fixes #61409)#61450kuangmi-bit wants to merge 1 commit into
Conversation
…put (fixes NousResearch#61409) Log record injection via unescaped newlines in interpolated values: an attacker-controlled string in a log argument (e.g. a tool output or a user-supplied command) could embed a newline to forge a second physical log line. Because hermes logs re-parses each physical line into a record, the forged line surfaced as a genuine record with an attacker-chosen level and component. Fix mirrors _log_safe_path in gateway/platforms/base.py: replace control characters (\x00-\x1f, DEL, NEL, LS, PS) with '?' in the formatted output so a single log call always produces a single record.
Duplicate of #61411 — same fix: both add the identical control-character neutralization regex to |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (LGTM)
Overview
Small, well-scoped security fix. Adds control character sanitization to RedactingFormatter to prevent log injection attacks where attacker-controlled data embedded in log arguments could forge additional log records.
Changes
agent/redact.py: Added_LOG_UNSAFE_CHARSregex pattern covering control chars (0x00-0x1F), DEL, NEL, LINE SEPARATOR, PARAGRAPH SEPARATOR. Applied informat()after redaction.tests/agent/test_redact.py: New testtest_control_characters_neutralizedcovering malicious input with embedded newline.
Security
- Log injection prevention — attacker-controlled data cannot forge additional log records
- Matches existing
_log_safe_pathpattern ingateway/platforms/base.py - No secrets or credentials in diff
Code Quality
- Clean implementation, minimal diff (~20 lines)
- Good documentation explaining the security rationale
- Test is thorough: checks replacement char, legitimate prefix survives, single physical line result
Testing
- Dedicated unit test covers the specific attack vector
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
What the PR Does
Neutralize control characters (including newlines) in log output via RedactingFormatter, preventing log-forging attacks where attacker-controlled data in log arguments injects fake log records.
Assessment
- Security: This is a genuine security fix. A malicious actor controlling log argument content could inject newlines to forge ERROR-level log entries. The regex covers control chars, DEL, NEL, LINE SEPARATOR, and PARAGRAPH SEPARATOR.
- Tests: New test test_control_characters_neutralized covers the newline-forging scenario directly.
- Correctness: Matches the existing _log_safe_path pattern in gateway/platforms/base.py.
Note: This is the formal review verdict. PR 61450 previously had a COMMENT activity.
Reviewed by Hermes Agent
Summary
RedactingFormatter.format()redacts secrets but never sanitized control characters, allowing log record injection. An attacker-controlled value in a log argument could embed a newline to forge additional log records with attacker-chosen levels and components.Fix
Add control-character neutralization to
RedactingFormatter.format(). Afterredact_sensitive_text(), replace control characters (\x00-\x1f, DEL, NEL, LINE SEPARATOR, PARAGRAPH SEPARATOR) with?— matching the existing_log_safe_pathpattern ingateway/platforms/base.py.Verification
test_control_characters_neutralizedcovers the injection scenario