feat($sce): handle URLs through the $sce service, plus allow concatenation in that $sce contexts. #14250

rjamet · 2016-03-16T14:04:10Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This is an improvement to uniformize the secure context code, by making the $sce handle URL-context in roughly the same way it handles HTML-context. That also required a more silent change, to have $sce-contexts where concatenation of values is allowed (described below).

What is the current behavior? (You can also link to an open issue here)

Currently, the whole $sce.URL context is unused. Dynamically-set URLs that are sanitized through various codepaths (mostly in compile.js, $set function).

Usually, if one wants to force a value through Angular's security mechanisms, the $sce.trustAs function should be used. There's no straightforward way to do it in a single place for URLs, as <a href="{{sce.trustAsUrl(foo)}}"> sanitizes silently its link.

Also, an attribute being a $sce context means that concatenations are blocked: <iframe src="generate.php?{{params}}"> would throw for instance. However, since it's not a $sce context, <img src="generate.php?{{params}}"/> is fine.

What is the new behavior (if this is a feature change)?

The new behavior makes Angular handle URLs with the $sce service, similarly to HTML context. $sce.trustAsUrl is now useful, as it allows bypassing the sanitization. If one uses plain strings in URL-context attributes, they will be sanitized through the existing $$sanitizeUri service.

I also have tweaked $interpolate to work as expected with concatenated values in select secure contexts. In the listed contexts (URL only for now, though that could be expanded to anything), trustAs'd values with no concatenations are not sanitized (eg "{{trustedVar}}" works as you'd expect), and anything else is downgraded to string, concatenated if needed, and then passed to $sce.getTrusted that handles the sanitization as usual. For instance, "java{{trustAsUrl('script:evil();')}}" is sanitized as it should.

Does this PR introduce a breaking change?

Yes: I've merged both URL sanitization methods in $$sanitizeUri (since there's a single $sce.URL context). This will break the applications that edit one of these whitelists, but I believe there's security benefits in doing so, as they will be able to use trustAsUrl in specific parts of their application instead of having to blanket-approve the special scheme they rely on. Otherwise, I believe the changes are backwards-compatible unless you're really hooking into Angular internals.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

I'm from Google security, but I'm not right all the time, so please ask about anything: I might have missed something, or this might not be something you want. I had to modifiy components I'm not that familiar with. Things to check:

If you had special reasons to split the URL sanitization in two parts, I haven't been able to find it. I just merged the whitelists.
I'm not entirely sure of the $interpolate change. I've had several issues with infinite digests with arrays while developping, the latest changes seem to pass, but there might still be dragons hidden in there.
I'm not sure either I got all the doc modified.
Finally, $sce.HTML / $sce.RESOURCE_URL could also be concatenated. At least for HTML it works, and you could have something like the commented-out test in sceSpecs.js, but I'm not sure it brings much value here, so I've left it disabled for now. RESOURCE_URL could follow the same path with either completely-trusted URLs, or concatenated-then-treated-as-string URLs that would be checked against the whitelist/blacklist mechanism.

rjamet · 2016-03-16T16:22:23Z

A few things are wrong in there, sorry about that :

I broke IE tests with "access denied" errors.
I'm too eager in assigning the URL context, that should probably strictly follow the current behavior instead. Not sure about xlink:href either.

I'll take a deeper look sometime this week or the following one.

googlebot · 2016-03-22T10:38:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-03-22T10:38:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-03-22T10:44:16Z

CLAs look good, thanks!

googlebot · 2016-03-22T10:44:16Z

CLAs look good, thanks!

rjamet · 2016-05-26T16:10:56Z

test/ng/directive/booleanAttrsSpec.js

