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

Use of [Unforgeable] in Trusted Types WebIDL #257

Closed
littledan opened this issue Feb 12, 2020 · 13 comments
Closed

Use of [Unforgeable] in Trusted Types WebIDL #257

littledan opened this issue Feb 12, 2020 · 13 comments
Labels

Comments

@littledan
Copy link

There's discussion of deprecating WebIDL's [Unforgeable] attribute at whatwg/webidl#350 (comment) . It seems like Trusted Types uses [Unforgeable].

I can see some pros and cons here: [Unforgeable] may make this mechanism more reliable in some way, but on the other hand, it makes polyfilling and other forms of manipulating/emulating the environment more difficult.

Is this usage necessary? I'm not sure if the rationale for the details of [Unforgeable] use is is documented somewhere already; I've had trouble finding it.

@koto
Copy link
Member

koto commented Feb 12, 2020

The rationale is to prohibit the user code from being able to modify the policy creation mechanisms and checking if the type instances were created through the policies (i.e. trustedTypes.isHTML will only ever return true if the object came through the natively created policy). If we drop unforgeable, reviewing what policies are actually being created, and what rules application uses for protecting against DOM XSS becomes harder.

I realize what the downsides are and I'm not opposed to removing [Unforgeable] from TT if this was the last feature using it. What's the story with Location and other window properties though? I don't want to derail the conversation, just trying to understand how determined are we to remove [Unforgeable] from the web APIs.

@annevk
Copy link
Member

annevk commented Feb 12, 2020

There's no active effort to reduce existing usage, except perhaps for https://github.com/Agoric/proposal-preserve-virtualizability which @littledan pointed out earlier today.

Having said that, motivation for existing usage is primarily plugins and those are going away. Location will have a somewhat magical setup forever most likely and I also suspect that we cannot stop things from being own properties if they are today.

@littledan
Copy link
Author

littledan commented Feb 12, 2020

@koto I'm not really strongly opposed to using [Unforgeable], but personally, I'm having a hard time understanding exactly what guarantees are provided. If the code setting up the Trusted Type policy has the ability to run first, then it could store these getters to have a reliable way to get the information. But, if it doesn't run first, then some other code could replace the trusted type classes entirely. So, it's hard for me to understand what the "threat model" is. Are we going for, preserving good ergonomics for using trusted types, assuming that the TT-related code does run first? (Edit: This isn't possible, see reply from @koto)

Note, in the related proposal for Array.isTemplateObject, an ordinary writable property is used. That's another case where you are likely to want integrity, but we in TC39 reasoned that you can have first-run code save off the original value if you want reliability. cc @mikesamuel

@koto
Copy link
Member

koto commented Feb 12, 2020

@koto I'm not really strongly opposed to using [Unforgeable], but personally, I'm having a hard time understanding exactly what guarantees are provided. If the code setting up the Trusted Type policy has the ability to run first, then it could store these getters to have a reliable way to get the information. But, if it doesn't run first, then some other code could replace the trusted type classes entirely.

How? window.trustedTypes and some of its properties are unforgeable, so window.trustedTypes.isHTML will always be the native function. That's exaclty the property we were after, and it does not rely on code running first.

So, it's hard for me to understand what the "threat model" is. Are we going for, preserving good ergonomics for using trusted types, assuming that the TT-related code does run first?

Note, in the related proposal for Array.isTemplateObject, an ordinary writable property is used. That's another case where you are likely to want integrity, but we in TC39 reasoned that you can have first-run code save off the original value if you want reliability. cc @mikesamuel

@littledan
Copy link
Author

Ah, sorry, I missed this one [Unforgeable]. Thanks for explaining.

@domenic
Copy link

domenic commented Feb 12, 2020

Note that window itself is not unforgable, so if you use window.trustedTypes.isHTML you can be intercepted. You have to use trustedTypes.isHTML.

@annevk
Copy link
Member

annevk commented Feb 12, 2020

self and frames are not, but window is [Unforgeable] readonly attribute WindowProxy window; at the moment. (Edit: this does not seem great if we ever get trusted types in workers and you want to write portable code of sorts.)

@koto
Copy link
Member

koto commented Feb 12, 2020

To reiterate, we're not opposed to removing [Unforgeable] if that attribute is going away or becomes legacy, but it does still offer some properties that are useful (not crucial) for us.

@domenic
Copy link

domenic commented Feb 12, 2020

I think it's definitely going to be marked legacy, but maybe trusted types is special enough that it can make an argument for being part of that legacy?

@koto
Copy link
Member

koto commented Feb 12, 2020

My preference would be to keep it [Unforgeable] for now, as we can always drop it later, but the reverse has webcompat issues.

@domenic
Copy link

domenic commented Feb 12, 2020

Unfortunately both directions have web-compat issues. E.g. removing unforgable will make trustedTypes.hasOwnProperty('isHTML') return false.

@annevk
Copy link
Member

annevk commented Feb 12, 2020

A way around that might be defining them all directly on the global as those basically only have own properties anyway (with some notable exceptions), e.g., window.trustedTypesIsHTML.

@koto koto added the spec label Mar 2, 2020
@koto
Copy link
Member

koto commented Mar 3, 2020

Revisiting this, removing [Unforgable] is better for polyfilling - and authors might need to partially polyfill the API when/if it gets new features with partial support in certain agents.

The case where I encountered this was polyfilling #258. In the current shape, there's no way to polyfill the policy construction by overloading the constructor for TrustedTypePolicy, and issues like this will likely happen again. I now think the polyfillability benefits outweigh the secuirty benefits here.

koto added a commit to koto/trusted-types that referenced this issue Mar 4, 2020
@koto koto closed this as completed in 8748474 Mar 4, 2020
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 6, 2020
Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2020
Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2020
Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyf@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748022}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2020
Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyf@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748022}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Mar 7, 2020
Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyf@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748022}
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Mar 11, 2020
…tonly

Automatic update from web-platform-tests
Make Trusted Types ununforgeable.

Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyf@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748022}

--

wpt-commits: 7c966db714c251c2b7bc243b9657e5dff72aa8dc
wpt-pr: 22118
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 12, 2020
…tonly

Automatic update from web-platform-tests
Make Trusted Types ununforgeable.

Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyf@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748022}

--

wpt-commits: 7c966db714c251c2b7bc243b9657e5dff72aa8dc
wpt-pr: 22118
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Mar 12, 2020
…tonly

Automatic update from web-platform-tests
Make Trusted Types ununforgeable.

Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyfchromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#748022}

--

wpt-commits: 7c966db714c251c2b7bc243b9657e5dff72aa8dc
wpt-pr: 22118

UltraBlame original commit: 643c88e00d7f34490218c623cad4d1ecec9fe30d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Mar 12, 2020
…tonly

Automatic update from web-platform-tests
Make Trusted Types ununforgeable.

Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyfchromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#748022}

--

wpt-commits: 7c966db714c251c2b7bc243b9657e5dff72aa8dc
wpt-pr: 22118

UltraBlame original commit: 643c88e00d7f34490218c623cad4d1ecec9fe30d
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Mar 12, 2020
…tonly

Automatic update from web-platform-tests
Make Trusted Types ununforgeable.

Context: w3c/trusted-types#257

Bug: 1058400
Change-Id: Ie91e8ad7e8eff814e3aaa1d940ca0bc1d6c2f09e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089705
Reviewed-by: Yifan Luo <lyfchromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#748022}

--

wpt-commits: 7c966db714c251c2b7bc243b9657e5dff72aa8dc
wpt-pr: 22118

UltraBlame original commit: 643c88e00d7f34490218c623cad4d1ecec9fe30d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants