Skip to content
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

Are all injection sinks covered by the spec? #385

Open
mbrodesser-Igalia opened this issue Jan 8, 2024 · 20 comments
Open

Are all injection sinks covered by the spec? #385

mbrodesser-Igalia opened this issue Jan 8, 2024 · 20 comments
Milestone

Comments

@mbrodesser-Igalia
Copy link
Collaborator

https://w3c.github.io/trusted-types/dist/spec/#introduction mentions "over 60 different injection sinks".

However, the spec contains:

12 occurrences of "HtmlString"
6 occurrences of "ScriptString"
12 occurrences of "ScriptUrlString"

Which is below 60.

https://w3c.github.io/trusted-types/dist/spec/#injection-sinks mentions The exact list of injection sinks covered by this document is defined in [§ 4 Integrations](https://w3c.github.io/trusted-types/dist/spec/#integrations)..

I web-searched for a full list of injection sinks but found none. If there's such a list, please share.

@mbrodesser-Igalia
Copy link
Collaborator Author

Some injection sinks are covered implicitly by one change of the spec. E.g. eval() and Function() are covered by https://w3c.github.io/trusted-types/dist/spec/#csp-eval.

@koto
Copy link
Member

koto commented Jan 15, 2024

All injection sinks are covered, some are covered not on the IDL+[[StringContext]] layer, e.g. https://w3c.github.io/trusted-types/dist/spec/#enforcement-in-event-handler-content-attributes, https://w3c.github.io/trusted-types/dist/spec/#require-trusted-types-for-pre-navigation-check, or the eval-related sinks.

@mbrodesser-Igalia
Copy link
Collaborator Author

All injection sinks are covered,

Is there a trustworthy (no pun intended) list of those? Otherwise, it's easy to miss some.

some are covered not on the IDL+[[StringContext]] layer, e.g. https://w3c.github.io/trusted-types/dist/spec/#enforcement-in-event-handler-content-attributes, https://w3c.github.io/trusted-types/dist/spec/#require-trusted-types-for-pre-navigation-check, or the eval-related sinks.

@koto
Copy link
Member

koto commented Jan 15, 2024

I don't think there's a single authoritative, exhaustive and up-to-date list of those, but the implementation following the spec should cover all of them, to the best of our knowledge.

The list could be compiled from various XSS related projects, e.g. https://github.com/w3c/trusted-types/blob/8041828e2426a00221850934ae0f9421f77e3e07/src/enforcement.js has the navigation sinks in DOM, https://github.com/google/safevalues/tree/main/src/dom has a similar list of DOM sinks, https://github.com/cure53/DOMPurify/blob/b3b441e6589f49c506b4840aea3421aa83c62f52/test/fixtures/expect.mjs has XSS vectors etc.

The closest thing in a spec is the reverse - a list of DOM elements and attributes that don't cause XSS; see subsections of https://wicg.github.io/sanitizer-api/#constants.

@mbrodesser-Igalia
Copy link
Collaborator Author

I don't think there's a single authoritative, exhaustive and up-to-date

Related to up-to-dateness: #399.

list of those, but the implementation following the spec should cover all of them, to the best of our knowledge.

The list could be compiled from various XSS related projects, e.g. https://github.com/w3c/trusted-types/blob/8041828e2426a00221850934ae0f9421f77e3e07/src/enforcement.js has the navigation sinks in DOM, https://github.com/google/safevalues/tree/main/src/dom has a similar list of DOM sinks, https://github.com/cure53/DOMPurify/blob/b3b441e6589f49c506b4840aea3421aa83c62f52/test/fixtures/expect.mjs has XSS vectors etc.

The closest thing in a spec is the reverse - a list of DOM elements and attributes that don't cause XSS; see subsections of https://wicg.github.io/sanitizer-api/#constants.

@mozfreddyb
Copy link
Collaborator

This goes into implementation-specific details, but some engines (e.g., blink) has a list of all known elements and event handler attributes, which makes it possible to properly collect and annotate in a central location.

I don't think the same is true for Gecko, but it's been a while since I last looked.

@lukewarlow
Copy link
Member

See #403 for one missing sink (it's quite new)

@mbrodesser-Igalia
Copy link
Collaborator Author

@mozfreddyb: the note at https://wicg.github.io/sanitizer-api/#builtins-justification mentions the values of the constants (the allow lists) will need to be updated when new HTML elements are added to user agents.
The situation for trusted types is similar: when new elements or attributes are added, they potentially need trusted types.

So when new elements or attributes are added to the spec, it seems desirable to check whether the Sanitizer API's constants require adaptation and whether those elements or attributes require trusted types.
TBH, ideally both should become mandatory. E.g. a PR for a new element or attribute could contain checkboxes for both instead of only relying on reviewers thinking about it.
Are there other similar requirements for new elements or attributes one may learn from?

CC @annevk

@annevk
Copy link
Member

