From d88875df866473a4f523a6169d8b314c52964880 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 18:24:00 +0000 Subject: [PATCH] fix(security): neutralize CWE-117 log forging in StructuredFormatter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scrub CR/LF and other line separators (VT, FF, NEL, U+2028, U+2029) from the fully rendered log record in StructuredFormatter.format(). Because the scrub runs on the final formatted string, it covers the message, exc_info traceback text (including str(exc)), and structured `extra` fields at once — closing the log-forging vector that inline message sanitizers miss at exc_info / logger.exception sinks. Separators are escaped rather than dropped, so content stays greppable while one logging call is guaranteed to render as one physical line. Adds tests/unit/test_logging_config_crlf.py asserting the *rendered* handler output (not a sanitizer return value) cannot be used to forge a standalone log line via message, exc_info traceback, or Unicode line separators. Addresses the confirmed unmitigated finding on #810 (CWE-117); this is the central formatter-level remediation that complements #810's per-sink message sanitization. Progresses #898. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01KDtvQR6b5JXkAStGbTCTxL --- .../backend/config/logging_config.py | 24 +++++++- tests/unit/test_logging_config_crlf.py | 55 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_logging_config_crlf.py diff --git a/src/youtube_extension/backend/config/logging_config.py b/src/youtube_extension/backend/config/logging_config.py index 98f28489e..654252333 100644 --- a/src/youtube_extension/backend/config/logging_config.py +++ b/src/youtube_extension/backend/config/logging_config.py @@ -15,6 +15,23 @@ from pathlib import Path +# CWE-117 (log injection / log forging): line-breaking characters that let a +# user-controlled value spawn a second, forged log line. Neutralized in the +# FINAL rendered record — see StructuredFormatter.format() — so exc_info +# tracebacks and structured ``extra`` fields are covered even when a value was +# not sanitized at the call site. Escaping (rather than dropping) keeps the +# content greppable while guaranteeing one log call renders as one physical line. +_LOG_FORGERY_ESCAPES = { + 0x0A: "\\n", # line feed + 0x0D: "\\r", # carriage return + 0x0B: "\\v", # vertical tab + 0x0C: "\\f", # form feed + 0x85: "\\x85", # NEL (Unicode next line) + 0x2028: "\\u2028", # line separator + 0x2029: "\\u2029", # paragraph separator +} + + class StructuredFormatter(logging.Formatter): """ Custom formatter for structured logging with enhanced metadata. @@ -39,7 +56,12 @@ def format(self, record: logging.LogRecord) -> str: # Format the base message formatted_message = super().format(record) - return formatted_message + # CWE-117: neutralize CR/LF (and other line separators) in the fully + # rendered record. This covers the message, any exc_info traceback text + # (including ``str(exc)``) and structured ``extra`` fields the format + # string references, so a user-controlled value carrying "\r\n" cannot + # forge an additional log line even when it was not sanitized inline. + return formatted_message.translate(_LOG_FORGERY_ESCAPES) def formatException(self, ei) -> str: """Format exception with enhanced stack trace""" diff --git a/tests/unit/test_logging_config_crlf.py b/tests/unit/test_logging_config_crlf.py new file mode 100644 index 000000000..284d6412c --- /dev/null +++ b/tests/unit/test_logging_config_crlf.py @@ -0,0 +1,55 @@ +"""CWE-117 regression: StructuredFormatter must neutralize log forging. + +These tests assert against the *rendered* handler output (not a sanitizer's +return value), covering the exc_info / logger.exception sinks that append raw +traceback and exception text after the formatted message. +""" + +import io +import logging + +import pytest + +from youtube_extension.backend.config.logging_config import StructuredFormatter + + +def _render(logger_call) -> str: + buf = io.StringIO() + handler = logging.StreamHandler(buf) + handler.setFormatter(StructuredFormatter("%(levelname)s - %(message)s")) + logger = logging.getLogger("crlf-regression") + logger.handlers[:] = [handler] + logger.propagate = False + logger.setLevel(logging.DEBUG) + logger_call(logger) + handler.flush() + return buf.getvalue() + + +def test_crlf_in_message_cannot_forge_a_line(): + out = _render(lambda lg: lg.info("video %s captured", "abc\r\nCRITICAL - FORGED")) + # Exactly one physical line (plus the trailing newline the handler adds). + assert out.count("\n") == 1 + assert "\r" not in out + assert "FORGED" in out # content preserved, just neutralized + assert "CRITICAL - FORGED" not in out.splitlines() # not a standalone line + + +def test_exc_info_traceback_cannot_forge_log_lines(): + def call(lg): + try: + raise ValueError("boom\r\nCRITICAL - FORGED ADMIN LINE") + except ValueError: + lg.error("Error in chat endpoint", exc_info=True) + + out = _render(call) + assert "\r" not in out + # The forged text must never appear as its own rendered line. + assert "CRITICAL - FORGED ADMIN LINE" not in out.splitlines() + + +@pytest.mark.parametrize("sep", ["
", "
", "\x0b", "\x0c", "\x85"]) +def test_unicode_line_separators_are_neutralized(sep): + out = _render(lambda lg: lg.info("session %s", "s" + sep + "INJECTED")) + assert sep not in out + assert "INJECTED" in out