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

mXSS: should createHTML get information about the context? #569

Open
mozfreddyb opened this issue Dec 20, 2024 · 6 comments
Open

mXSS: should createHTML get information about the context? #569

mozfreddyb opened this issue Dec 20, 2024 · 6 comments

Comments

@mozfreddyb
Copy link
Collaborator

From my understanding, a lot of mXSS is caused by additional parsing:

With Trusted Types, additional parsing is basically impossible to avoid, because

  • user calls innerHTML
  • browser calls createHTML
  • createHTML uses a sanitizer (parses, sanitizes and serializes to a string)
  • return value of createHTML is being parsed again by innerHTML

I think we could make mXSS easier to avoid if the createHTML function gets optional arguments about the parsing context (an element? or an element name?).

CCing some folks since I am not sure if they watch the repo 🙂 @annevk, @koto

@Sora2455
Copy link

Wasn't this effectively solved by this issue in the Sanitiser spec?

@lukewarlow
Copy link
Member

This isn't directly about the sanitizer API so no that issue is separate. This is about trusted types as used today with the likes of DOMPurify.

One question I have is does this context actually help. Like does DOMPurify have to guess currently where with this new param it could behave better with an existing option?

Also worth being aware this only applies in the case of the default policy.

@mozfreddyb
Copy link
Collaborator Author

This is about a lower-case sanitizer, not the Sanitizer spec.
Fundamentally, if you want to apply a sanitizer to the input that is passed to createHTML(), it would be nice to know which context element this will be parsed into.

I think this is required because parsing HTML fragments is not a function that accepts a string. Parsing behavior is very different depending on the context.

All of this can be sidelined if the allow-list in the sanitizer is very strict, of course.

@mozfreddyb
Copy link
Collaborator Author

One question I have is does this context actually help. Like does DOMPurify have to guess currently where with this new param it could behave better with an existing option?

Well, DOMPurify hacks around it here and there.
As an example, the maximum tree depth for an HTML document tree is 512. If you start nesting elements deeper, they will be added as siblings instead of children. Without contextual parsing, a sanitizer can't know the current depth and will see elements as children (e.g., within a template element, making them seem harmless) when they will in fact become siblings.

Also worth being aware this only applies in the case of the default policy.

Not sure I understand. Can you give an example?

@lukewarlow
Copy link
Member

The general usage of trusted types policies is that the author calls createHTML and then passes the resultant TrustedHTML to the sink. The browser only gets involved in that process when there's a default policy in use and you assign a string to a sink.

@annevk
Copy link
Member

annevk commented Jan 16, 2025

So what would the design look like here? createHTML() takes a context element which is then enforced when the resulting TrustedHTML object is passed somewhere?

I think that could be interesting as defense-in-depth, but I'm not sure it's worth the complexity. Probably worth keeping this open though to see if this is something that people run into in the wild. What's not worth it today might be tomorrow.

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

4 participants