✨ feat(sanitize): add DOM-clobbering isolation#568
Merged
Conversation
Policy.isolate_named_props prefixes every kept id and name value with user-content-, DOMPurify's SANITIZE_NAMED_PROPS. It defuses DOM clobbering: an attacker-set id or name whose value matches a built-in document or form property shadows that property through named access, and the allowlist keeps the attribute because it is otherwise ordinary. Prefixing moves the value out of the property namespace so no value can collide. An already-prefixed value is left alone, so re-sanitizing is a fixpoint. turbohtml sanitizes a parsed tree with no DOM to probe, so it takes the static SANITIZE_NAMED_PROPS design rather than SANITIZE_DOM's live value-in-document check. The prefix runs after attribute_filter in the existing single C walk, below the caller's reach, and the safety baseline stays unconditional. closes tox-dev#534
for more information, see https://pre-commit.ci
Merging this PR will not alter performance
Performance Changes
Comparing Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sanitizing against an allowlist keeps
idandnameattributes because nothing about them is malformed, so a value an attacker picks to match a built-in property survives the walk. That value then shadows the property through named access:<input name="attributes">makesform.attributesresolve to the input,<img name="body">hidesdocument.body, and<a id="location">can stand in fordocument.location. 🛡️ DOMPurify hardens against this withSANITIZE_NAMED_PROPS; turbohtml had no equivalent, so a policy that allowedid/nameon kept elements left the collision in place.Policy.isolate_named_propscloses the gap by prefixing every keptidandnamevalue withuser-content-, moving it out of the property namespace so no value can collide. turbohtml sanitizes a parsed tree with no DOM to probe, so it takes the staticSANITIZE_NAMED_PROPSdesign rather thanSANITIZE_DOM's livevalue in documentcheck: the prefix is unconditional and complete, where a static reimplementation of that check would be a hand-maintained name list that misses whatever property a future engine adds. A value already carrying the prefix is left alone, so re-sanitizing is a fixpoint.The prefix runs in the existing single C walk, after
attribute_filterhas had its say, so the value a filter returns is what gets namespaced and no filter can hand back a clobbering value the isolation then fails to prefix. 🔒 The option is off by default and the safety baseline stays unconditional:on*handlers, scripting elements, andjavascript:URLs are removed regardless of the policy, so isolation is one more layer rather than the only one.closes #534