feat(http): JSONP requests now require trusted resource URLs #15143

petebacondarwin · 2016-09-15T20:31:41Z

JSONP requests allow full access to the browser and the JavaScript context. So allowing a malicious attacker to make a JSONP request to an evil server could have bad results.

By requiring that JSONP urls are trusted we make it easier for developers to see that their app is only able to make JSONP requests to urls that they have audited.

There are two ways to trust:

whitelisting the url
explicitly marking a url as trusted by calling $sce.trustAs

The first commit in this PR ensures that all JSONP requests use a URL that is validated against the $sce ResourceUrl whitelist/blacklist checking. This commit creates a Breaking Change for app developers as they must now add all of their JSONP endpoints to the whitelist before their app will work. Although as significant breaking change, it is fairly simple to fix: just search for all calls to $http.jsonp and identify the url. Of course if the url is being generated dynamically then this is harder to fix but also indicates that perhaps there is a security vulnerability.

The second commit allows developers to provide explicitly trusted urls as a result of calling $sce.trustAsResourceUrl. This second approach has a few quirks:

the $http service needs to append parameters to the url, which means that the trusted url must be unwrapped, modified and then re-wrapped as trusted before passing it to $httpBackend.
it may be possible (??) that there might be an attack vector if an attacker is able to access a different endpoint by changing the parameters. But perhaps since the domain and path cannot be modified by the param serializer this is not an issue?

Is it acceptable (from a security point of view) to allow parameters to be added to a trusted url? If so then we could refactor the checks to happen in the $http service instead of $httpBackend which would remove the need for the re-wrapping of the built url.

Closes #11352
Closes #11328

googlebot · 2016-09-15T20:31:42Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-09-15T20:31:42Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

petebacondarwin · 2016-09-15T20:36:32Z

src/ng/http.js

@@ -1293,7 +1297,7 @@ function $HttpProvider() {
          reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
        }