-          // on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
-          // then calling element.setAttribute('src', 'foo') doesn't do anything, so we need
-          // to set the property as well to achieve the desired effect
+  it('should update the element property as well as the attribute', inject(


This is an observable change : I'm really not sure why these weren't canonicalized on IE before, but now they are, and it just seems like the right behavior.

rjamet · 2016-05-26T16:18:01Z

I finally got this to work! There's another very minor change: IE props for URLs are now canonicalized, and I assume sanitized. I'm not entirely sure of the origin of the previous behavior, nor if that matters much?

rjamet · 2016-05-27T09:15:31Z

(And the CI looks good, except mobile Safari that crashed :/)

gkalpak · 2016-08-18T13:46:57Z

src/ng/compile.js

-        if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
-            (nodeName === 'img' && key === 'src')) {
-          // sanitize a[href] and img[src] values
-          this[key] = value = $$sanitizeUri(value, key === 'src');


Afaict, this change means that people using attrs.$set(key, value) will not benefit from sanitization (for <a href/xlink:href or <img src/ng-src) any more. Is that correct?

Yes. I assumed users shouldn't use these and instead rely on attributes at the template level.

Bumping this: there's actually a lot of uses of attrs.$set that would be relevant, and probably a bunch of vulnerabilities created by this PR. But then, as far as I can tell, URLs are the only things sanitized at this level, all the rest isn't :/

This is a large patch to handle URLs with the $sce service, similarly to HTML context. However, to keep rough compatibility with existing apps, we need to allow URL-context concatenation, since previously $sce contexts prevented sanitization. There's now a set of special contexts (defined in $interpolate) that allow concatenation, in a roughly intuitive way: trusted types alone are trusted, and any concatenation of values results in a non-trusted type that will be handled by getTrusted once the concatenation is done. Thus, "{{safeType}}" will not be sanitized, "{{ 'javascript:foo' }}" will be, and "javascript:{{safeType}}" will also be. There's backwards incompatible changes in there, mostly the fusion of the two URL context sanitization whitelists (the separation is nice, but not important for security).

…sts. Only apply the $sce.URL context where there was previously sanitization, so that the final result matches more closely the old behaviors. Also, fix a few minor mistakes in tests (mismatching tags, wrong expected errors, differences with the surrounding testing style), and mock out $$sanitizeUri where it makes sense.

I'm not really sure why they weren't in the first place, but now they really are, and that doesn't seem like a negative change.

Fixes an error message in interpolate, the affected tests, and add URL context to $compile's getTrustedContext.

rjamet · 2016-08-30T16:27:35Z

(running into Node trouble, I'll update when I manage to check tests, sorry about that)

Merge branch 'master' of https://github.com/angular/angular.js into plumbUrlContextThroughSce

rjamet · 2016-08-31T12:09:00Z

Ok, this is ready for review, I sorted my issues and tests are green :)

koto · 2016-11-30T17:28:39Z

Are there any remaining concerns for merging this? It seems useful.

petebacondarwin · 2016-11-30T18:45:59Z

If this has a breaking change then we won't get it into 1.6.0 now. We can merge it into master once 1.6.0 has been released.

petebacondarwin

I have downloaded and rebased this locally - not a trivial task since this PR is getting quite old.
It looks good to me but since this is all hitting security sensitive code, I would like two more LGTM from core AngularJS members.

petebacondarwin · 2017-01-30T13:57:12Z

src/ngSanitize/sanitize.js

@@ -158,7 +158,7 @@ function $SanitizeProvider() {
    return function(html) {
      var buf = [];
      htmlParser(html, htmlSanitizeWriter(buf, function(uri, isImage) {


We don't need the isImage parameter here any more?

I don't think so, it was only used to decide which sanitization method to use.

thanks. Waiting on a last review from @IgorMinar and @gkalpak before merging. Did you @rjamet test this on Google3 already?

I'll take a look today, but it's at least breaking for custom whitelists since the two functions got merged: it's trivial to fix though. I might find other issues though (I'll update tonight). Should that wait for a minor version bump?

Should that wait for a minor version bump?

Yes, as a breaking change this will be merged only into master (which will eventually become 1.7.0; probably some time around May).

In that case I can handle and document the google3-specific issues, since that's likely to be plumbed differently internally. There's still going to be a few issues for everyone:

if we keep this common whitelist model, all URL bindings of blob urls that were previously fine will break without $sce.trustAsUrl, and I don't think we can figure these out beforehand.

whitelist customizations will break (renamed)

things plugging directly into Angular's $set for URLs will lose sanitization (in line with other contexts).

gkalpak · 2017-01-31T12:14:46Z

If you had special reasons to split the URL sanitization in two parts, I haven't been able to find it. I just merged the whitelists.

A little background:

First only a[href] was sanitized: 9532234
Then img[src] was sanitized based on the same whitelist: 1adf29a
Then a[href] and img[src] were sanitized based on different whitelists: 3e39ac7

The main reason for the separate whitelists afaict is the ability to trust different schemes / URLs. For example, by default, a[href] allows mailto and tel schemes, while img[src] allows blob and data:image. It should also allow more granular control/auditing from developers: trusting on source for images only or trusting another source for links only etc.

It seems useful to be able to sanitize a[href] and img[src] from different whitelists (but then again I am no security expert 😃)

@rjamet, what are your thoughts/reasoning on this?

rjamet · 2017-01-31T13:55:46Z

src/ng/sanitizeUri.js

-    }
-    return aHrefSanitizationWhitelist;
-  };
+  var uriSanitizationWhitelist = /^\s*((https?|ftp|file|blob|tel|mailto):|data:image\/)/i;


I'm wrong there, blob shouldn't be safe for navigation. A link to a blob with user-controlled contents and a dangerous MIME type will run in the origin that created it.

rjamet · 2017-01-31T13:58:52Z

The initial thought was to align with Closure and simplify, but it's probably not a great reason to break things... How about aligning with Angular?(https://github.com/angular/angular/blob/master/modules/%40angular/platform-browser/src/security/url_sanitizer.ts)

I think that the distinction is not necessary, the safe subset is fairly much the same. There's some nonsensical choices (img src=tel://), but they're safe, except blob that I shouldn't have included there.

More generally, a configurable whitelist is only useful in very specific cases, and most apps shouldn't configure it. It makes sense if your platform has unusual conventions that don't follow the rest of the world (angular/angular#12840 comes to mind). Something like tel:// or market:// urls shouldn't be allowed app-wide, and since with that PR, URLs can be trustAs'd, so in most cases a trustMarketUrl-filter to allow these in specific bindings makes more sense to me.

petebacondarwin · 2017-12-19T15:06:37Z

Closing in favour of #16378 , which is rebased and does not require breaking change to white lists.

googlebot added the cla: yes label Mar 16, 2016

gkalpak added component: misc core component: ngSanitize type: feature needs: breaking change severity: security labels Mar 20, 2016

gkalpak added this to the 1.6.x milestone Mar 20, 2016

googlebot added cla: no and removed cla: yes labels Mar 22, 2016

rjamet force-pushed the plumbUrlContextThroughSce branch from b4d5582 to 2ce3c85 Compare March 22, 2016 10:44

googlebot added cla: yes and removed cla: no labels Mar 22, 2016

rjamet force-pushed the plumbUrlContextThroughSce branch from 2ce3c85 to c71e082 Compare May 26, 2016 15:13

rjamet reviewed May 26, 2016
View reviewed changes

gkalpak reviewed Aug 18, 2016
View reviewed changes

petebacondarwin self-assigned this Aug 29, 2016

rjamet added 5 commits August 30, 2016 16:31

fix(IE): Embrace canonicalization of ng-src specified URLs.

23a5484

I'm not really sure why they weren't in the first place, but now they really are, and that doesn't seem like a negative change.

fix(*): handle empty srcsets, and jshint prefers === overall

2b5de99

fix(*): Minor fixes addressing gkalpak's comments.

4e92679

Fixes an error message in interpolate, the affected tests, and add URL context to $compile's getTrustedContext.

fix(*): Make this branch up to date.

1f867be

Merge branch 'master' of https://github.com/angular/angular.js into plumbUrlContextThroughSce

rjamet force-pushed the plumbUrlContextThroughSce branch from 9d2a6ef to 1f867be Compare August 31, 2016 11:41

petebacondarwin approved these changes Jan 30, 2017

View reviewed changes

rjamet commented Jan 31, 2017

View reviewed changes

rjamet mentioned this pull request Mar 3, 2017

feat($compile): Lower the security context of SVG's a and image xlink:href #15736

Closed

petebacondarwin modified the milestones: 1.6.x, 1.7.0 Oct 2, 2017

petebacondarwin removed their assignment Oct 31, 2017

petebacondarwin self-assigned this Dec 11, 2017

petebacondarwin mentioned this pull request Dec 19, 2017

feat($sce): handle URLs through the $sce service #16378

Merged

3 tasks

petebacondarwin closed this Dec 19, 2017

feat($sce): handle URLs through the $sce service, plus allow concatenation in that $sce contexts. #14250

feat($sce): handle URLs through the $sce service, plus allow concatenation in that $sce contexts. #14250

Uh oh!

Conversation

rjamet commented Mar 16, 2016

Uh oh!

rjamet commented Mar 16, 2016

Uh oh!

googlebot commented Mar 22, 2016

Uh oh!

googlebot commented Mar 22, 2016

Uh oh!

googlebot commented Mar 22, 2016

Uh oh!

googlebot commented Mar 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjamet commented May 26, 2016

Uh oh!

rjamet commented May 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjamet commented Aug 30, 2016

Uh oh!

rjamet commented Aug 31, 2016

Uh oh!

koto commented Nov 30, 2016

Uh oh!

petebacondarwin commented Nov 30, 2016

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjamet commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Dec 19, 2017

Uh oh!

Uh oh!

gkalpak commented Jan 31, 2017 •

edited

Loading

rjamet commented Jan 31, 2017 •

edited

Loading