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

Consider removing getPolicyNames() #235

Closed
koto opened this issue Nov 8, 2019 · 5 comments · Fixed by #264
Closed

Consider removing getPolicyNames() #235

koto opened this issue Nov 8, 2019 · 5 comments · Fixed by #264
Labels

Comments

@koto
Copy link
Member

koto commented Nov 8, 2019

Pulling up a side-note as a separate comment:

With 'allow-duplicates' in place, I'd be a bit worried that

trustedTypes.createPolicy(trustedTypes.getPolicyNames()[0], {...}}.

becomes a common idiom to create policy without getting a security review.

It might be worth getting rid of getPolicyNames; it doesn't seem all that valuable. For detailed monitoring/telemetry a callback or event that can also see stack traces would be more appropriate.

Originally posted by @xtofian in #222 (comment)

@koto
Copy link
Member Author

koto commented Nov 11, 2019

That would only work if the code runs within the environment where 'allow-duplicate'is used - so it's probably less probable in a library code.

I'm worried that removing this only gives the impression of security. Policy name is not designed to be a secret. Protecting secrets from Javascript code that runs in your realm is at least tricky, so we're trying not to go that way. For example, even if we removed getPolicyNames one can fetch() JS code (or read the inline script texts) and String.search for the policy name. Or, more practically, a name google-analytics or (window.jQuery ? 'jquery' : 'myapp') will be used. There are other legitimate places that would leak the name - e.g. the securitypolicyviolation event details.

In general, I'd like to stress out not to use policy names as the sole security controls - these are hints that let the app owner simplify the trust decisions, but they are not sufficient measures to stop authors determined to bypass the restrictions. With additional tooling names can be used in a mode that provides more security (e.g. when controlling what gets compiled into the application), but even that this will fall apart when dealing with malicious authors.

getPolicyNames() is probably less useful if we have an event for monitoring policy creation, but nevertheless it provides introspection into policy names

  • that were used to successfully create a policy (ignoring the blocked policies)
  • ... since the realm was created (and not since the event listener was installed)

so there is some value to it when compared to e.g. events.

@koto
Copy link
Member Author

koto commented Nov 11, 2019

/ cc @xtofian

@xtofian
Copy link

xtofian commented Nov 11, 2019 via email

@koto
Copy link
Member Author

koto commented Nov 13, 2019

Essentially, the purpose of these controls is not to strictly prevent a malicious developer from doing something nefarious, but rather to reasonably convincingly communicate to a non-malicious developer that they're doing something they shouldn't.

That's opinionated, but for me it is already pretty clear that if there's an allowlist for getting access to some resource, then pretending you're on the allowlist is an intentional bypass - and if that's the intention, then all bets are off. An abuse of the API is indeed easier with getPolicyNames, but I think it's clear this is an abuse, and should be treated like one - in documentation, and other security controls like code review.

The policy name getter can always be constructed, even wrapped in a single function. e.g. (() => {addEventListener("securitypolicyviolation", e => {__p = e.originalPolicy.match(/trusted-types (\w+)/)[1]}, {once:1});try{location.href="javascript:__p = 'anything'"}catch(e){};return __p })(), it can even be a npm module so that the complexity is abstracted away.

In general, I'm worried it might be limiting for the future extension of the API if we decide to make it hard, or impossible to get the policy names from within the program. E.g. it might (in future) be helpful if a trusted type, or DOM attribute getter gave you information about which policy was used to create it.

That said, if we think getPolicyNames() delivers little value to the devs, I'm happy to remove it.

@koto koto added the spec label Mar 2, 2020
koto added a commit to koto/trusted-types that referenced this issue Mar 5, 2020
A proper introspection at policy creation time (possibly via events)
will be created at a later point.

Fixes w3c#235.
@koto koto closed this as completed in #264 Mar 5, 2020
koto added a commit that referenced this issue Mar 5, 2020
A proper introspection at policy creation time (possibly via events)
will be created at a later point.

Fixes #235.
@koto
Copy link
Member Author

koto commented Mar 5, 2020

We removed getPolicyNames, in favor of creating a proper introspection at policy creation time (possibly via events). Thanks!

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 5, 2020
Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 5, 2020
Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Auto-Submit: Krzysztof Kotowicz <koto@google.com>
Cr-Commit-Position: refs/heads/master@{#747250}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 5, 2020
Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Auto-Submit: Krzysztof Kotowicz <koto@google.com>
Cr-Commit-Position: refs/heads/master@{#747250}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Mar 5, 2020
Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Auto-Submit: Krzysztof Kotowicz <koto@google.com>
Cr-Commit-Position: refs/heads/master@{#747250}
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Mar 7, 2020
…PolicyNames()., a=testonly

Automatic update from web-platform-tests
[Trusted Types] Removed trustedTypes.getPolicyNames().

Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Auto-Submit: Krzysztof Kotowicz <koto@google.com>
Cr-Commit-Position: refs/heads/master@{#747250}

--

wpt-commits: d8dea9c9ca97bab3138d89db56e707046533680f
wpt-pr: 22101
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 7, 2020
…PolicyNames()., a=testonly

Automatic update from web-platform-tests
[Trusted Types] Removed trustedTypes.getPolicyNames().

Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Auto-Submit: Krzysztof Kotowicz <koto@google.com>
Cr-Commit-Position: refs/heads/master@{#747250}

--

wpt-commits: d8dea9c9ca97bab3138d89db56e707046533680f
wpt-pr: 22101
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Mar 9, 2020
…PolicyNames()., a=testonly

Automatic update from web-platform-tests
[Trusted Types] Removed trustedTypes.getPolicyNames().

Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheimchromium.org>
Commit-Queue: Daniel Vogelheim <vogelheimchromium.org>
Auto-Submit: Krzysztof Kotowicz <kotogoogle.com>
Cr-Commit-Position: refs/heads/master{#747250}

--

wpt-commits: d8dea9c9ca97bab3138d89db56e707046533680f
wpt-pr: 22101

UltraBlame original commit: 0f42be9545337471a2ae6e07f598c0ac7f9e8176
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Mar 9, 2020
…PolicyNames()., a=testonly

Automatic update from web-platform-tests
[Trusted Types] Removed trustedTypes.getPolicyNames().

Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheimchromium.org>
Commit-Queue: Daniel Vogelheim <vogelheimchromium.org>
Auto-Submit: Krzysztof Kotowicz <kotogoogle.com>
Cr-Commit-Position: refs/heads/master{#747250}

--

wpt-commits: d8dea9c9ca97bab3138d89db56e707046533680f
wpt-pr: 22101

UltraBlame original commit: 0f42be9545337471a2ae6e07f598c0ac7f9e8176
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Mar 10, 2020
…PolicyNames()., a=testonly

Automatic update from web-platform-tests
[Trusted Types] Removed trustedTypes.getPolicyNames().

Context: w3c/trusted-types#235

Bug: 739170
Change-Id: I8499534f4351ea2c9f689071f3d6510238382d72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089651
Reviewed-by: Daniel Vogelheim <vogelheimchromium.org>
Commit-Queue: Daniel Vogelheim <vogelheimchromium.org>
Auto-Submit: Krzysztof Kotowicz <kotogoogle.com>
Cr-Commit-Position: refs/heads/master{#747250}

--

wpt-commits: d8dea9c9ca97bab3138d89db56e707046533680f
wpt-pr: 22101

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

Successfully merging a pull request may close this issue.

2 participants