Bypass via insertAdjacentText #133

securityMB · 2019-02-20T21:19:57Z

insertAdjacentHTML is covered in TrustedTypes as an unsafe sink and treated as HTML context. In the <script> tag insertAdjacentText might also be dangerous. It is worth noting that it's dangerous not only on a <script> tag itself but also if it contains any children (but probably nobody should do that anyway?).

Consider the following example:

<meta http-equiv=Content-Security-Policy
      content="trusted-types test">
<body>
<script>
  const policy = TrustedTypes.createPolicy('test', {
    // empty policy but it doesn't matter for the example
  }) 

  const script = document.createElement('script');
  const p = document.createElement('p');
  
  script.appendChild(p);
  
  script.insertAdjacentText('afterBegin', 'alert("script.insertAdjacentText");');
  p.insertAdjacentText('beforeBegin', 'alert("p.insertAdjacentText");');
  
  document.body.appendChild(script);
  // two alerts will fire
  
</script>

The text was updated successfully, but these errors were encountered:

koto · 2019-02-21T13:50:15Z

That's a good one. It's clear how to fix it for the script case, and unclear yet what to do for the p case :/ See also #47

koto · 2019-03-14T11:59:10Z

I'm trying to think of an effective solution to this bypass. It seems like creating a text node (here via insertAdjacentText on another node) should track whether this node is inside a node that enforces a type for it (I think only <script> for now). This would be the algorithm to hook into: https://dom.spec.whatwg.org/#concept-node-pre-insert

This attack requires that <p> is inside the <script> at the moment of node creation. Do you know if this works for nested nodes as well?

cc @otherdaniel

securityMB · 2019-03-24T07:52:43Z

It shouldn't work with nested nodes as only the direct child text content of a <script> tag is executed.

otherdaniel · 2019-03-25T17:38:31Z

The "direct child text content" thing is a pretty good hint; I had overlooked that.

Could we spec & implement it like the regular checks for 'afterbegin' and 'beforeend', and as a check against the parent for 'beforebegin' and 'afterend'? That's a tad awkward to implement (because irregular), but should otherwise cover this case, shouldn't it?

otherdaniel · 2019-03-25T17:42:15Z

Hrm. Which default policy should be used for 'beforebegin'/'afterend'? That suitable for the parent element, I guess?

mikesamuel · 2019-03-25T19:33:23Z

@otherdaniel Which 'afterbegin' and 'beforeend' checks do you have in mind?

koto · 2019-03-25T20:26:36Z

Hooking into pre-insert would also cover:

<meta http-equiv=Content-Security-Policy
      content="trusted-types test">
<body>
<script>
  const policy = TrustedTypes.createPolicy('test', {
    // empty policy but it doesn't matter for the example
  });
  const script = document.createElement('script');
  const t = document.createTextNode('alert("appendChild(document.createTextNode)")');
  script.appendChild(t);
  document.body.appendChild(script);
</script>

Does that help with the implementation? What do you mean regarding the default policy? There's only one, unless you mean for cross-document insertion (in that case, I'd apply the policy local to the parent element, script in this case).

koto · 2019-03-25T21:00:34Z

Actually, that should be in https://dom.spec.whatwg.org/#concept-node-insert , to also cover replaceChild.

koto · 2019-03-26T09:10:34Z

FIxing this comprehensively (e.g. guard at text node insertion to a script node) requires a slightly different approach than just hijacking the setters.

We'd have to either annotate the type that was used to set the node content (e.g. that it was a TrustedScript), clear that annotation if that node was adopted from adifferent document, and reject / convert with default policy when the node is inserted with the script as a parent without that annotation. There might still be a problem here, as the text node will not form the full body of the script, it's merged with another text nodes.

Alternatively, we trigger the default policy's createScript always after a text node is inserted with the full text of the script. That's less backwards-compatible, but I don't expect this pattern happening a lot for script nodes. It sounds ok to cover those corner cases with a default policy for simplicity.

otherdaniel · 2019-03-26T13:28:44Z

@mikesamuel: What I should have said is that 'afterbegin' and 'beforeend' should be treated as they currently are, which is: no checks.

@koto: I'm very nervous about "annotate the type that was used to set the node content" & similar. That is a massive extension of the trusted types concept, both conceptually & implementation-wise. Currently, Trusted Types sits at the boundary of any string-goes-into-DOM, while that would get us into a much larger boundary that goes through pretty much all DOM operations.

Hrm. So after re-reading all of the discussion here, and after discussing this with @mikewest:

Initially, I had seen this as a problem of only the insertAdjacentText method. The discussion here convinces me that doesn't hold, because creating text nodes and inserting those in <script> elements has all the same problems, regardless of which DOM manipulation methods are being used. I think it's more that our String -> DOM boundary doesn't quite hold when it comes to the <script> element. So I think we'd need to treat that element differently. (koto said as much)

So... the main question I have: Is this a problem of only the <script> element? Are there other elements/APIs where inserting a text node is effectively a trusted types bypass?

If it's only <script>, then we can treat that element differently: Either by disallowing text node insertions when trusted types is enabled (what Mike prefers), or by treating text node insertions (when trusted types is enabled) like a text assignment and run the default policy.

koto · 2019-03-26T13:31:37Z

For all we know, and for addressing DOM XSS, only the script nodes are tricky like that. I understand the scope creep, and agree on a workable solution you propose in the last sentence.

koto · 2019-03-26T13:36:07Z

I would lean to a default policy invocation. So you can manipulate script body in two ways:

using innerText and similar setters (with a typed object, or through a default policy).
using text node insertion (in that case a default policy will run on the concatenation of all text nodes; if it rejects, the insertion fails).

mikesamuel · 2019-03-26T17:28:01Z

So... the main question I have: Is this a problem of only the <script> element? Are there other elements/APIs where inserting a text node is effectively a trusted types bypass?

Yes. Since we have elected not to spec TrustedStylesheet, we can ignore <style> text content and HTMLStyleElement.cssText.

I'm not familiar with whether <object> text content is available to the underlying third-party code, but I'm not sure that's in scope.

mikesamuel · 2019-03-26T17:29:10Z

(Sorry, commenting in tab that I hadn't reloaded so missed koto's update.)

koto · 2019-05-06T10:40:35Z

This is addressed in Chrome via https://chromium-review.googlesource.com/c/chromium/src/+/1547746 (in progress). Adding @annevk, as this approach might actually be non-speccable.

koto · 2019-05-06T10:51:20Z

To clarify, we aim to prevent appending text nodes to script elements (which would also cover the insertAdjacentText bypass). Spec-wise, it seems like we might be able to throw on scriptNode.appendChild(aTextNode) (and other callsites that try that) by modifying the pre-insert validity algorithm. It already throws e.g. a HierarchyRequestError, so it's not guaranteed that an appendChild will always succeed, we just add an additional check.

insertAdjacentText into <script> elements can be used to bypass Trusted Types checks. This change fixed that hole by adding TT checks to related Node API methods, and changing insertAdjacentText to use the public methods. See additional discussion on the root issue in w3c/trusted-types#133 Bug: 739170 Change-Id: Ie26b2aca2d60c7e14a0118c6fd957f32928afabf

insertAdjacentText into <script> elements can be used to bypass Trusted Types checks. This change fixed that hole by adding TT checks to related Node API methods, and changing insertAdjacentText to use the public methods. See additional discussion on the root issue in w3c/trusted-types#133 Bug: 739170 Change-Id: Ie26b2aca2d60c7e14a0118c6fd957f32928afabf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1547746 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Daniel Vogelheim <[email protected]> Cr-Commit-Position: refs/heads/master@{#669210}

…3c#189)

insertAdjacentText into <script> elements can be used to bypass Trusted Types checks. This change fixed that hole by adding TT checks to related Node API methods, and changing insertAdjacentText to use the public methods. See additional discussion on the root issue in w3c/trusted-types#133 Bug: 739170 Change-Id: Ie26b2aca2d60c7e14a0118c6fd957f32928afabf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1547746 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Daniel Vogelheim <[email protected]> Cr-Commit-Position: refs/heads/master@{#669210}

…ns into <script> elements., a=testonly Automatic update from web-platform-tests [trusted types] Check text node insertions into <script> elements. insertAdjacentText into <script> elements can be used to bypass Trusted Types checks. This change fixed that hole by adding TT checks to related Node API methods, and changing insertAdjacentText to use the public methods. See additional discussion on the root issue in w3c/trusted-types#133 Bug: 739170 Change-Id: Ie26b2aca2d60c7e14a0118c6fd957f32928afabf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1547746 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Daniel Vogelheim <[email protected]> Cr-Commit-Position: refs/heads/master@{#669210} -- wpt-commits: 90a794e8e93a5e22e7cb3b2f8c79d751989fa7b0 wpt-pr: 17230