annevk commented Jan 16, 2024

There's a comment at the top of whatwg/html's source with a checklist. That will have to be updated. But also, per-element information should be maintained in the HTML Standard directly so it's unlikely to get looked over.

@mbrodesser-Igalia
Copy link
Collaborator Author

There's a comment at the top of whatwg/html's source with a checklist.

That's at the top of https://raw.githubusercontent.com/whatwg/html/main/source. TIL.

That will have to be updated. But also, per-element information should be maintained in the HTML Standard directly so it's unlikely to get looked over.

Okay, trusting your experience here. Filed WICG/sanitizer-api#206 for updating it for the Sanitizer API.

@mbrodesser-Igalia
Copy link
Collaborator Author

mbrodesser-Igalia commented Jan 16, 2024

See #403 for one missing sink (it's quite new)

@lukewarlow: how did you find it?

This ticket should stay open until one can be sure that all sinks are covered.

@lukewarlow
Copy link
Member

I found it by chance I happened to be losely following the sanitizer API work and remembered the unsafe variants were merged into the HTML spec recently for DSD parsing support.

@lukewarlow
Copy link
Member

#358 - It's possible there's additional sinks that aren't covered too, here's an issue that seems to suggest another one (haven't tested to confirm)

@lukewarlow
Copy link
Member

lukewarlow commented Jan 16, 2024

https://github.com/shhnjk/cursed_types - I've also come across this repository that claims to list some DOM XSS bypasses for trusted types. They all seem to be solvable using CSP directives and I'm not sure if any are valid bypasses for what this spec is concerned with but worth bringing to attention.

This page references #357 as something that might need to be fixed still?

@lukewarlow
Copy link
Member

#232 - also mentions potential issues with dynamic imports that mentions this stage 2 Ecamscript proposal https://github.com/tc39/proposal-dynamic-import-host-adjustment Are there any updates on that?

@lukewarlow
Copy link
Member

#359 - here's another issue that suggests there's a sink not covered.

@mbrodesser-Igalia
Copy link
Collaborator Author

There's a comment at the top of whatwg/html's source with a checklist. That will have to be updated. But also, per-element information should be maintained in the HTML Standard directly so it's unlikely to get looked over.

@mozfreddyb during our internal meeting last Thursday you mentioned an alternative, more robust proposal. Unfortunately I don't remember the details. In case you do, can you please write down that idea here?

@mozfreddyb
Copy link
Collaborator

mozfreddyb commented Jan 23, 2024

The HTML spec has a list of properties that go with every element. For example the section introducing the style element, has metadata about the element (categories, content model, etc.).

It would be nice if there was an script execution property that goes with every element. This would allow a robust definition of what HTML built-in syntax can cause XSS and what doesn't. I think that is useful for a lot of other specs and probably everyone else building XSS protections for the web.
(Editorial: It would be nice if the spec kept all of those listed for a reference table in the appendix of the spec. This bit would require work on the HTML spec parser, I suppose)

@mbrodesser-Igalia
Copy link
Collaborator Author

The HTML spec has a list of properties that go with every element. For example the section introducing the style element, has metadata about the element (categories, content model, etc.).

It would be nice if there was an script execution property that goes with every document.

Not every document, but every element?

Be aware that functions specified in ES wouldn't be covered. E.g. new Function() (https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-function-constructor) eval (https://tc39.es/ecma262/multipage/global-object.html#sec-eval-x).

This would allow a robust definition of what HTML built-in syntax can cause XSS and what doesn't. I think that is useful for a lot of other specs

Mainly the Sanitizer API (https://wicg.github.io/sanitizer-api/), presumably.

and probably everyone else building XSS protections for the web.

Agreed.

(Editorial: It would be nice if the spec kept all of those listed for a reference table in the appendix of the spec. This bit would require work on the HTML spec parser, I suppose)

Does that proposal only affect the HTML spec, or other specs like the DOM Standard too?

@mozfreddyb
Copy link
Collaborator

Not every document, but every element?

Typo (and modified above): Yes a script execution property.

Be aware that functions specified in ES wouldn't be covered. E.g. new Function() (https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-function-constructor) eval (https://tc39.es/ecma262/multipage/global-object.html#sec-eval-x).

Yes, that's a different language though. The HTML bits should be in the HTML spec. ECMAScript does have a hook for script execution as part of CSP's control for eval, Function constructor etc - doesn't it?

(Editorial: It would be nice if the spec kept all of those listed for a reference table in the appendix of the spec. This bit would require work on the HTML spec parser, I suppose)

Does that proposal only affect the HTML spec, or other specs like the DOM Standard too?

I'd hope the DOM standard does not create or define additional HTML elements on the fly. But WICG drafts might. Admittedly, this is a bit of a gap that we'll need to close. I'm sure people will try and ship functionality before fully upstreaming to HTML. I suppose that will remain a challenge...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants