Prevent custom elements from being considered formatting elements in HTML parser. [chromium/src : main]
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
Thanks to Arthur Sonzogni for the tests, originally from
https://crrev.com/c/7706938@arthurs...@chromium.org, I'm noticing that without this change I'm only seeing two of the tests fail: `adoption_agency_shift.html` with a `DCHECK failed: Contains(old_element)` and `adoption_agency_uaf.html` with a SEGV. I'm curious if some of the other tests were failing for you in a different configuration?
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 4 comments![]( "Open in Gerrit")
The code looks good, and I agree it'll fix the bug. Just some questions.
+securitymb@ for the mXSS question
// Disallow custom elements from being on the stack of active formatting
// elements.You might consider also saying that this isn't per-spec, but is a corner case we're willing to live with? Or something to that effect?
if (token->GetAttributeItem(html_names::kIsAttr) &&Per my offline comment, my concern is that this might (?) introduce mXSS concerns? I.e. the behavior within trees of nested formatting elements will be different, depending on whether the formatting element has `is=something`. So if a sanitizer is stripping out `is` attributes, and then serializing the resulting tree, the resulting HTML will be parsed into a different tree because of the lack of `is`.
@securitymb for comments.
Perhaps just `-crash.html` rather than `_uaf.html`? This test doesn't actually make real use of `testharness`. (Also replace "use after free" with "crash" in the content of the test?)
This comment applies to a few tests above. Perhaps all of them.
Related details
- David Baron
- Michał Bentkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Bentkowski (Gerrit)
Michał Bentkowski added 2 comments![]( "Open in Gerrit")
Removing myself from reviewers as I don't actually have permission to vote.
if (token->GetAttributeItem(html_names::kIsAttr) &&Per my offline comment, my concern is that this might (?) introduce mXSS concerns? I.e. the behavior within trees of nested formatting elements will be different, depending on whether the formatting element has `is=something`. So if a sanitizer is stripping out `is` attributes, and then serializing the resulting tree, the resulting HTML will be parsed into a different tree because of the lack of `is`.
@securitymb for comments.
I think this should be fine. The list of active formatting elements is meant to handle mis-nested tags. So the sanitizer should create a valid HTML structure (i.e. no mis-nested tags) and on re-parsing this algorithm shouldn't kick in.
Related details
- David Baron
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 2 comments![]( "Open in Gerrit")
// Disallow custom elements from being on the stack of active formatting
// elements.You might consider also saying that this isn't per-spec, but is a corner case we're willing to live with? Or something to that effect?
Better, I filed https://github.com/whatwg/html/issues/12327 and I'll link to it.
Perhaps just `-crash.html` rather than `_uaf.html`? This test doesn't actually make real use of `testharness`. (Also replace "use after free" with "crash" in the content of the test?)
This comment applies to a few tests above. Perhaps all of them.
Moved to `crashtests` subdirectory, renamed, changed them to use crashtest infrastructure (`class="test-wait"` and `TestRendered` event).
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
// Disallow custom elements from being on the stack of active formatting
// elements.David BaronYou might consider also saying that this isn't per-spec, but is a corner case we're willing to live with? Or something to that effect?
Better, I filed https://github.com/whatwg/html/issues/12327 and I'll link to it.
even better
if (token->GetAttributeItem(html_names::kIsAttr) &&Michał BentkowskiPer my offline comment, my concern is that this might (?) introduce mXSS concerns? I.e. the behavior within trees of nested formatting elements will be different, depending on whether the formatting element has `is=something`. So if a sanitizer is stripping out `is` attributes, and then serializing the resulting tree, the resulting HTML will be parsed into a different tree because of the lack of `is`.
@securitymb for comments.
I think this should be fine. The list of active formatting elements is meant to handle mis-nested tags. So the sanitizer should create a valid HTML structure (i.e. no mis-nested tags) and on re-parsing this algorithm shouldn't kick in.
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |