Chromium

[Empty comment from Monorail migration]

[Monorail components: Blink>HTML>Parser]

Setting Pri-2 to match security severity Low. If this is incorrect, please reset the priority. Sheriffbot won't make this change again.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Hey, any update on this issue?

It is already public (https://research.securitum.com/dompurify-bypass-using-mxss/ )  and lead to more bypasses of DOMPurify (and possibly other sanitizers).

chrishtr: Can core/render team help with this bug? I'm a bit busy these two weeks working on a launch.

->Mason for triage/next steps.

Sorry for the delay here, I will take a look.

Adding jun.kokatsu@microsoft.com, who has some thoughts on this.

[Comment Deleted]

Sorry - I haven't had a chance to look into this yet. Will start now.

So it would seem that the current behavior is "correct" according to spec:
 -  In the "in body" insertion mode [1], for a </p> tag, if the stack of open elements does not have a p element in button scope, then this is a parse error; insert an HTML element for a "p" start tag token with no attributes. Close a p element.
 - When parsing tokens in a foreign context (e.g. <svg>) [2], when "Any other end tag" is encountered, nothing special happens in this case, for a </p> found within an <svg>. Normal processing (the bullet point above) happens in step 7. All of the special "jumping out" behavior is specified for the start tags only, a bit higher up in [2]. For those, the behavior is: Pop an element from the stack of open elements, and then keep popping more elements from the stack of open elements until the current node is a MathML text integration point, an HTML integration point, or an element in the HTML namespace.

So... the current behavior of Firefox, and (I guess?) the desired behavior of Chrome would run counter to the current spec. Which likely means we should open a spec bug for these two cases, of </p> and </br> within a foreign context. It seems that those two (only?) should be added to the special list of start tags that "jump out".

From your blog post ([3], very helpful, by the way) it sounds like these are the only two problem tags, </p> and </br>. Is that correct?

[1] https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inbody
[2] https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inforeign
[3] https://research.securitum.com/dompurify-bypass-using-mxss/

Nice catch that it's a spec bug! And you're right, from my analysis, only </p> and </br> are problematic here.

If you open a spec bug (or you want me to do it), please let me know.

Thanks. I've opened a spec bug here: https://github.com/whatwg/html/issues/5113

Tentative fix here https://chromium-review.googlesource.com/c/chromium/src/+/1940722

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d16226271d4d501de19f019aba1c145930b45503

commit d16226271d4d501de19f019aba1c145930b45503
Author: Mason Freed <masonfreed@chromium.org>
Date: Sat Nov 30 07:48:15 2019

Fix parser mXSS sanitizer bypass for <p> and <br> within foreign context

Prior to this CL, the following code:
 <svg></p></svg>
parsed to this innerHTML: <svg><p></p></svg>

This is in contrast to this code:
 <svg><p></svg>
which parses to <svg></svg><p></p>

The fact that the </p> is left inside the <svg> allowed sanitizer
bypasses as detailed in [1]. Please also see [2] for the spec
discussion.

With this CL, </p> and </br> within a foreign context now cause
the closing of the foreign context.

[1] https://research.securitum.com/dompurify-bypass-using-mxss/
[2] https://github.com/whatwg/html/issues/5113

Bug: 1005713
Change-Id: Ic07ee50de4eb1ef19b73a075bd83785c99f4f891
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1940722
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720315}

[modify] https://crrev.com/d16226271d4d501de19f019aba1c145930b45503/third_party/blink/renderer/core/html/parser/html_tree_builder.cc
[modify] https://crrev.com/d16226271d4d501de19f019aba1c145930b45503/third_party/blink/renderer/core/html/parser/html_tree_builder_simulator.cc
[add] https://crrev.com/d16226271d4d501de19f019aba1c145930b45503/third_party/blink/web_tests/external/wpt/html/syntax/parsing/html_content_in_foreign_context.html

Based on the published blog post referenced in https://crbug.com/chromium/1005713#c3, I'm marking this public.

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

I'm marking this fixed - michal@, could you please verify? The fix (https://crbug.com/chromium/1005713#c14) landed in Canary version 80.0.3982.0.

[Empty comment from Monorail migration]

Mason, I verified the fix and couldn't find any bypass so far. Looks good to me.

Great, thanks michal@!

Unfortunately the Panel declined to reward this report

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

This issue was migrated from crbug.com/chromium/1005713?no_tracker_redirect=1

[Auto-CCs applied]
[Monorail components added to Component Tags custom field.]