sanitize urls: svg/xlink href, formaction, and data: on iframe/object#21442
sanitize urls: svg/xlink href, formaction, and data: on iframe/object#21442rootvector2 wants to merge 4 commits into
Conversation
| // SVG element tagNames are lowercase (e.g. `a`), so they never match the | ||
| // uppercase `badTags` entries unless we normalize first. | ||
| let normalizedTag = tagName === null ? tagName : tagName.toUpperCase(); | ||
| return checkURI(normalizedTag, attribute) || checkDataURI(normalizedTag, attribute); |
There was a problem hiding this comment.
if checkURI and checkDataURI did this normalization instead, would all your PRs be able to just be one PR?
There was a problem hiding this comment.
Good call. Moved the normalization into checkURI/checkDataURI and dropped the toUpperCase from requiresSanitization and sanitizeAttributeValue, so it's single sourced now.
It wouldn't fold all of them into one though. Normalizing here only closes the case-sensitivity gap. The other open ones add new entries to the lists (formaction in #21438, data: on iframe/object in #21441) or change the url stripping itself (#21433), so they're independent of the tag/attribute matching. Happy to squash the related ones if that's easier to review.
There was a problem hiding this comment.
can you add the tests from all your other PRs?
There was a problem hiding this comment.
Done. Folded in formaction (#21438) and data: on iframe[src]/object[data] (#21441) with their tests, plus the tab/newline strip from #21433. That last one only touches the fastboot url.parse path (the WHATWG parser already strips those chars), so there's no integration test that actually exercises it, which is why the original had none. Skipped #21430 since you already rebased it into #21434. Happy to put together limber reproductions for these if that helps.
There was a problem hiding this comment.
can you close the other PRs?
yes, reproduction will be required for each issue <3
There was a problem hiding this comment.
Closed #21433 just now. #21438, #21441 and #21430 were already closed, so this is the only sanitizer PR left. The one I left open is #21436, which is the routing prefix regex-escaping fix and unrelated to the sanitizer, so it's independent of this - happy to close that too if you'd rather. Reproductions for each case are posted in a comment below.
There was a problem hiding this comment.
can you update the PR description and title with the new contents of this PR?
There was a problem hiding this comment.
Done. Retitled it and rewrote the description to cover all four cases (svg/xlink, formaction, data: on iframe/object, and the tab/newline strip) with the limber repros linked.
| return (tagName === null || has(badTags, tagName)) && has(badAttributes, attribute); | ||
| // SVG element tagNames are lowercase (e.g. `a`), so they never match the | ||
| // uppercase `badTags` entries unless we normalize first. | ||
| let normalizedTag = tagName === null ? tagName : tagName.toUpperCase(); |
There was a problem hiding this comment.
this line probably isn't needed anymore
There was a problem hiding this comment.
Yeah, gone. checkURI just does has(badTags, tagName.toUpperCase()) behind the existing null check now.
simplify checkURI to inline the tag normalization, and bring in the other open sanitization gaps so they live behind the same tag/attribute matching: - formaction on button/input - data: protocol on iframe[src] and object[data] - strip ascii tab/newline/cr before the fastboot url protocol check
rootvector2
changed the title
|
Folded the Click the box and on a released Glimmer it fires; with this PR the href comes back as |
2 participants
Consolidates the open URL-sanitizer fixes into one PR (per review). Each change closes a way an attacker-controlled value can reach a dangerous protocol that the sanitizer currently misses.
checkURI/checkDataURImatchedelement.tagNameagainst the uppercasebadTagslist, but SVG tagNames come through lowercase (a), so<a href={{value}}>inside an<svg>skipped thejavascript:/vbscript:check. Normalization now happens insidecheckURI/checkDataURIso it is single-sourced. Also addedxlink:hrefto the attribute list, since that is the SVG href alias and was not covered at all.button[formaction]andinput[formaction]submit to their URL, so ajavascript:value there executes. AddedBUTTON/INPUTtobadTagsandformactionto the attribute list.data:URL in these loads as a nested document and can run script. Added adata:-protocol check foriframe/object.java\nscript:runs asjavascript:. The fastbooturl.parsepath kept those chars and reported a null protocol, slipping past the check. Strip them there to match the WHATWGURLparser used on the browser path.Tests added for each case except the tab/newline strip, which only runs on the fastboot
url.parsepath (the browserURLparser already strips those chars, so no integration path exercises it).Reproductions on limber for each case are in the comment below.