fix: close the two CVE rows behind "no single call" (#614, #615) - #632
Conversation
They had to land together. Each was one of the exactly two vectors that made docs/security/cve-validation.md say no entry point cleared everything, and tests/test_cve_vectors.py:2121 was written to fail loudly when either closed. Fixing one alone would have left the other failing and forced the guidance to be rewritten twice. #614 — strip_obfuscation named confusables instead of folding them. 49 code points appear in both emoji_single.tsv and confusables_to_latin.tsv, and most are not emoji: typographic punctuation, currency, math operators, CJK brackets, reaching the emoji table from CLDR annotationsDerived. strip_obfuscation("€xample.com") produced "euro xample.com", so the spoof and the genuine host stopped being equal rather than becoming equal — CVE-2017-5383 surviving a preset documented as maximum-strength deobfuscation. NOT the fix the issue proposed. Reordering the confusable fold before demojize would break idempotency: punctuation inside emoji NAMES (the apostrophe in "woman's hat") has to be folded by the confusable pass. That is documented three times and pinned by tests/test_presets.py. Instead the overlap is DERIVED at build time as an intersection of the two tables, so it cannot drift the way a curated list would, and demojize skips those rows inside comparison presets only. Standalone demojize is unchanged. build.rs asserts the count is 49, so a table refresh that claims another confusable source fails the build rather than widening the gap silently. #615 — canonicalize cannot cap its way out of an eclipsing mark. The anti-zalgo step is a COUNT, and by count one Arabic shadda is indistinguishable from one acute accent, so no threshold removes CVE-2017-7833's spoof and keeps café. The discriminator was already in disarm's script data: strip a combining mark whose own Script is a specific script differing from its base's; keep Inherited marks, which attach to anything. UTS #39's mixed-script reasoning applied per grapheme rather than per string. canonicalize_strict ONLY. The rule is destructive for scholarly transliteration, IPA and linguistic transcription, where marks from one script legitimately sit on bases of another — the corpus least able to notice. canonicalize stays one short deliberately, asserted by a test rather than left implied. Nine legitimate samples in five scripts, including Arabic WITH its own vowel marks, pass through completely unchanged. The step sits AFTER the confusable fold, and that ordering is load-bearing. Placed before it, `а` (Cyrillic) + U+0489 (Cyrillic mark) agrees on the first pass, then the fold rewrites the base to Latin `a` and the next pass strips the mark — f(f(x)) != f(x). canonicalize_strict_idempotent caught it. Deciding against the FINAL base script is the only stable point. canonicalize_strict and strip_obfuscation now each clear the whole matrix, so TestOneCall is inverted rather than deleted: it now asserts the two sufficient entry points stay sufficient and every other one stays short. The published advice is unchanged; its reason moved from "nothing suffices" to "the two that suffice are the two most destructive ones", which is the same conclusion for a caller who has to forward the text they cleaned. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
|
📄 Docs preview: https://eea3075e.disarm-docs.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The new skip_tr39_claimed demojize path can unintentionally concatenate emoji-name output with later-folded letters (e.g. 👒€ → woman's hate), which is a correctness regression for strip_obfuscation output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR closes the two CVE vectors (#614, #615) that previously supported the documented claim that no single entry point neutralizes every vector, by adjusting comparison-preset behavior in strip_obfuscation and strengthening canonicalize_strict against cross-script combining-mark eclipsing.
Changes:
- Derive (at build time) the 49-codepoint overlap between
emoji_single.tsvand TR39 confusables, and teach comparison presets to skip naming those rows so TR39 folding wins (#614). - Add a
canonicalize_strict-only step to drop combining marks whose Script is a specific script different from their base character’s Script (#615). - Update CVE-matrix tests and security docs to reflect that
strip_obfuscationandcanonicalize_strictare now sufficient, while other entry points intentionally remain short.
File summaries
| File | Description |
|---|---|
| tests/test_cve_vectors.py | Updates CVE characterization tests and inverts the “no single call” guard into “two sufficient entry points remain sufficient”. |
| src/zalgo.rs | Adds strip_cross_script_marks_into to remove cross-script, non-Inherited combining marks (used only by canonicalize_strict). |
| src/tables/mod.rs | Adds lookup helper for the derived emoji/TR39 overlap set. |
| src/tables/emoji_data.rs | Includes the generated PHF set for the overlap codepoints. |
| src/presets.rs | Adds the new preset step and extends demojize step config with skip_tr39_claimed; wires both into canonicalize_strict / strip_obfuscation. |
| src/pipeline.rs | Updates the pipeline demojize call for the new function signature (passes skip_tr39_claimed=false). |
| src/emoji.rs | Extends demojize_rust_into to optionally skip naming the TR39-overlap codepoints so later confusable folding can act on them. |
| proptest-regressions/presets.txt | Records a new proptest regression seed related to idempotency ordering changes. |
| docs/security/cve-validation.md | Updates security guidance and examples reflecting the closed mirror pair and the new “two calls suffice” framing. |
| CHANGELOG.md | Documents the behavioral/security fixes and the rationale/order constraints. |
| build.rs | Generates the overlap set as an intersection and asserts the overlap size is exactly 49 as a drift gate. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ore it (#614) From review on #632. The separator decision looked at what the character IS, not what it will BECOME. The euro sign is not alphanumeric, so no space was emitted — but TR39 folds it to `e`, so "👒€" produced "woman's hat" + "e" and the later fold turned that into "woman's hate": a word present in neither the input nor any emoji name. Same shape for U+2211 -> `s` ("hats") and U+2200 -> `a` ("hata"). The fix reads the fold target rather than the raw character. Punctuation targets (U+2010 -> `-`) still take no separator, which matches how every other non-alphanumeric is emitted here, and the unspaced form now agrees with the spaced one. Only reachable through the #614 skip path, so it is a defect this branch introduced rather than a pre-existing one. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
Closes #614. Closes #615.
These had to land together. Each closes one of the exactly two vectors behind the published claim that no entry point clears everything, and
tests/test_cve_vectors.pywas written to fail loudly when either closed. Fixing one alone would have left the other failing and forced the guidance to be rewritten twice.#614 —
strip_obfuscationnamed confusables instead of folding them49 code points appear in both
emoji_single.tsvandconfusables_to_latin.tsv, and most are not emoji at all: typographic punctuation, currency, math operators, CJK brackets. They reach the emoji table from CLDRannotationsDerived, which names non-emoji characters.The spoof and the genuine host stopped being equal rather than becoming equal — CVE-2017-5383 surviving a preset documented as maximum-strength deobfuscation.
Not the fix the issue proposed
The issue suggested making the TR39 fold take precedence over the naming. Measurement says that breaks idempotency: punctuation inside emoji names (the
’in "woman’s hat") has to be folded by the confusable pass. The order is documented three times and pinned bytests/test_presets.py:434.Instead the overlap is derived at build time as an intersection of the two tables, so it cannot drift the way a curated override file would, and
demojizeskips those rows inside comparison presets only:build.rsasserts the count is 49, so a table refresh that claims another confusable source fails the build with a message rather than widening the gap silently. That is the drift gate the issue asked for.#615 —
canonicalizecannot cap its way out of an eclipsing markThe anti-zalgo step is a count. By count, one Arabic shadda is indistinguishable from one acute accent, so no threshold removes CVE-2017-7833's spoof and keeps
café.The discriminator was already in disarm's script data: strip a combining mark whose own Script is a specific script differing from its base's, and keep
Inheritedmarks, which attach to anything. That is UTS #39's mixed-script reasoning applied per grapheme rather than per string.canonicalize_strictonly. The rule is destructive for scholarly transliteration, IPA and linguistic transcription, where marks from one script legitimately sit on bases of another — the corpus least able to notice.canonicalizestays one short deliberately, with a test asserting that rather than leaving it implied.Nine legitimate samples in five scripts pass through completely unchanged, including Arabic with its own vowel marks, which is the likeliest false positive:
The ordering is load-bearing, and a property test proved it
I first placed the step before the confusable fold.
canonicalize_strict_idempotentfailed on"а҉"— Cyrillic base plus a Cyrillic mark. They agree on the first pass; then the fold rewrites the base to Latina, and the next pass strips the mark.f(f(x)) != f(x).Deciding against the final base script is the only stable point, so the step now runs after the fold. Worth recording, because the bug is invisible to any example that does not involve a base the fold rewrites.
The guard is inverted, not deleted
test_no_single_entry_point_clears_everythingassertedassert missed, f"{name} now clears everything — update the guidance". It did exactly what it was written to do. It now asserts that the two sufficient entry points stay sufficient and every other one stays short, so a regression in either direction fails.Measured over the full 24-row matrix:
canonicalize_strictandstrip_obfuscationeach clear everything.catalog_keymisses only the Tags block;canonicalizemisses only the eclipsing mark.The published advice is unchanged; its reason moved. From "nothing suffices" to "the two that suffice are the two most destructive ones" — the same conclusion for a caller who has to forward the text they cleaned.
Verification
4,818 pytest, 24 Rust test targets (674 unit tests),
cargo fmt --check, clippy clean on both feature sets,scripts/perf_lint.shclean, mypy clean, ruff clean, language-consistency audit clean.docs/fences execute under Sybil, so the rewritten guidance is checked rather than asserted — 43 blocks pass.