Skip to content

fix(event-publisher): sanitise untrusted values before logging - #67

Merged
perezmlal merged 1 commit into
mainfrom
fix/log-injection-event-publisher
Aug 10, 2026
Merged

fix(event-publisher): sanitise untrusted values before logging#67
perezmlal merged 1 commit into
mainfrom
fix/log-injection-event-publisher

Conversation

@perezmlal

@perezmlal perezmlal commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes CodeQL alert #3py/log-injection (CWE-117).

The flagged line was already safe

The alert points at app.logger.info("key: %s", key), where key is built from the sub_service, sub_service_group and geohash path parameters. All three are allowlist-validated a few lines earlier: GEOHASH_RE and SUB_SERVICE_GROUP_RE cannot match CR or LF, and sub_service must pass a set-membership check. CodeQL does not model regex-match guards as taint barriers, so it reports the flow regardless.

The scan missed a real one three lines below

app.logger.warning("Encoding failed: %s", exc) logs the asn1tools EncodeError message, which embeds field names and values taken from the request JSON body. That body is only checked for being a dict — its contents are never validated, and the error message can carry raw CR/LF.

Verified end-to-end against a real ASN.1 encoder. Against the unpatched service:

$ curl -X POST localhost:5000/api/publish/denm/public/7y0191k4 \
    -H 'Content-Type: application/json' \
    -d '{"header":"x\r\n2026-01-01 00:00:00,000 CRITICAL EventPublisher : FORGED ENTRY"}'

app.log gains a fully-formed forged entry, indistinguishable from a genuine one:

2026-08-10 23:24:26,686 WARNING EventPublisher waitress-0 : Encoding failed: DENM.header: Expected data of type dict, but got x
2026-01-01 00:00:00,000 CRITICAL EventPublisher : FORGED ENTRY.

With the fix, the same request stays on one line:

2026-08-10 23:24:06,151 WARNING EventPublisher waitress-0 : Encoding failed: DENM.header: Expected data of type dict, but got x2026-01-01 00:00:00,000 CRITICAL EventPublisher : FORGED ENTRY.

Changes

  • New sanitize_for_log() in EventPublisher.py: strips C0/C1 control characters and DEL, and truncates to LOG_VALUE_MAX_LENGTH (default 256, overridable via config, following the existing getattr(config, ...) pattern) so a large payload cannot flood the log.
  • Applied at both user-derived sinks. The other log calls are untouched: two report Kafka broker state, one is a constant, and one logs a hexlify output.
  • Six tests, including a regression test that drives the EncodeError path and asserts the emitted record contains no line break.
  • CHANGELOG.md entry under Unreleased → Security.

Note for reviewers

The trailing .replace("\r", "").replace("\n", "") is redundant with the regex that precedes it. It is deliberate and load-bearing: it is the sanitisation pattern CodeQL's py/log-injection query recognises as a taint barrier. Removing it as a simplification reopens the alert. There is a comment saying so at the call site.

Verification

Check Result
pytest --cov-fail-under=80 17 passed, 91.86% coverage
ruff check All checks passed
bandit -r No issues identified
End-to-end forgery repro Reproduced unpatched, blocked patched

Alert closure is confirmed by this PR's Analyze (python) run, since no CodeQL CLI is available locally.

CodeQL alert py/log-injection (CWE-117) flagged the `key: %s` log call,
which is built from the sub_service, sub_service_group and geohash path
parameters. Those three are already allowlist-validated before the log
call, so that specific line was not exploitable -- CodeQL simply does not
model regex-match guards as taint barriers.

Auditing the alert turned up a genuine instance the scan missed three
lines below: the asn1tools EncodeError message embeds field names and
values taken from the request JSON body, which is only checked for being
a dict. That message can carry raw CR/LF. Verified end to end against a
real ASN.1 encoder -- POSTing {"header": "x\r\n<fake log line>"} to the
unpatched service writes a fully-formed forged CRITICAL entry to app.log
that is indistinguishable from a genuine one.

Add a sanitize_for_log() helper that strips C0/C1 control characters and
DEL, and truncates to a bounded length so a large payload cannot flood
the log. Apply it at both user-derived sinks. The trailing replace()
calls are redundant with the regex, but they are the pattern CodeQL
recognises as a taint barrier; a comment marks them as load-bearing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Luis Alfredo Perez Medina <perezmlal@outlook.com>
@perezmlal
perezmlal force-pushed the fix/log-injection-event-publisher branch from 3034197 to abb4f60 Compare August 10, 2026 21:26
@perezmlal perezmlal self-assigned this Aug 10, 2026
@perezmlal
perezmlal merged commit c2442e6 into main Aug 10, 2026
12 checks passed
@perezmlal
perezmlal deleted the fix/log-injection-event-publisher branch August 10, 2026 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant