Remove the $sce context for the src attribute on video, audio, and source #14019
Conversation
…ack. Previously, video, audio, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks are possible through these attributes as far as I can tell. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored.
Older msies reject my video element test, and format things better, no more tests for video[src] contexts under a img[src] sanitization describe.
But maybe it'll be in the future? I'd rather be safe than sorry in this case |
The context reduction didn't test retrocompatibility with trustAsResourceUrl(...) values.
Track files might contain CSS, and haven't been around for long. Keeping the high security context as a precaution is justified.
Source src is only for media files (videos, audio, images), and present no known script execution possibilities. We also don't expect new ones to pop up, as this tags is only supported on recent browsers.
Forgot to update the $sce doc, now done.
|
After looking more thoroughly at the spec, I found that track files have a specific grammar, which can contain some CSS. In that case I understand the precaution of not including it here, and so I removed it from the PR. However, if video and audio were changed to allow the multimedia content to execute script and/or access the DOM of the origin that loads them, that would probably put many existing pages at serious risk. It seems extremely unlikely that could ever happen and I don't know of any element/attribute where such a change ever happened. At the limit we could argue that every URL-context attribute should be RESOURCE_URL just in case it changes. Caja and Closure also don't require TrustedResourceUrls there. Caja, which sanitizes untrusted HTML, whitelists the src attribute for audio, video (not track, for the reasons above). See https://github.com/google/caja/blob/master/src/com/google/caja/lang/html/html5-attributes-whitelist.json. It might still get sanitized (so that javascript: URLs are not allowed) but video/audio/track src is allowed on untrusted HTML. Closure's SafeHtml also doesn't require TrustedResourceUrl to set src on audio, video and track, only SafeUrl (so there are checks for javascript: again): https://github.com/google/closure-library/blob/v20160125/closure/goog/html/safehtml.js#L530. Similar for Closure templates. Finally, verifying that a URL complies with the TrustedResourceUrl contract is not necessarily easy. It might mean checking that the domain where the URL points to is fully under the control of some organization, that it contains no open-redirects, etc. So it poses a real cost on applications that take the contract seriously. I'm worried that this speed bump teaches developers to bypass the $sce service, and that in the long run this will hurt the whole mechanism. Something similar already appears to have happened: not bundling $sanitize by default in Angular made trustAsHtml used all over the place. By the way, the source HTML5 element has a similar src attribute that links to media files. It's used in combination with audio / video tags (plus the experimental picture). I've added it here, as it's closely related, with a minor caveat: the spec says it "must not" have a src attribute when it's under a picture. Since picture is still experimental, and Angular allows arbitrary attributes that doesn't exist, it seems fine to lower the requirement for that. So, to sum up, as of this comment, this PR:
|
rjamet
changed the title
|
I am going to get Google Security to check this and approve the change before we can merge. |
|
@petebacondarwin @rjamet is from the google and working on the security team there, specializing on hardening Angular. Can you please review this PR and if it looks good get it merged? thank you! |
|
@rjamet I merged this via Github and unfortunately Github used me as the commit author after squashing. I'll revert this commit and re-instantiate you as the commit author. |
|
Cool, thanks a lot :) |
…ce, track Previously, video, audio, source, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks (script execution) are possible through these attributes as far as we can tell. Angular2 also uses the same categorization. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. This has also been given a LGTM by @mprobst via email. Commit 4853201 on the master branch contains the same changes, but is missing this commit description. This commit does not backport the BC introduced in 04cad41. PR (#15039) Closes #14019
…ce, track Previously, video, audio, source, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks (script execution) are possible through these attributes as far as we can tell. Angular2 also uses the same categorization. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. This has also been given a LGTM by @mprobst via email. Commit 4853201 on the master branch contains the same changes, but is missing this commit description. This commit does not backport the BC introduced in 04cad41. PR (angular#15039) Closes angular#14019
…ce, track Previously, video, audio, source, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks (script execution) are possible through these attributes as far as we can tell. Angular2 also uses the same categorization. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. This has also been given a LGTM by @mprobst via email. Commit 4853201 on the master branch contains the same changes, but is missing this commit description. This commit does not backport the BC introduced in 04cad41. PR (angular#15039) Closes angular#14019
Video, audio, and track sources require $sce.RESOURCE_URL, so by default you need a $sce.trustAsResourceUrl to set them dynamically if the resources are not on the same subdomain. This is not justified: no script execution is possible through the src attribute as far as the state of the art goes, so there's no reason to restrict it.
The change shouldn't be breaking: uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently unwrapped.
(BTW, not sure about the amount of tests to add there. We have![]()
already, I'm adding one of the new set, but adding the two others just doesn't seem productive)