fix: detect bidi control characters outside markdown files (CVE-2021-42574) - #473
fix: detect bidi control characters outside markdown files (CVE-2021-42574)#473MohammedAlkindi wants to merge 2 commits into
Conversation
The bidi-control pattern (Trojan Source, CVE-2021-42574) lived in P2_PATTERNS, which only runs when file_type is markdown or other. static_runner maps .py to python, .sh to shell and so on, so a bidi payload in a bundled script was never checked - the file types the CVE actually targets. Evaluate the bidi pattern regardless of file_type, alongside the existing unconditional Unicode Tag-block check. Zero-width detection stays markdown-gated deliberately: ZERO_WIDTH_CHARS includes U+FEFF, so ungating it would flag every BOM-prefixed source file. Signed-off-by: Mohammed Alkindi <alkndymhmd692@gmail.com>
yashrajp22
left a comment
yashrajp22
left a comment
There was a problem hiding this comment.
Fix looks right to me. I ran it end to end: a bidi payload in a bundled .py goes from 0 findings to 1 HIGH P2, all 9 paired control chars are caught individually, and markdown output is byte-identical to main (same lines, same confidence, no double-reporting). The zero-width/BOM gating is untouched — checked with BOM-prefixed .py and .md files. It also quietly starts covering .txt files, which the description doesn't mention — nice bonus. A few small notes inline, none blocking.
| # Bidirectional control characters (Trojan Source, CVE-2021-42574). Evaluated | ||
| # unconditionally below, NOT in the markdown-gated P2_PATTERNS list -- see the | ||
| # comment at that check for why. | ||
| _BIDI_CONTROL_PATTERN = r"[\u202a-\u202e\u2066-\u2069]" |
There was a problem hiding this comment.
One thing worth a one-line code comment here: this class covers the 9 paired controls (U+202A–202E and U+2066–2069) but not the direction marks U+200E/U+200F/U+061C — I checked, those produce zero findings anywhere (they're not in ZERO_WIDTH_CHARS either). That matches GCC's -Wbidi-chars default, since the marks can't reorder spans the way the paired controls can, so I think the exclusion is right. But since the PR references CVE-2021-42574, documenting that it's deliberate would keep someone from "fixing" it later without understanding the tradeoff.
| # (issue #39). Zero-width detection stays markdown-gated because | ||
| # ZERO_WIDTH_CHARS includes U+FEFF (BOM), which would false-positive on | ||
| # every BOM-prefixed source file; the bidi range never overlaps it. | ||
| for match in _p2_pattern_matches(content, _BIDI_CONTROL_PATTERN): |
There was a problem hiding this comment.
On the i18n tradeoff you called out in the description — I confirmed it's real but narrow: a legit FSI/PDI-wrapped Arabic string in a .json l10n file now fires HIGH/0.85, while plain Arabic/Hebrew text without control chars stays clean. If false positives ever become a problem, the surgical option is a lower confidence (~0.6) for just the isolate range U+2066–2069 (the well-formed idiom message-formatting libraries emit), keeping 0.85 for U+202A–202E, which is what Trojan Source actually needs. Fine as-is for a security scanner — just leaving the option here.
| findings.append( | ||
| AnalyzerFinding( | ||
| rule_id="P2", | ||
| message="Hidden Instructions", |
There was a problem hiding this comment.
Tiny nit, feel free to ignore: "Hidden Instructions" is indistinguishable from the zero-width and HTML-comment P2 findings when triaging results — the Tag-block check above uses a self-describing message. Changing it would shift the message on existing markdown findings though (your parity test asserts that), so leaving it is also reasonable.
|
Added the comment (850a32b), with the I re-measured your check before writing it: U+200E, U+200F and U+061C each produce 0 findings in both a Leaving the isolate-range confidence split and the "Hidden Instructions" wording alone for now, since both move existing markdown findings that the parity test pins. Worth revisiting if l10n false positives actually turn up. |
2 participants
P2's bidi-control pattern (Trojan Source, CVE-2021-42574) sits inP2_PATTERNS, which only runs underif file_type in ("markdown", "other").static_runnermaps.pytopython,.shtoshelland so on, so a bidi payload in a bundled script was never checked — the file types the CVE actually targets.Same payload (
U+202E…U+202Creversing a comment) in one skill:SKILL.mdP2P2scripts/helper.pyP2P2Markdown is unchanged and still emits exactly one
P2.#39 reported this exact
helper.pycase. #92 added the regex to the markdown-gated list without widening the gate, and #39 auto-closed with no comments.Only the bidi entry moves. Zero-width stays gated deliberately:
ZERO_WIDTH_CHARSincludesU+FEFF, so ungating it would flag every BOM-prefixed source file — the false positive the Tag-block comment already calls out.Your call on one tradeoff: bidi controls have legitimate RTL/i18n uses, so this can fire on source with mixed-direction text. I added no locale carve-out, matching the existing unconditional Tag-block check.