๐ก๏ธ Sentinel: [HIGH] Fix BiDi spoofing vulnerability - #735
seonghobae wants to merge 2 commits into
Conversation
|
๐ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a ๐ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reachedNext included review available in 54 minutes. View limit detailsLimit details: Youโve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: โ๏ธ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ๐ Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
ํ์ฌ exact head d388fa9cae95b84ad3dc81761b76ebedf7bfd1a5์ isolate ์ ์ฉ ๋ฐฉํฅ์ ๋ง์ง๋ง, escapeHtml()์ด ์ฐจ๋จํ๋ BiDi control set์ด ๋ถ์์ ํฉ๋๋ค. ์ง๊ธ์ U+202AโU+202E์ U+2066โU+2069 9๊ฐ๋ง literal \\uXXXX๋ก ๋ฐ๊พธ๊ณ , Unicode๊ฐ Bidi_Control๋ก ์ ์ํ๋ U+061C ARABIC LETTER MARK (ALM), U+200E LEFT-TO-RIGHT MARK (LRM), U+200F RIGHT-TO-LEFT MARK (RLM) 3๊ฐ๋ ๊ทธ๋๋ก ๋ ๋๋ง๋ฉ๋๋ค.
Unicode UAX #9 ยง2๋ directional formatting characters๋ฅผ ์ธ ๊ทธ๋ฃน์ผ๋ก ๋ช
์ํฉ๋๋ค: implicit marks LRM/RLM/ALM, embedding/override LRE/RLE/LRO/RLO/PDF, isolate LRI/RLI/FSI/PDI. ํ์ฌ PR์ ๋ค ๋ ๊ทธ๋ฃน๋ง ์ฒ๋ฆฌํฉ๋๋ค. ์ต์ normative reference: https://unicode.org/reports/tr9/#Directional_Formatting_Characters . Unicode Core Spec์ Bidirectional Ordering Controls ํ๋ ์ ํํ 12๊ฐ๋ฅผ ์ด๊ฑฐํฉ๋๋ค: https://unicode.org/versions/Unicode18.0.0/core-spec/chapter-23/#G14116 . ๋ฐ๋ผ์ PR ์ ๋ชฉ/CHANGELOG์ โBiDi control charactersโ ๋ฐฉ์ด๋ฅผ ํ์ฌ head๋ก ์๋ฃํ๋ค๊ณ ๋ณด๊ธฐ ์ด๋ ต์ต๋๋ค.
FSI/PDI๋ก untrusted filename์ ๋ฐ๊นฅ ๋ฌธ๋งฅ๊ณผ ๊ฒฉ๋ฆฌํ๋ฉด spillover๋ฅผ ํฌ๊ฒ ์ค์ผ ์ ์์ง๋ง, isolate ๋ด๋ถ์์ ๋จ์ ์๋ implicit directional mark๋ filename ์์ฒด์ ์ซ์/๋ฌธ์ฅ๋ถํธ ordering์ ๋ฐ๊ฟ ์ ์์ต๋๋ค. ํนํ ํ์ผ๋ช /ํ์ฅ์ spoofing์ threat๋ก ์ก์๋ค๋ฉด ์ด 3๊ฐ๋ hostile corpus์ ํฌํจํด์ผ ํฉ๋๋ค.
RED: Unicode์ 12๊ฐ Bidi_Control ๊ฐ๊ฐ์ directory name/file name/title/visible link text์ ์ฃผ์
ํด generated index.html์ raw attacker-supplied control code point๊ฐ 0๊ฐ์ธ์ง ๊ฒ์ฌํ์ญ์์ค. ๋จ์ escapeHtml() unit test๋ฟ ์๋๋ผ ์ค์ process_dir output๊ณผ browser rendering์ ํ์ธํ๊ณ , ์ ์ RTL/Arabic/Hebrew ํ์ผ๋ช
(์ ์ด๋ฌธ์ ์์ด ์์ฐ ๋ฌธ์๋ฅผ ์ฐ๋ ๊ฒฝ์ฐ)์ ํผ์ํ์ง ์๋ parity๋ ๊ณ ์ ํ์ญ์์ค. ํผํฉ report\u200Fcod.exe, safe\u061C123.jpg, nested explicit/isolate sequence ๊ฐ์ hostile case๋ ํฌํจํ๋ ํธ์ด ๋ง์ต๋๋ค.
GREEN: attacker input์ ์ ์ฒด Bidi_Control ์งํฉ์ ์ผ๊ด๋๊ฒ neutralize/escapeํ๊ณ , app์ด ์์ฒด ์ฝ์
ํ๋ isolate boundary๋ง ๋ณ๋ trusted rendering primitive๋ก ์ ์งํ์ญ์์ค. ๊ฐ๋ฅํ๋ฉด escapeHtml() ์ผ๋ฐ HTML escaping๊ณผ renderUntrustedDisplayText() ๊ฐ์ BiDi display policy๋ฅผ ๋ถ๋ฆฌํด, ํฅํ ๋ค๋ฅธ caller๊ฐ HTML escape๋ง์ผ๋ก spoofing-safe๋ผ๊ณ ์คํดํ์ง ์๊ฒ ํ๋ ๊ฒ์ด ์ข์ต๋๋ค. ํ์ฌ .jules/sentinel.md์ finding ๋ ์ง 2024-05-24๋ 2026-09 current finding๊ณผ ๋ง์ง ์์ ์ค์ ๊ด์ฐฐ์ผ๋ก currentizeํด์ผ ํฉ๋๋ค.
Security gate๋ ์ด exact generation์์ FAIL์ ๋๋ค.
seonghobae
left a comment
There was a problem hiding this comment.
Security/PR-0 acceptance is RED on exact bbd227a3cd551d52fab8ea12d4bdfd37bde5c852.
This is a second writer on the same BiDi rendering boundary already owned by #705 (0de5422d3276e419831f28148cfbd6ffd5b66904, same protected base). #705 carries an additional valid semantic delta that this head does not fully inherit: the link title value isolates the user-controlled filename (⁨...⁩) before the trusted type label. #735 instead removes that isolation from ariaLabel and relies on escapeHtml() to neutralize a subset of controls. A successor cannot make #705 PR=0 until every valid source/test/fixture/evidence delta is ordinary-forwarded.
The neutralization set is also incomplete if the security contract is "escape BiDi controls". Unicode 18.0 ยง23.2.5 and current UAX #9 define the Bidi_Control set as ALM U+061C, LRM U+200E, RLM U+200F, LRE/RLE/PDF/LRO/RLO U+202AโU+202E, and LRI/RLI/FSI/PDI U+2066โU+2069. This head handles only the latter two groups. Primary references: https://unicode.org/versions/Unicode18.0.0/core-spec/chapter-23/ and https://www.unicode.org/reports/tr9/.
RED: render filenames containing every Bidi_Control, mixed RTL/LTR basename + extension, nested controls, unmatched controls, and ordinary Arabic/Hebrew text through the actual generated HTML; assert both DOM/text semantics and browser visual containment in Chromium/Firefox/WebKit. Include the visible <span>, <title>/<h1>, and the link title/accessible-name boundary. Ordinary RTL filenames must remain readable; security repair must not flatten legitimate script direction.
GREEN: choose one canonical BiDi owner (#705 or this PR), merge the valid isolation + explicit-neutralization contracts into that one descendant, cover the complete declared control set (or narrow the claim to the exact controls intentionally neutralized), and preserve all sibling tests/evidence before considering the other PR PR=0. Browser/a11y evidence is still required; source-string assertions alone do not prove the rendered filename cannot spoof a trusted suffix.
Delivery Gate on this exact head: intentionality PASS; functional/security completeness FAIL; resilience/browser+a11y evidence FAIL; claim evidence FAIL until the control-set contract and sibling lineage are reconciled.
๐จ Severity: HIGH
๐ก Vulnerability: ์์ฑ๋
index.html์์ ์๋ฐฉํฅ ํ ์คํธ(BiDi) ์ ์ด ๋ฌธ์(์:\u202ERTL override)๋ฅผ ํ์ฉํ์ฌ ์ ์์ ์ธ ์ฌ์ฉ์๊ฐ ํ์ผ ์ด๋ฆ๊ณผ ํ์ฅ์๋ฅผ ์คํธํํ๊ฑฐ๋ ๋๋ ํํ ์ ์๋ ์ทจ์ฝ์ ์ด ๋ฐ๊ฒฌ๋์์ต๋๋ค. ์ด๋ฅผ ํตํด ์ ์ฑ ์คํ ํ์ผ์ ์์ ํ ์ด๋ฏธ์ง ํ์ผ์ฒ๋ผ ๋ณด์ด๊ฒ ์์ผ ์ ์์ต๋๋ค.๐ฏ Impact: ์ฌ์ฉ์๊ฐ ๋๋ ํ ๋ฆฌ ๋ชฉ๋ก์ ์ด๋ํ ๋ ์๊ฐ์ ์ผ๋ก ๋ณ์กฐ๋ ํ์ผ ์ด๋ฆ์ ์ ๋ขฐํ์ฌ ์ ์ฑ ํ์ผ์ ๋ค์ด๋ก๋ํ๊ฑฐ๋ ์คํํ๋๋ก ์ ๋๋ ์ ์์ต๋๋ค.
๐ง Fix:
escapeHtml()ํจ์์ BiDi ์ ์ด ๋ฌธ์(LRE, RLE, PDF, LRO, RLO, LRI, RLI, FSI, PDI)๋ฅผ ๋ ๋๋ง๋์ง ์๋ ์์ ํ ์ด์ค์ผ์ดํ ์ ๋์ฝ๋ ๋ฌธ์์ด(\u202E๋ฑ)๋ก ๋ณํํ๋ ๋ก์ง์ ์ถ๊ฐํ์ต๋๋ค.process_dir๋ด์์ ๋๋ ํ ๋ฆฌ ์ด๋ฆ ๋ฐ ํ์ผ ์ด๋ฆ์ด ๋ ๋๋ง๋๋ ๋ชจ๋ ์์น(<h1>,<title>,<a>ํ๊ทธ ๋ด<span>)์ First Strong Isolate (⁨)์ Pop Directional Isolate (⁩)๋ฅผ ์ถ๊ฐํ์ฌ, ์ฌ์ฉ์๊ฐ ์ ์ดํ๋ ํ ์คํธ ๋ฐฉํฅ์ด ๋ฌธ์ ์ ์ฒด ๊ตฌ์กฐ์ ์ํฅ์ ๋ฏธ์น์ง ์๋๋ก ๊ฒฉ๋ฆฌ(Isolate)ํ์ต๋๋ค.BidiEscapeTest.kt) ๋ฐ 100% ์ปค๋ฒ๋ฆฌ์ง ์ ์ง๋ฅผ ์ํ ํ ์คํธ ๊ฒ์ฆ์ ์๋ฃํ๊ณ CHANGELOG ๋ฐ.jules/sentinel.md๋ฅผ ์ ๋ฐ์ดํธํ์ต๋๋ค.โ Verification: ํ ์คํธ(
./gradlew test jacocoTestReport jacocoTestCoverageVerification)๊ฐ 100% ์ปค๋ฒ๋ฆฌ์ง์ ํจ๊ป ์ฑ๊ณต์ ์ผ๋ก ํต๊ณผํ๋ฉฐ, ์ถ๊ฐ๋BidiEscapeTest๊ฐ ์๋๋ ๋ฌธ์์ ์ ํํ ์ด์ค์ผ์ดํ๋ฅผ ๊ฒ์ฆํฉ๋๋ค.PR created automatically by Jules for task 6814770744237234256 started by @seonghobae