…ns into <script> elements., a=testonly Automatic update from web-platform-tests [trusted types] Check text node insertions into <script> elements. insertAdjacentText into <script> elements can be used to bypass Trusted Types checks. This change fixed that hole by adding TT checks to related Node API methods, and changing insertAdjacentText to use the public methods. See additional discussion on the root issue in w3c/trusted-types#133 Bug: 739170 Change-Id: Ie26b2aca2d60c7e14a0118c6fd957f32928afabf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1547746 Reviewed-by: Mason Freed <masonfreedchromium.org> Commit-Queue: Daniel Vogelheim <vogelheimchromium.org> Cr-Commit-Position: refs/heads/master{#669210} -- wpt-commits: 90a794e8e93a5e22e7cb3b2f8c79d751989fa7b0 wpt-pr: 17230 UltraBlame original commit: edd51bf2c9befb08253cc19fefb1f7fe64def9b3

Siegrift · 2020-11-16T10:50:54Z

This issue is not fixed properly in the polyfill:

This test fails (and alert is triggered):

  fit('insertAdjacentText not working properly', () => {
    const enforcer = new TrustedTypesEnforcer(ENFORCING_CONFIG);
    enforcer.install();

    const s = document.createElement('script');
    const p = document.createElement('p');
    s.appendChild(p);
    
    expect(() => {
      s.insertAdjacentText(
          'afterBegin',
          'alert("script.insertAdjacentText");'
      );
    }).toThrow();

    document.body.appendChild(s);

    enforcer.uninstall();
    document.body.innerHTML = '';
  });

Note, you have already a similar test, which asserts that calling insertAdjacentText fails. But it missed a case, when called on script object.

This was referenced May 3, 2019

How to handle <template>? #42

Closed

Put safeguards around attribute nodes #47

Closed

chromium-wpt-export-bot mentioned this issue Jun 7, 2019

[trusted types] Check text node insertions into <script> elements. web-platform-tests/wpt#17230

Merged

koto added this to the v1 milestone Jun 24, 2019

koto added the spec label Jun 24, 2019

koto added a commit to koto/trusted-types that referenced this issue Jun 28, 2019

Fix some variants of w3c#133 in the polyfill.

629c4fa

koto added a commit to koto/trusted-types that referenced this issue Jul 11, 2019

Fix some variants of w3c#133 in the polyfill.

96dd197

koto added a commit to koto/trusted-types that referenced this issue Jul 11, 2019

Added the text to the spec. Fixes w3c#133.

3c2ff7e

koto closed this as completed in ea0358a Jul 11, 2019

koto added a commit to koto/trusted-types that referenced this issue Jul 16, 2019

Protect text node manipulation under script elements. Fixes w3c#133. (w…

b2757a4

…3c#189)

koto mentioned this issue Oct 11, 2019

Consider something like a "Trusted Type" system for DOM manipulation. whatwg/html#3052

Closed

koto mentioned this issue Nov 16, 2020

insertAdjacentText in polyfill #300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bypass via insertAdjacentText #133

Bypass via insertAdjacentText #133

securityMB commented Feb 20, 2019

koto commented Feb 21, 2019

koto commented Mar 14, 2019

securityMB commented Mar 24, 2019

otherdaniel commented Mar 25, 2019

otherdaniel commented Mar 25, 2019

mikesamuel commented Mar 25, 2019

koto commented Mar 25, 2019

koto commented Mar 25, 2019

koto commented Mar 26, 2019

otherdaniel commented Mar 26, 2019

koto commented Mar 26, 2019

koto commented Mar 26, 2019

mikesamuel commented Mar 26, 2019

mikesamuel commented Mar 26, 2019

koto commented May 6, 2019

koto commented May 6, 2019

Siegrift commented Nov 16, 2020

Bypass via insertAdjacentText #133

Bypass via insertAdjacentText #133

Comments

securityMB commented Feb 20, 2019

koto commented Feb 21, 2019

koto commented Mar 14, 2019

securityMB commented Mar 24, 2019

otherdaniel commented Mar 25, 2019

otherdaniel commented Mar 25, 2019

mikesamuel commented Mar 25, 2019

koto commented Mar 25, 2019

koto commented Mar 25, 2019

koto commented Mar 26, 2019

otherdaniel commented Mar 26, 2019

koto commented Mar 26, 2019

koto commented Mar 26, 2019

mikesamuel commented Mar 26, 2019

mikesamuel commented Mar 26, 2019

koto commented May 6, 2019

koto commented May 6, 2019

Siegrift commented Nov 16, 2020