allow arbitrary attributes and their values for configured custom elements #602

franktopel · 2021-11-24T18:23:59Z

also added basic custom element tagname check so we don't open the floodgates with ALLOWED_CUSTOM_ELEMENTS: /script/

This pull request fixes #601, which is a followup for #596.

Dependencies

If there are any dependencies on PRs or API work then list them here.

no dependencies

franktopel · 2021-11-24T18:42:07Z

Did I mention I hate prettier? 🗡️

…lements * added basic custom element tagname test so we don't open the floodgates with `ALLOWED_CUSTOM_ELEMENTS: /script/` adjusted tests added comments explaining what is going on new build fixed broken test

cure53 · 2021-11-25T07:21:57Z

Good morning, just quick FYI, I removed the conflicts, no other PRs expected from my end today, so things should stay conflict-free :)

franktopel · 2021-11-25T07:34:29Z

test/test-suite.js

@@ -593,15 +593,15 @@ module.exports = function (DOMPurify, window, tests, xssTests) {
    );
    assert.equal(
      DOMPurify.sanitize(
-        '<foo-bar baz="foobar"></foo-bar><div is="foo-baz"></div',
-        { ALLOWED_CUSTOM_ELEMENTS: (tagName) => tagName.startsWith('foo-') }
+        '<foo-bar bas="foobar"></foo-bar><div is="foo-baz"></div>',


bas should be baz

franktopel · 2021-11-25T07:34:43Z

test/test-suite.js

      ),
      '<foo-bar baz="foobar"></foo-bar><div is="foo-baz"></div>'
    );
    assert.equal(
      DOMPurify.sanitize(
-        '<foo-bar baz="foobar"></foo-bar><div is="foo-baz"></div>',
-        { ALLOWED_CUSTOM_ELEMENTS: (tagName) => tagName.endsWith('-bar') }
+        '<foo-bar bas="foobar"></foo-bar><div is="foo-baz"></div>',


bas should be baz

Should I fix real quick?

Yes, please. I'd be happy not to have to do another fork and PR for this.

franktopel · 2021-11-25T07:51:42Z

Good morning, just quick FYI, I removed the conflicts, no other PRs expected from my end today, so things should stay conflict-free :)

Thank you for that, but some changes you introduced make two tests fail, see my review comments above.

franktopel · 2021-11-25T08:09:43Z

I still have the conflicted version on my hard drive, not sure how to test without fixing that first.

franktopel · 2021-11-25T08:13:15Z

Just tested that agains our "components onepager", looking fine to me.

To make this config option a little more secure, I think we need to make a better check if it's a custom element in the future, without introducing a massive performance drop with the option enabled.

cure53 · 2021-11-25T08:15:19Z

What risks do you presently see here - i.e. how would the existing check be bypassable?

franktopel · 2021-11-25T08:17:09Z

I'm not a security person, and thus, I have no idea. But I know there's alot of very creative ways to bypass security measures implemented by people whose main concern isn't security.

cure53 · 2021-11-25T08:25:33Z

So, what I think will bite us in the ass right now is this: https://w3c.github.io/mathml-core/#dfn-annotation-xml

Because, this would be falsely detected as a custom element, no?

franktopel · 2021-11-25T08:29:23Z

The basic check would let that pass, because it only detects the presence of a -. How is that a problem? Also I even think annotation-xml by all means would be a valid custom element name. I'd expect that one to pass even Sindre's https://github.com/sindresorhus/validate-element-name

cure53 · 2021-11-25T08:32:05Z

Not sure yet, but it's not a custom element but it will be detected as one. And Firefox fully supports it via MathML.

Further, are we sure we want to really allow any attribute name for custom elements? Like, why do we allow onclick and the likes for custom elements just so?

franktopel · 2021-11-25T08:33:27Z

Further, are we sure we want to really allow any attribute name for custom elements?

Probably we ideally wouldn't, but remember all this only happens if a user decides they need/want that.

Also, if the attribute is in the forbid list, isn't it still eradicated?

Need to walk the dog now, back in 20 minutes.

cure53 · 2021-11-25T08:34:54Z

Further, are we sure we want to really allow any attribute name for custom elements?

Probably we ideally wouldn't, but remember all this only happens if a user decides they need/want that.

Correct, but the user has likely no idea that this exists. Also, will the user be aware that this will work?

DOMPurify.sanitize(
    '<annotation-xml><x-y onclick=alert(1)></x-y></annotation-xml>', 
    {ALLOWED_CUSTOM_ELEMENTS: /\w+-\w+/ }
)

cure53 · 2021-11-25T08:35:25Z

Also, if the attribute is in the forbid list, isn't it still eradicated?

There is no forbid-list. We only have an allow-list.

franktopel · 2021-11-25T08:36:37Z

It should be quite obvious to everyone that this will open the floodgates: /\w+-\w+/

cure53 · 2021-11-25T08:38:14Z

I am talking about the attributes, mostly. Like, same thing here:

DOMPurify.sanitize('<x-y onclick=alert(1)>AAA</x-y>', {ALLOWED_CUSTOM_ELEMENTS: /-y$/ })

I am fairly fine with the element checks, we might have to add a case for <annotation-xml> to be on the safe side, but I am not sure if attributes are handled properly right now. Should there not better be a regex or predicate as well for those?

franktopel · 2021-11-25T09:29:18Z

A regex or predicate for that imo would only serve one purpose: It would make it clearer to the user what they are allowing. It is practically impossible to whitelist those attributes.

On the other hand it would also slow down the purification process, and it would make the whole feature harder to use. It's you library, so it's up to you to decide.

I just need a solution that allows me to a) use DOMPurify with custom elements, and through this, allows me b) to enable Trusted Types.

