Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unmatched </p> or </br> inside foreign context needs a special parser rule #5113

Open
mfreed7 opened this issue Nov 27, 2019 · 6 comments
Open

Unmatched </p> or </br> inside foreign context needs a special parser rule #5113

mfreed7 opened this issue Nov 27, 2019 · 6 comments

Comments

@mfreed7
Copy link

@mfreed7 mfreed7 commented Nov 27, 2019

As the current parser spec is written, <svg></p></svg> and <svg></br></svg> both result in <p> and <br> DOM nodes as children of the <svg>. As mentioned in this Chromium bug and this blog post, this can be exploited as a sanitizer bypass. Here is an example DOM Viewer link showing the behavior.

By my reading of the spec:

  • In the "in body" insertion mode, 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. (This adds the <p> within <svg>.)
  • When parsing tokens in a foreign context, 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 the foreign context section. 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. (This is what causes a <p> found within <svg> to close the </svg> and leave the <p> outside.)

Current implementations:

  • Blink leaves the <p> or <br> inside <svg>.
  • Webkit leaves the <p> or <br> inside <svg>.
  • Gecko (correctly?) moves the <p> or <br> outside the <svg>.

I believe the spec should follow current Gecko behavior. I think the easiest way to change the spec would be to add a special case within the foreign context section for end tags whose tag name is "p" or "br", which closes the foreign context and then processes the </p> or </br> as normal for a non-foreign context.

@domenic

This comment has been minimized.

Copy link
Member

@domenic domenic commented Nov 27, 2019

@bathos

This comment has been minimized.

Copy link

@bathos bathos commented Nov 27, 2019

(Additional impl data:) parse5 is currently consistent with Blink/Webkit

@mfreed7

This comment has been minimized.

Copy link
Author

@mfreed7 mfreed7 commented Nov 27, 2019

I should also have referenced the corresponding Chromium bug.

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 27, 2019
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] whatwg/html#5113

Bug: 1005713
Change-Id: Ic07ee50de4eb1ef19b73a075bd83785c99f4f891
@sideshowbarker

This comment has been minimized.

Copy link
Member

@sideshowbarker sideshowbarker commented Nov 28, 2019

html5lib leaves the <p> or <br> inside <svg>:

$ echo "<svg></p></svg>" | python -c "from sys import stdin; \
import html5lib; from lxml import html; \
doc = html5lib.parse(stdin, treebuilder='lxml', namespaceHTMLElements=False); \
print html.tostring(doc)"
<html><head></head><body><ns0:svg xmlns:ns0="http://www.w3.org/2000/svg"><p></p></ns0:svg>
</body></html>

$ echo "<svg></br></svg>" | python -c "from sys import stdin; \
import html5lib; from lxml import html; \
doc = html5lib.parse(stdin, treebuilder='lxml', namespaceHTMLElements=False); \
print html.tostring(doc)"
<html><head></head><body><ns0:svg xmlns:ns0="http://www.w3.org/2000/svg"><br></ns0:svg>
</body></html>
aarongable pushed a commit to chromium/chromium that referenced this issue Nov 30, 2019
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] whatwg/html#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}
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 30, 2019
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] whatwg/html#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}
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 30, 2019
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] whatwg/html#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}
@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Dec 2, 2019

@mfreed7 could you copy @hsivonen and I on the Chrome issue (or unhide it now that you and others essentially made it public)?

@mfreed7

This comment has been minimized.

Copy link
Author

@mfreed7 mfreed7 commented Dec 2, 2019

No problem - I just marked it all public. I had essentially been treating it as public given the blog post.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 5, 2019
… and <br> within foreign context, a=testonly

Automatic update from web-platform-tests
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] whatwg/html#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}

--

wpt-commits: 561b765308e6d188618f3ba73091bb598d8357ce
wpt-pr: 20489
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Dec 5, 2019
… and <br> within foreign context, a=testonly

Automatic update from web-platform-tests
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] whatwg/html#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}

--

wpt-commits: 561b765308e6d188618f3ba73091bb598d8357ce
wpt-pr: 20489
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.