fix($sce): Consider document base URL for 'self' URL policy. #15145

adob · 2016-09-15T22:38:18Z

This PR fixes the issue in bug #15144. If the page specifies a base URL using <base> tag, Angular may start to reject relative resource URL if the base URL domain does not match the loading domain. This PR adjusts matchUrl() to also consider the base URL (document.baseURI) in addition to location.href when matching URLs against the 'self' URl policy.

This PR does not introduce any breaking changes.

Security note: SCE policy can be circumvented by rogue <base> tags on the page. However, I believe this is not too much of a concern. If an attacker can inject <base> tags, then it's very likely they can inject arbitrary HTML, which is a way worse vulnerability.

gkalpak

Looks good in general. I have left a couple of minor (or not so minor) comments.

I think we need to account for base.href chaning dynamically.
And there should probably be some unit tests too.

gkalpak · 2016-09-21T12:51:04Z

src/ng/urlUtils.js

+ * or a parsed URL object.
+ * @returns {boolean} Whether the URL is same-origin as the document base URL.
+ */
+function urlIsSameOriginAsBaseUrl(requestUrl) {


Nit: I would rather not duplicate the code. Maybe have a base function that urlIsSameOrigin/urlIsSameOriginAsBase can delegate two (and pass the originUrl/baseUrl as parameters).

gkalpak · 2016-09-21T12:58:24Z

src/ng/urlUtils.js

+ * @returns {boolean} Whether the URL is same-origin as the document base URL.
+ */
+function urlIsSameOriginAsBaseUrl(requestUrl) {
+  if (!baseUrl) {


What if base.href changes dynamically ? I think we need to account for that too.

That is a good point. We can do this relatively painlessly by working directly with the anchor tag, since it keeps in sync with the base URL.

gkalpak · 2016-09-21T13:02:15Z

test/e2e/tests/base_tag.spec.js

+
+    urlIsTrusted = browser.executeScript('return isTrustedUrl("/relative")');
+    expect(urlIsTrusted).toBe(true);
+  });


Let's add a test for absolute URL that matches base.href.

adob · 2016-09-22T01:04:54Z

Please take another look. I refactored the code to use the the anchor tag more directly. Since it's always in always in sync with the base URL, the code now keeps up with updates to the base tag.

adob · 2016-09-22T04:09:17Z

It turned out the code was not working due to a bug in Firefox so I had to refactor again. When the base changed, the .href property on the anchor tag would get updated, but .host would retain its old value. I think the current implementation is very reasonable, though.

adob · 2016-09-29T03:34:03Z

Hey gkalpak, can you take another look? Let me know if there are any outstanding issues.

gkalpak · 2016-09-30T17:17:17Z

src/ng/urlUtils.js

+/**
+ * Parse a request URL and determine whether it is same-origin as the current document base URL.
+ *
+ * Note: The base URL is usually the same the document location (`location.href`) but can


can --> can be

Thanks. Done.

the same the document location --> the same as ...

gkalpak · 2016-09-30T17:18:11Z

src/ng/urlUtils.js

+ * @returns {boolean} Whether the URL is same-origin as the document base URL.
+ */
+function urlIsSameOriginAsBaseUrl(requestUrl) {
+  var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl;


Why not move this line into urlsAreSameOrigin as well?

gkalpak · 2016-09-30T17:23:04Z

src/ng/urlUtils.js

+ * @return {boolean} True if both URLs have the same origin, and false otherwise.
+ */
+function urlsAreSameOrigin(url1, url2) {
+  // IE sometimes includes a port in the 'host' property, even if it is the default 80 port so


Why is that a problem? Both URLs should be consistent (either contain the port or don't).
Under what circumstances can the URLs differ in that regard?

Reverting. You are right. This may have been more of an issue in a prior iteration of the code. If I recall correctly, the issue appeared in IE after the base tag had been changed dynamically. After this had occurred, urlParsingNode.host started returning hosts with ports, like www.example.com:80.

gkalpak · 2016-09-30T17:38:00Z

test/e2e/tests/base_tag.spec.js

+  it('tracks changes to the base URL', function() {
+    browser.executeScript(
+        'document.getElementsByTagName("base")[0].href = "http://xxx.example.com/";');
+    checkUrl('http://xxx.example.com/path/to/file.html', true);


Can you also add checkUrl('http://www.example.com/path/to/file.html', false); (just in case 😃)?

Done. I think it's okay to just add it, but let me know if it should be in a separate it() block.

gkalpak · 2016-09-30T17:39:32Z

test/e2e/tests/base_tag.spec.js

+  }
+
+  beforeAll(function() {
+    loadFixture('base_tag');


Can you change all names to kebab-case (to be consistent with the rest of the tests)?

adob · 2016-10-01T03:10:29Z

closed/re-opened pull request to get Travis CI to rebuild. Looks like there was some connection error to SauceLabs that caused the CI build to fail

gkalpak · 2016-10-01T14:21:45Z

Thx for the changes, @adob!
I just realized there are no unit tests (e.g. here and there).

Other than that LGTM 👍

adob · 2016-10-03T22:25:12Z

I added some tests for urlIsSameOriginAsBaseUrl() in test/ng/urlUtilsSpec.js. I'm not quite sure how to test non-default base URL behavior in the unit test. We generally need to change the global document base URL to fully test things, but it seems sub-optimal to do so from a unit test.

I originally put tests in an end-to-end test where we have more control over the page to avoid this kind of issue. So I think all the the tests are there, just kind of spread out.

adob · 2016-10-12T21:20:56Z

gkalpak, can you advise on how to test inside the unit tests? Or just LGTM if you think it's okay as is :-)

gkalpak · 2016-10-12T22:20:53Z

@adob, I was planning to look into it, but I haven't found the time yet. We are focusing on getting the 1.6.0 release out, so we prioritize related tasks and issues/PRs. Once 1.6.0 is out, I will come back to this.

_{(I am not even sure if it possible to add these tests in a reasonable way. I'll have to look into it. Feel free to leave the PR in its current state and I'll take it from there.)}

Thx for all the work so far btw 👍

adob · 2016-10-12T23:04:47Z

OK. Thank you

adob · 2017-01-03T18:39:13Z

gkalpak, can you take a look at this? Is there anything I should add?

gkalpak · 2017-01-03T21:58:47Z

I would still like to see a test in sceSpec.js. After that, leaving it up to @rjamet to give the final LGTM.

…licy. Page authors can use the base tag in HTML to specify URL to use as a base when resovling relative URLs. This can cause SCE to start rejecting relative URLs on the page because the fail the same-origin test. To improve compatiblity with the base tag, this commit changes the logic for matching urls to 'self' to allow URLs that match the protocol and domain of the base URL in addition to URLs that match the loading origin. Security note: If an attacker can inject a base tag into the page, they can circumvent SCE protections. Howerver, injecting a base tag typically requires the ability to inject arbitrary HTML into the page, which is a more serious vulnerabilty than bypassing SCE.

adob · 2017-01-05T20:26:25Z

Added unit tests to test/ng/sceSpecs.js. Please take a look.

gkalpak · 2017-01-09T10:18:10Z

src/ng/urlUtils.js

+/**
+ * Parse a request URL and determine whether it is same-origin as the current document base URL.
+ *
+ * Note: The base URL is usually the same the document location (`location.href`) but can


the same the document location --> the same as ...

gkalpak · 2017-01-09T10:30:24Z

test/ng/sceSpecs.js

+        afterAll(function() {
+          window.document.head.removeChild(baseElem);
+        });
+        function expectAllowed($sce, url) {


This helper (and expectBlocked below) seem redundant in this case. Let's remove them and inline the code.

gkalpak · 2017-01-09T10:31:48Z

test/ng/sceSpecs.js

+        var baseElem = window.document.createElement('BASE');
+        var cfg = {whitelist: ['self'], blacklist: []};
+        baseElem.setAttribute('href', window.location.protocol + '//foo.example.com/path/');
+        beforeAll(function() {


If there is no particular reason, I would prefer re-creating/adding/removing the <base> element for each test.

gkalpak

LGTM

gkalpak · 2017-01-10T10:49:36Z

cc @rjamet (to give the final LGTM as per discussion in #15537)

rjamet · 2017-01-10T10:53:22Z

LGTM too !

Page authors can use the `<base>` tag in HTML to specify URL to use as a base when resovling relative URLs. This can cause SCE to reject relative URLs on the page, because they fail the same-origin test. To improve compatibility with the `<base>` tag, this commit changes the logic for matching URLs to the 'self' policy to allow URLs that match the protocol and domain of the base URL in addition to URLs that match the loading origin. **Security Note:** If an attacker can inject a `<base>` tag into the page, they can circumvent SCE protections. However, injecting a `<base>` tag typically requires the ability to inject arbitrary HTML into the page, which is a more serious vulnerabilty than bypassing SCE. Fixes #15144 Closes #15145

gkalpak · 2017-01-12T09:28:53Z

FYI:
Reverted on v1.5.x (f73a651) and v1.6.x (f418ffd), while investigating security implications of this without #15597 (which is possibly a breaking change, thus not suitable for these branches).

rjamet · 2017-01-12T11:02:06Z

(cc @xtofian and @adob for more pairs of eyes: bindings on base href don't have a context currently in AngularJS. This PR adds baseURI's origin to the URLs allowed by the 'self' whitelist for RESOURCE_URLs, and #15597 bumps the context of base href bindings to RESOURCE_URL. Do you think this PR is okay even without the added context on base href, or did I miss anything?)

I assumed that was okay, but it's more complex than I thought, you're right to rollback :/ sorry I didn't figure that out before.

As far as i can tell, there's only three relevant cases: an app on a.com has a static base href to b.com (1), a dev-controlled binding on base href to b.com (2), or a user-controlled one (3). That's assuming the $sce RESOURCE_URL whitelist contains 'self'.

This PR makes it explicit that the other origin is trusted, so any relative link will work without $sce.trustAsResourceUrl (which is the intuitive meaning of 'self' IMO). If the dev added checks before on bindings when doing these conversions, they're still valid, same for $sce blacklist entries. The second PR doesn't change anything. I think that's @adob's use case.
Same general idea, except the $sce requirement in the second PR will signify to the dev that this is something security-sensitive (those are the breaking cases). Trust on b.com is already fairly established: before the first PR, none of the RESOURCE_URL relative links work without explicit security whitelisting or trustAs calls.
The situation is already dire for the app: the attacker has partial control over any relative link on the page, $sce or not: <script src="./whatever"> in templates is enough to have at least a weird race or maybe XSS. Since there's no binding on that, there'll be no $sce involved on that script, so this PR does't change that. It only makes the $sce accept them in case of bindings, but IMO that's not worse than before, and the second PR solves the whole mess assuming the whitelist/trustAs is used correctly.

The only caveat I can think of there is if the app lives on a.com, baseURI is b.com/something/, with a whitelist containing 'self' and 'b.com/something/**', then the whole b.com origin would become trusted after this. That also happens currently with apps living on a.com/myapp/... who trust the whole a.com origin by default, so it's arguably WAI. Finally, that's only after bindings happen: attacker control over a RESOURCE_URL binding is bad news in the first place regardless of baseURI issues.

On the plus side, that will remove the need for a lot of trustAsResourceUrl calls. They're seen as the only way to solve insecurl errors, and used in stuff like iframe src="{{variable | noCheckTrustAsFilter}}", which bypasses whitelists entirely. I'd better have same-origin checks than none at all.

I lean towards saying it's fine. The more concerning stuff with this PR without the other only happens when binding on base href, which is a bad idea in the first place. Otherwise, I don't see regressions, only quirkish behavior in unusual cases.

rjamet · 2017-01-12T13:19:25Z

Quick tests for the order of things when setting base href in various way: https://plnkr.co/edit/R54ihAo4q9gIlV6QPlye?p=preview for base href with no interpolation
https://plnkr.co/edit/kSIixu57Xd4Nxk1IP29L?p=preview for base href with interpolation

More quirks :/

gkalpak · 2017-01-17T11:01:13Z

I can think of some very extreme scenarios, like:

An app uses the self policy, has interpolation in base[href] and a vulnerability that allows a malicious user to take over base[href].
(This is already disastrous from a security perspective, but let's see how Angular affects security in such a situation.)
The app is supposed to use absolute URLs in RESOURCE_URL-sensitive contexts.
Due to a typo, a relative URL is used (which essentially allows the malicious user to control the destination).

Before both PRs: The app is "safe", since the destination URL is not trusted as RESOURCE_URL.
After both PRs: The app is "safe" (or at least the devs have the tools to properly audit it), because base[href] is RESOURCE_URL context.
Without the 2nd PR: Angular will happily trust the base[href]-based URL, without requiring a RESOURCE_URL context for it. (AFAIK, this i what SCE is trying to prevent.)

I understand this is a very contrived example, but still 😃

So, here is where we are currently:

v1.5.x does not contain either PR. (This is likely to stay this way, since v1.6.x is the new stable version and v1.5.x is not actively maintained.)
v1..6.x does not contain either PR. (We could add features, but we should avoid breaking changes, since v1.6.x is the current stable version.)
master (which will become v1.7+) contains the first PR and will also contain fix($compile) : Add base#href to the list of RESOURCE_URL context attrs #15597 (once merged). (This is OK, because we can make breaking changes on master.)

I think it is safest to keep things that way (and not merge this PR into v1.6.x without #15597).
Unless @rjamet strongly believes this PR is safe on its own.... 😃

rjamet · 2017-01-17T11:24:08Z

I agree with your analysis. We could argue on whether these PRs make the app less safe than it was before, but overall, I'm siding with you: both PRs at the same time would be the safest.

…attributes Previously, `base[href]` didn't have an SCE context. This was not ideal, because `base[href]` affects the behavior of all relative URLs across the page. Furthermore, since #15145, `base[href]` also affects which URLs are considered trusted under the 'self' policy for white- or black-listed resource URLs. This commit tightens the security of Angular apps, by adding `base[href]` to the list of RESOURCE_URL context attributes, essentially putting the same constraints on bindings to `base[href]` as on iframe or script sources. Refer to the [`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce) for more info on SCE trusted contexts. Closes #15597 BREAKING CHANGE: Previously, `<base href="{{ $ctrl.baseUrl }}" />` would not require `baseUrl` to be trusted as a RESOURCE_URL. Now, `baseUrl` will be sent to `$sce`'s RESOURCE_URL checks. By default, it will break unless `baseUrl` is of the same origin as the application document. Refer to the [`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce) for more info on how to trust a value in a RESOURCE_URL context. Also, concatenation in trusted contexts is not allowed, which means that the following won't work: `<base href="/something/{{ $ctrl.partialPath }}" />`. Either construct complex values in a controller (recommended): ```js this.baseUrl = '/something/' + this.partialPath; ``` ```html <base href="{{ $ctrl.baseUrl }}" /> ``` Or use string concatenation in the interpolation expression (not recommended except for the simplest of cases): ```html <base href="{{ '/something/' + $ctrl.partialPath }}" /> ```

Page authors can use the `<base>` tag in HTML to specify URL to use as a base when resovling relative URLs. This can cause SCE to reject relative URLs on the page, because they fail the same-origin test. To improve compatibility with the `<base>` tag, this commit changes the logic for matching URLs to the 'self' policy to allow URLs that match the protocol and domain of the base URL in addition to URLs that match the loading origin. **Security Note:** If an attacker can inject a `<base>` tag into the page, they can circumvent SCE protections. However, injecting a `<base>` tag typically requires the ability to inject arbitrary HTML into the page, which is a more serious vulnerabilty than bypassing SCE. Fixes angular#15144 Closes angular#15145

…attributes Previously, `base[href]` didn't have an SCE context. This was not ideal, because `base[href]` affects the behavior of all relative URLs across the page. Furthermore, since angular#15145, `base[href]` also affects which URLs are considered trusted under the 'self' policy for white- or black-listed resource URLs. This commit tightens the security of Angular apps, by adding `base[href]` to the list of RESOURCE_URL context attributes, essentially putting the same constraints on bindings to `base[href]` as on iframe or script sources. Refer to the [`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce) for more info on SCE trusted contexts. Closes angular#15597 BREAKING CHANGE: Previously, `<base href="{{ $ctrl.baseUrl }}" />` would not require `baseUrl` to be trusted as a RESOURCE_URL. Now, `baseUrl` will be sent to `$sce`'s RESOURCE_URL checks. By default, it will break unless `baseUrl` is of the same origin as the application document. Refer to the [`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce) for more info on how to trust a value in a RESOURCE_URL context. Also, concatenation in trusted contexts is not allowed, which means that the following won't work: `<base href="/something/{{ $ctrl.partialPath }}" />`. Either construct complex values in a controller (recommended): ```js this.baseUrl = '/something/' + this.partialPath; ``` ```html <base href="{{ $ctrl.baseUrl }}" /> ``` Or use string concatenation in the interpolation expression (not recommended except for the simplest of cases): ```html <base href="{{ '/something/' + $ctrl.partialPath }}" /> ```

googlebot added the cla: yes label Sep 15, 2016

gkalpak added type: feature needs: investigation frequency: low needs: review component: misc core severity: inconvenient labels Sep 19, 2016

gkalpak added this to the Backlog milestone Sep 19, 2016

gkalpak suggested changes Sep 21, 2016

View reviewed changes

gkalpak added needs: work and removed needs: investigation needs: review labels Sep 21, 2016

gkalpak suggested changes Sep 30, 2016

View reviewed changes

adob closed this Oct 1, 2016

adob reopened this Oct 1, 2016

adob force-pushed the fix-base-tag branch from 6367cc4 to 2872d5d Compare October 3, 2016 22:17

gkalpak mentioned this pull request Dec 21, 2016

fix(sce/urlUtils): Use baseURI for same-origin checks, and bump base#href to RESOURCE_URL #15537

Closed

adob added 2 commits January 4, 2017 15:21

improve test

08432a0

adob added 6 commits January 4, 2017 15:21

fix tests

49b8e0d

make urlIsSameOriginAsBaseUrl() run on Linux

abd7dd7

review feedback

38ddb97

remove redundant url parsing calls

48a0123

Add unit test

d2b2d37

adding unit test to sceSpec.js

9db4eaf

adob force-pushed the fix-base-tag branch from 2872d5d to 9db4eaf Compare January 4, 2017 23:26

adob added 3 commits January 4, 2017 17:35

lint fixups

01fc4f1

fix test for IE9

ebf45a4

fix lint issue

79ab27a

adob closed this Jan 5, 2017

adob reopened this Jan 5, 2017

gkalpak suggested changes Jan 9, 2017

View reviewed changes

fix typo and refactor tests as per code review

f6f5d9e

gkalpak approved these changes Jan 10, 2017

View reviewed changes

gkalpak closed this in 9412962 Jan 10, 2017

rjamet mentioned this pull request Jan 11, 2017

fix($compile) : Add base#href to the list of RESOURCE_URL context attrs #15597

Closed

fix($sce): Consider document base URL for 'self' URL policy. #15145

fix($sce): Consider document base URL for 'self' URL policy. #15145

Uh oh!

Conversation

adob commented Sep 15, 2016

Uh oh!

gkalpak 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!

adob commented Sep 22, 2016

Uh oh!

adob commented Sep 22, 2016

Uh oh!

adob commented Sep 29, 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!

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 Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

adob commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Oct 1, 2016

Uh oh!

adob commented Oct 3, 2016

Uh oh!

adob commented Oct 12, 2016

Uh oh!

gkalpak commented Oct 12, 2016

Uh oh!

adob commented Oct 12, 2016

Uh oh!

adob commented Jan 3, 2017

Uh oh!

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

Uh oh!

Uh oh!

adob commented Jan 5, 2017

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

gkalpak Sep 30, 2016 •

edited

Loading

adob commented Oct 1, 2016 •

edited

Loading

gkalpak commented Jan 3, 2017 •

edited

Loading

rjamet commented Jan 12, 2017 •

edited

Loading