If that helps, we could check if lcName matches /^on/, but that would also eradicate attributes like once="true", which is likely to produce follow-up issues.

cure53 · 2021-11-25T09:32:12Z

So, right now we allow any attribute in case an element passes a certain check. This cannot stay as is. We must that it is not possible too use custom elements to smuggle in risky attribute, there is no way around that.

If that helps, we could check if lcName matches /^on/, but that would also eradicate attributes like once="true", which is likely to produce follow-up issues.

This is a terrible idea and we cannot do that.

cure53 · 2021-11-25T09:33:50Z

I see only one way here: Re-enforce the attribute allow-list for custom elements and give a developer the chance to allow-list more (we have that already) and potentially define a wider pattern (i.e. via regex or alike).

franktopel · 2021-11-25T09:33:54Z

If you want to discuss the issue on the phone, feel free to give me a call. Find my contact details on https://connexo.de/kontakt.php

cure53 · 2021-11-25T09:50:13Z

Thanks for the good call, proposal currently is:

Additional config option to specify a regex for attribute names (default: allow-list is enforced)
Rename existing config option to something more fitting (maybe CUSTOM_ELEMENT_ALLOW_TAGS, CUSTOM_ELEMENT_ALLOW_ATTR)

franktopel · 2021-11-25T09:53:45Z

Bundling related config options in an object would be what I would do:

CUSTOM_ELEMENT_HANDLING: Object.seal(Object.create(null, {
  tagNameCheck: { writable: true, configurable: false, enumerable: true, value: null },
  attributeNameCheck:  { writable: true, configurable: false, enumerable: true, value: null },
  allowCustomizedBuiltInElements:  { writable: true, configurable: false, enumerable: true, value: false },
}))

cure53 · 2021-11-25T09:57:38Z

Hmmm, this looks reasonable.Works for me! As long as the defaults are strict and prohibitive, this should be good.

franktopel · 2021-11-25T10:00:40Z

Just out of curiosity, why is prohibiting /^on/ a terrible idea? This is probably what I'd configure to disallow in my attributeNameCheck.

cure53 · 2021-11-25T10:02:24Z

a) Block-lists are by design dysfunctional in security
b) There is way more attributes that execute JavaScript than on* (href, action, formaction, values, etc.)

franktopel · 2021-11-25T10:15:41Z

I assume I can't make use of Object.assign since it's not supported in any IE?

cure53 · 2021-11-25T10:18:04Z

Yes, I kicked MSIE10 out last night but MSIE11 we cannot give up on yet.

franktopel · 2021-11-25T11:13:28Z

Does DOMPurify in certain cases change the order of attributes?

Getting this test fail:

expected: "<foo-bar baz=\"foobar\" forbidden=\"true\"></foo-bar><div is=\"\"></div>", 
got:      "<foo-bar forbidden=\"true\" baz=\"foobar\"></foo-bar><div is=\"\"></div>"

this being the input '<foo-bar baz="foobar" forbidden="true"></foo-bar><div is="foo-baz"></div>'.

cure53 · 2021-11-25T11:16:10Z

It is possible that browsers to that in several situations.

franktopel · 2021-11-25T11:17:22Z

Can I rely on that order and adjust expected accordingly?

