Put safeguards around attribute nodes

[mikesamuel]

There are two cases where moving a node from one parent to another might be problematic.

const div = document.createElement('div')
div.appendChild(document.createTextNode('alert(1)'));
const script = document.createElement('script')
while (div.firstChild) {
  script.appendChild(div.firstChild);
}

We need to be suspicious of append to <script> elements regardless, but there's also a problem with attributes.

const div = document.createElement('div');
const a = document.createElement('a');

div.setAttribute('href', 'javascript:alert(1)');
const attr = div.getAttributeNode('href');
div.removeAttributeNode(attr);

a.setAttributeNode(attr);

But what about when a node comes from one context to a similar context?

const a0 = document.createElement('a');
const a1 = document.createElement('a');

a0.setAttribute('href', policy.createURL('http://example.com'));
const attr = a0.getAttributeNode('href');
a0.removeAttributeNode(attr);

a1.setAttributeNode(attr);

Should we support this kind of transparent DOM restructuring?

[koto]

Perhaps we can simply disable the offending functions? At least the ones related to attribute nodes seem to be on the way to being deprecated in DOM4.

[mikesamuel]

Disabling setAttributeNode would work.

myElement.attributes also exposes Attrs so we might need to guard the Attr.value setter regardless.

aElement.attributes.href.value = ...

aElement.getAttributeNode('href').value = ...

[koto]

The text node in scripts vector should be covered in #133 (https://chromium-review.googlesource.com/c/chromium/src/+/1547746)

Regarding attribute node deprecation, this is probably not happening soon in practice: https://github.com/search?l=JavaScript&q=setAttributeNode&type=Code. Assuming they are here to stay, and are used it sounds like we need to guard them.

It seems like the attribute nodes are immutable, apart from their value (in specific, changing the name, localName or namespaceURI doesn't have any effect). Given that, we could be able to secure this by:

• requiring the type at document.createAttribute(NS) and at Attr.value setter, assuming that for HTML namespace src attribute requires TrustedURL.
• disallowing setAttributeNode on script elements, totally, or if attribute node name is src (as the type for script.src is different).

A more elegant fix would be to require TrustedURL|TrustedScriptURL on src nodes, tracking which type was used, and checking that with the element contract on setAttributeNode.

cc @otherdaniel

[koto]

Closing this, see #50 (comment) for rationale (for cross-realm nodes). As for the script node text manipulation via child nodes, we addressed this in #133.

[koto]

Reopening for attribute nodes. We should

• Forbid setAttributeNode (or route their value through default policy like for script texts) on content attributes that are reflected by one of covered attributes, e.g.
  • iframe.srcdoc
  • script.src
• Same for attribute node value setters.

[koto]

A possible solution outlined in whatwg/dom#789 (comment)

[koto]

Closing this, the followup is in whatwg/dom#809.

[bathos]

If I understand right this was resolved without disabling setAttributeNode, right?

I’m looking to confirm because currently setAttributeNode is (afaik) the only way to set attributes with names that don’t pass the “Name production in XML”* step in setAttribute/setAttributeNS.

* Or rather the unspecified(?) variant of that grammar which browsers really implement in practice. For example, 💩 is a valid HTML attribute name and a valid XML name, but all browsers today seem to think it’s not the latter).

[koto]

The value of the node goes through a default policy, if the elem it's attaching to + attribute name matches what needs Trusted Types. If there's no default policy (or the value is rejected by it), setAttributeNode will throw:

http://gadgets.kotowicz.net/poc/tt/attributenode.html