-        $httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
+        $httpBackend(config.method, (url === $sce.valueOf(origUrl) ? origUrl : url), reqData, done, reqHeaders, config.timeout,


So this line is weird since it is saying: "if the newly built url is not the same as the original one then just use the original one." But using the original one defeats the purpose of being able to provide parameters, etc.

But then if we pass the untrusted URL through it will not be allowed by SCE inside the httpBackend, which now expects trusted URLs for JSONP.

And if we "re-trust" the built URL for JSONP requests we are undermining the security unless we can ensure that the buildUrl function cannot modify the URL in a way that allows an attack.

Given all of this, it might be easier to say that $http can only take strings as URLs, which is what is suggested by @klhuillier in #11328 (comment). That way the only way to get JSONP requests to work is if the request URL is in the SCE resource URL whitelist.

So the url parameter for $http only takes strings. Is toString() meant when talking about non strings?

So this line is weird since it is saying: "if the newly built url is not the same as the original one then just use the original one." But using the original one defeats the purpose of being able to provide parameters, etc.

Not quite 😃 It says: "If the newly built URL is the sameas the original one (possibly unwrapped) - in other words, if there were no parameters to be added - then use the original one (which is just a possibly wrapped version of the newly built one)."

It is true that if you want to pass parameters for JSONP requests, then an $sce-trusted URL won't help you. The resulting URL (inclusing the parameters) must be approved based on the white-/blacklist for resource URLs.
It is not an ideal solution, but I don't think we can ensure that a URL that is safe before buildUrl() will be safe after.

In any case, this behavior should be documented (if we decide to go with this implementation).

If we must rely on the whitelist for parameterised requests then there is no benefit in allowing explicitly trusted urls for those requests at all.

The trouble is that many JSONP requests are going to be parameterised, right? So the majority of requests will not benefit from being able to explicitly trust their url.

So perhaps we should only allow string urls (i.e. you cannot explicitly trust them), which is a massive breaking change for JSONP requests.

The trouble is that many JSONP requests are going to be parameterised, right?

There are two types of "parameterized". If the user trusts the URL (including the parameters - e.g. $http.jsonp($sce.trustAsResourceUrl('url?with=params', ...)), then it could be fine.
What they can't do, is pass the parameters as config.params (e.g. $http.jsonp('url', {params: {...}})).

So perhaps we should only allow string urls (i.e. you cannot explicitly trust them), which is a massive breaking change for JSONP requests.

This particular thing is not a breaking change, since we are already restricting to string URLs, right?

gkalpak · 2016-09-16T09:22:28Z

src/ng/httpBackend.js

+      // This is a pretty sensitive operation where we're allowing a script to have full access to
+      // our DOM and JS space.  So we require that the URL satisfies SCE.RESOURCE_URL.
+      url = $sce.getTrustedResourceUrl(url);
+    }


I would move this block below the next line (inside the other jsonp block).

gkalpak · 2016-09-16T09:26:13Z

src/ng/http.js

-          url = buildUrl(config.url, config.paramSerializer(config.params));
+          // origUrl could be a $sce trusted URL which used a wrapper class
+          origUrl = config.url,
+          url = $sce.valueOf(origUrl);


Isn't there a check near the top of $http that will fail if requestConfig.url is not a string?
That check also needs to be tweaked to accept $sce-wrapped values.

(I haven't looked at the failed tests, but I hope some failed because of this 😃)

There is! I just found it. All the tests in this PR are only testing the $httpBackend so none of the changes in this file are tested.

I am working on improving that :-)

gkalpak · 2016-09-16T09:30:12Z

src/ng/http.js

+          origUrl = config.url,
+          url = $sce.valueOf(origUrl);
+
+      url = buildUrl(url, config.paramSerializer(config.params));


I think it is simpler if all changes inside $http are done inside buildUrl. Something like this:

function buildUrl(url, serializedParams) { if (serializedParams.length > 0) { + url = $sce.valueOf(url); url += ((url.indexOf('?') === -1) ? '?' : '&') + serializedParams; } return url; }

gkalpak · 2016-09-16T09:40:05Z

src/ng/http.js

@@ -1293,7 +1297,7 @@ function $HttpProvider() {
          reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
        }

-        $httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
+        $httpBackend(config.method, (url === $sce.valueOf(origUrl) ? origUrl : url), reqData, done, reqHeaders, config.timeout,


So this line is weird since it is saying: "if the newly built url is not the same as the original one then just use the original one." But using the original one defeats the purpose of being able to provide parameters, etc.

Not quite 😃 It says: "If the newly built URL is the sameas the original one (possibly unwrapped) - in other words, if there were no parameters to be added - then use the original one (which is just a possibly wrapped version of the newly built one)."

It is true that if you want to pass parameters for JSONP requests, then an $sce-trusted URL won't help you. The resulting URL (inclusing the parameters) must be approved based on the white-/blacklist for resource URLs.
It is not an ideal solution, but I don't think we can ensure that a URL that is safe before buildUrl() will be safe after.

In any case, this behavior should be documented (if we decide to go with this implementation).

gkalpak · 2016-09-16T09:42:35Z

test/ng/httpBackendSpec.js

+    it('should not throw error if the url is an explicitly trusted resource', function() {
+      expect(function() {
+        $backend('JSONP', $sce.trustAsResourceUrl('http://example.org/path?cb=JSON_CALLBACK'), null, callback);
+      }).not.toThrowMinErr('$sce', 'insecurl');


I think generic .not.toThrow() expectations are preferrable, because a slight change in the error name (or a typo) render the test useless.

petebacondarwin

Thanks @gkalpak

petebacondarwin · 2016-09-16T09:55:29Z

src/ng/http.js

-          url = buildUrl(config.url, config.paramSerializer(config.params));
+          // origUrl could be a $sce trusted URL which used a wrapper class
+          origUrl = config.url,
+          url = $sce.valueOf(origUrl);


There is! I just found it. All the tests in this PR are only testing the $httpBackend so none of the changes in this file are tested.

I am working on improving that :-)

petebacondarwin · 2016-09-16T09:56:44Z

src/ng/http.js

+          origUrl = config.url,
+          url = $sce.valueOf(origUrl);
+
+      url = buildUrl(url, config.paramSerializer(config.params));


petebacondarwin · 2016-09-16T10:00:09Z

src/ng/http.js

@@ -1293,7 +1297,7 @@ function $HttpProvider() {
          reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
        }

-        $httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
+        $httpBackend(config.method, (url === $sce.valueOf(origUrl) ? origUrl : url), reqData, done, reqHeaders, config.timeout,


If we must rely on the whitelist for parameterised requests then there is no benefit in allowing explicitly trusted urls for those requests at all.

The trouble is that many JSONP requests are going to be parameterised, right? So the majority of requests will not benefit from being able to explicitly trust their url.

So perhaps we should only allow string urls (i.e. you cannot explicitly trust them), which is a massive breaking change for JSONP requests.

petebacondarwin · 2016-09-16T10:00:37Z

src/ng/httpBackend.js

+      // This is a pretty sensitive operation where we're allowing a script to have full access to
+      // our DOM and JS space.  So we require that the URL satisfies SCE.RESOURCE_URL.
+      url = $sce.getTrustedResourceUrl(url);
+    }


petebacondarwin · 2016-09-16T10:00:46Z

test/ng/httpBackendSpec.js

+    it('should not throw error if the url is an explicitly trusted resource', function() {
+      expect(function() {
+        $backend('JSONP', $sce.trustAsResourceUrl('http://example.org/path?cb=JSON_CALLBACK'), null, callback);
+      }).not.toThrowMinErr('$sce', 'insecurl');


gkalpak · 2016-09-16T11:43:48Z

src/ng/httpBackend.js


    if (lowercase(method) === 'jsonp') {
+      // This is a pretty sensitive operation where we're allowing a script to have full access to
+      // our DOM and JS space.  So we require that the URL satisfies SCE.RESOURCE_URL.
+      url = $sce.getTrustedResourceUrl(url) || $browser.url();


So, we are basically only allowing JSONP requests to whitelisted URLs (no support for trusting a URL "on-the-fly").
I am fine with this, except that we should document it 😃

Right - documentation ....

gkalpak · 2016-09-16T11:46:38Z

src/ng/httpBackend.js

  // TODO(vojta): fix the signature
  return function(method, url, post, callback, headers, timeout, withCredentials, responseType, eventHandlers, uploadEventHandlers) {
-    url = url || $browser.url();


Why this change? Why are we implicitly allowing JSONP requests to the current URL?
I don't expect this to be a security concern, but (a) it is most probably a programming error and (b) we are duplicating the || browser.url() part unnecessarily. Did I miss something?

I originally moved the trust check inside the second if clause as you suggested but it broke stuff as we need to convert a null url to the current browser url after the trust check. So now I basically moved this line to lines 64 and 74 below.

Couldn't we leave this url = url || browser.url() here and just add url = $sce.getTrustedResourceUrl(url) in the JSONP case.

The url = url || ... has to go after the getTrustedResource so you are back to earlier when you suggested that we move the check inside the if statement.

Anyway, I am hoping that we can accept that the param changes are not a security issue, in which case we can refactor the whole thing out and move the checks out from the $httpBackend, which means that we don't need to do the re-wrapping etc

Reject JSONP requests that are not trusted by `$sce` as "ResourceUrl". Closes angular#11352 BREAKING CHANGE All JSONP requests now require the URL to be whitelisted with the `$sceDelegateProvider.resourceUrlWhitelist()` method. You configure this list in a module configuration block: ``` appModule.config(['$sceDelegateProvider', function($sceDelegateProvider) { $sceDelegateProvider.resourceUrlWhiteList([ // Allow same origin resource loads. 'self', // Allow JSONP calls that match this pattern 'https://some.dataserver.com/**.jsonp?**` ]); }]); ```

mprobst · 2016-09-18T16:12:21Z

So if I understand this correctly, the question is how to vet the safety of a JSONP URL, and safe construction when appending parameters to it.

As far as I understand, Angular 1 always appends query parameters with ?=..., so as long as there is any original safe URL, it'll be safe to append them for the local page context (AFAIK). If your attack scenario is just whether the <script src=...> URL is safe, I think that'd be sufficient.

However many JSONP endpoints take a function name to call with the result as a parameter (e.g. ?callback=myFunction). If an attacker can inject a parameter that overrides this callback parameter, they can probably inject code into the page (e.g. ?callback=myFunction&arg1=val1&callback=evilCode).

To defend against that, I think indeed you'll have to get a trusted value for both the base URL and all of the query parameter names and their values. That might turn the API into something unusable though, $http.jsonp($sce.trustAsResourceUrl('/myurl'), {params: {a: $sce.trustAsResourceUrl(aval), b: $sce.trustAsResourceUrl(bval)}}); doesn't exactly roll off the tongue. And you'd still risk attackers controlling the parameter names.

I'm not sure if this is worth securing?

/CC @rjamet

rjamet · 2016-09-19T09:44:37Z

src/ng/http.js

@@ -1249,12 +1255,14 @@ function $HttpProvider() {
          cache,
          cachedResp,
          reqHeaders = config.headers,
-          url = buildUrl(config.url, config.paramSerializer(config.params));
+          needsTrustedUrl = (lowercase(config.method) === 'jsonp'),


needsTrustedUrl is a little misleading, since you have a $sce.trustAsUrl function for another context :/ needsTrustedResourceUrl ?

Right thanks!

petebacondarwin · 2016-09-19T11:36:36Z

@mprobst are you questioning whether we should attempt to secure the params or whether it is worth trying to secure JSONP at all in this way?

rjamet · 2016-09-19T11:59:30Z

For the ?callback=myFunction&arg1=val1&callback=evilCode case, maybe removing the rightmost duplicate parameters after appending them would be enough?

I'm not fond of having to trust parameters either: this doesn't really make sense with the ResourceUrl semantics, a param value isn't a resource url, and getTrustedResourceUrl with its same-origin whitelist will see everything as relative URLs :/ It just sounds bad. I'm not sure of the right approach, though. Being permissive and building the request on top of a trusted URL from strings isn't great, but it might be the best option.

mprobst · 2016-09-19T12:08:03Z

@petebacondarwin I think the reasonable thing to do is requiring the base URL to be a trusted resource URL, and then just accepting the fact that there's a risk factor in the callback keys and values. It's better than nothing, and at least I cannot come up with something more appealing.

I think @rjamet is of the same opinion, if I understand him correctly.

rjamet · 2016-09-19T12:18:35Z

src/ng/http.js

@@ -1293,7 +1301,7 @@ function $HttpProvider() {
          reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
        }

-        $httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
+        $httpBackend(config.method, needsTrustedUrl ? $sce.trustAsResourceUrl(url) : url, reqData, done, reqHeaders, config.timeout,


Slight nit, this is the first time AngularJS calls a trustAs function in the core library. That's not wrong, but it's a weird pattern: how about a parameter that will disable the check below instead?

(selfish reason for that: I maintain a custom $sce that uses the Closure's implementation of safe types and disables trustAs calls, and this is going to need ad-hoc patching. That's doable, so if you'd rather keep it this way it's fine)

If we are going with not securing the params then I can do a tidy refactor on this PR that removes the need for this trustAs call

rjamet · 2016-09-19T12:28:26Z

@mprobst @petebacondarwin There's an alternative: since the jsonp callback is a parameter, could these be $sce.getTrustedJs / trustAsJs ? You have the plumbing already in place. This type hasn't been used before, but since parameters there might be run as-is... But this might end up being yet another hurdle that everyone will search then copy-paste from a blog. WDYT ?

(i'm not that familiar with jsonp, so please point out if I'm wrong)

petebacondarwin · 2016-09-20T09:05:39Z

Closing in favour of #15161 - PTAL @mprobst @rjamet

… config The query parameter that will be used to transmit the JSONP callback to the server is now specified via the `jsonpCallbackParam` config value, instead of using the `JSON_CALLBACK` placeholder. * Any use of `JSON_CALLBACK` in a JSONP request URL will cause an error. * Any request that provides a parameter with the same name as that given by the `jsonpCallbackParam` config property will cause an error. This is to prevent malicious attack via the response from an app inadvertently allowing untrusted data to be used to generate the callback parameter. Closes #15161 Closes #15143 Closes #11352 Closes #11328 BREAKING CHANGE You can no longer use the `JSON_CALLBACK` placeholder in your JSONP requests. Instead you must provide the name of the query parameter that will pass the callback via the `jsonpCallbackParam` property of the config object, or app-wide via the `$http.defaults.jsonpCallbackParam` property, which is `"callback"` by default. Before this change: ``` $http.json('trusted/url?callback=JSON_CALLBACK'); $http.json('other/trusted/url', {params:cb:'JSON_CALLBACK'}); ``` After this change: ``` $http.json('trusted/url'); $http.json('other/trusted/url', {callbackParam:'cb'}); ```

… config The query parameter that will be used to transmit the JSONP callback to the server is now specified via the `jsonpCallbackParam` config value, instead of using the `JSON_CALLBACK` placeholder. * Any use of `JSON_CALLBACK` in a JSONP request URL will cause an error. * Any request that provides a parameter with the same name as that given by the `jsonpCallbackParam` config property will cause an error. This is to prevent malicious attack via the response from an app inadvertently allowing untrusted data to be used to generate the callback parameter. Closes angular#15161 Closes angular#15143 Closes angular#11352 Closes angular#11328 BREAKING CHANGE You can no longer use the `JSON_CALLBACK` placeholder in your JSONP requests. Instead you must provide the name of the query parameter that will pass the callback via the `jsonpCallbackParam` property of the config object, or app-wide via the `$http.defaults.jsonpCallbackParam` property, which is `"callback"` by default. Before this change: ``` $http.json('trusted/url?callback=JSON_CALLBACK'); $http.json('other/trusted/url', {params:cb:'JSON_CALLBACK'}); ``` After this change: ``` $http.json('trusted/url'); $http.json('other/trusted/url', {callbackParam:'cb'}); ```

petebacondarwin added type: feature component: $http needs: work severity: security branch: 1.6.x labels Sep 15, 2016

petebacondarwin added this to the 1.6.0 milestone Sep 15, 2016

googlebot added the cla: no label Sep 15, 2016

petebacondarwin mentioned this pull request Sep 15, 2016

sec(http): JSONP should require trusted resource URLs #11623

Closed

petebacondarwin commented Sep 15, 2016

View reviewed changes

gkalpak suggested changes Sep 16, 2016

View reviewed changes

petebacondarwin commented Sep 16, 2016

View reviewed changes

petebacondarwin force-pushed the pr-11623 branch 4 times, most recently from c364004 to de4a462 Compare September 16, 2016 10:51

gkalpak suggested changes Sep 16, 2016

View reviewed changes

petebacondarwin force-pushed the pr-11623 branch from de4a462 to f5a72fc Compare September 16, 2016 12:57

petebacondarwin added the needs: docs label Sep 16, 2016

feat($http): allow explicitly trusted URLs for JSONP requests

800f9ad

rjamet reviewed Sep 19, 2016

View reviewed changes

petebacondarwin mentioned this pull request Sep 20, 2016

feat($httpBackend): JSONP requests now require trusted resource #15161

Closed

3 tasks

petebacondarwin closed this Sep 20, 2016

feat(http): JSONP requests now require trusted resource URLs #15143

feat(http): JSONP requests now require trusted resource URLs #15143

Conversation

petebacondarwin commented Sep 15, 2016 • edited Loading

googlebot commented Sep 15, 2016

googlebot commented Sep 15, 2016

petebacondarwin Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprobst commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Sep 19, 2016

rjamet commented Sep 19, 2016

mprobst commented Sep 19, 2016

rjamet Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjamet commented Sep 19, 2016 • edited Loading

petebacondarwin commented Sep 20, 2016 • edited Loading

petebacondarwin commented Sep 15, 2016 •

edited

Loading

petebacondarwin Sep 15, 2016 •

edited

Loading

gkalpak Sep 16, 2016 •

edited

Loading

rjamet Sep 19, 2016 •

edited

Loading

rjamet commented Sep 19, 2016 •

edited

Loading

petebacondarwin commented Sep 20, 2016 •

edited

Loading