cure53 · 2021-11-25T11:18:07Z

I would say so, yes - but sometimes, we sadly need to add several expectations to cover that. Ugly but necessary.

franktopel · 2021-11-25T11:19:02Z

If DOMPurify output would not be deterministic in all cases, I guess we'd have bigger problems.

cure53 · 2021-11-25T11:21:08Z

Well, we are a sanitizer, not a markup manager. And sometimes, browsers mess with the order of things, attribute names, etc. etc. - nothing much we can do about that. Check this for example:

https://github.com/cure53/DOMPurify/blob/main/test/fixtures/expect.js#L6

Many different ways how an SVG can end up.

franktopel · 2021-11-25T11:22:19Z

How are browsers involved in this? I was assuming DOMPurify parses and processes Strings, and outputs Strings.

cure53 · 2021-11-25T11:23:33Z

Completely faulty assumption, we utilize the browser to create a document which we sanitize. Then we return the sanitized document as string, node, fragment or whatever people like :)

franktopel · 2021-11-25T11:24:19Z

Interesting, how is that document creation not unsafe in and of itself? Just curious.

cure53 · 2021-11-25T11:25:07Z

https://developer.mozilla.org/en-US/docs/Web/API/DOMParser
https://developer.mozilla.org/en-US/docs/Web/API/Document/implementation

Both allow creating documents that don't execute scripts.

franktopel · 2021-11-25T11:26:44Z

Very interesting, since afaik DOMParser.prototype.parse itself is an XSS sink that is being secured using TrustedTypes, just like innerHTML, outerHTML and insertAdjacentHTML setters.

cure53 · 2021-11-25T11:28:07Z

Nope :) This way is safe:

https://github.com/cure53/DOMPurify/blob/main/src/purify.js#L726

franktopel · 2021-11-25T11:30:05Z

Ah, it's just Domparser.parseFromString that is an XSS sink.

Which is what you're using!

cure53 · 2021-11-25T11:32:04Z

It's not an XSS sink :D Unless you use what comes out on a live document.

franktopel · 2021-11-25T11:32:41Z

Well Google says so in that article.

cure53 · 2021-11-25T11:34:02Z

We're going off-topic here. The way we use DOMParser is safe and needs no review at the moment.

franktopel · 2021-11-25T11:37:37Z

I've raised an issue on web.dev. GoogleChrome/web.dev#6890 Feel free to comment there.

franktopel · 2021-11-25T11:59:53Z

Back to our original topic, tests are all green now and expected results fit. Creating a PR now.

franktopel · 2021-11-25T13:37:29Z

PR is ready and good to go. One build job fails because yarn times out. Not sure what to do about that.

franktopel marked this pull request as draft November 24, 2021 18:49

* allow arbitrary attributes and their values for configured custom e…

9941992

…lements * added basic custom element tagname test so we don't open the floodgates with `ALLOWED_CUSTOM_ELEMENTS: /script/` adjusted tests added comments explaining what is going on new build fixed broken test

franktopel marked this pull request as ready for review November 24, 2021 19:41

franktopel marked this pull request as draft November 24, 2021 19:42

cure53 marked this pull request as ready for review November 24, 2021 21:05

Merge branch 'main' into main

d012202

franktopel commented Nov 25, 2021

View reviewed changes

Update test-suite.js

33503b5

cure53 merged commit b9dbc69 into cure53:main Nov 25, 2021

franktopel mentioned this pull request Nov 25, 2021

switched custom element handling to an object defining three differen… #603

Merged

1 task

allow arbitrary attributes and their values for configured custom elements #602

allow arbitrary attributes and their values for configured custom elements #602

Conversation

franktopel commented Nov 24, 2021

Dependencies

franktopel commented Nov 24, 2021

cure53 commented Nov 25, 2021

franktopel Nov 25, 2021

Choose a reason for hiding this comment

franktopel Nov 25, 2021

Choose a reason for hiding this comment

cure53 Nov 25, 2021

Choose a reason for hiding this comment

franktopel Nov 25, 2021

Choose a reason for hiding this comment

franktopel commented Nov 25, 2021

franktopel commented Nov 25, 2021

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021 • edited Loading

cure53 commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021

cure53 commented Nov 25, 2021

franktopel commented Nov 25, 2021 • edited Loading

franktopel commented Nov 25, 2021

franktopel commented Nov 25, 2021

franktopel commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading

cure53 commented Nov 25, 2021 •

edited

Loading

franktopel commented Nov 25, 2021 •

edited

Loading