feat(security): do not bootstrap from unknown schemes with a different origin

[Rob--W]

Follow-up to #15427. There is no reason for allowing cross-origin automatic bootstrapping at URLs with an unknown scheme.

@mprobst @petebacondarwin

[mprobst]

I'm not so sure about this - are we certain that we won't have extension protocols in which origin is different between e.g. different parts of the extension, but the scheme/protocol would be the same?

I think it's a good idea to be conservative in areas where we know it's a security issue, but do we have any idea about an attack vector at all for this?

[IgorMinar]

+1, this looks too risky and doesn't seem to add that much value. Let's deal with this once we have a concrete example where the original change is not sufficient.

[Rob--W]

are we certain that we won't have extension protocols in which origin is different between e.g. different parts of the extension, but the scheme/protocol would be the same?

Yes I am. All URLS that reference parts of one extension have a single origin (at least in Chrome, Firefox, Safari, Opera; I haven't looked at others but I assume the same holds too).
I can't think of a legitimate use case for loading Angular.js from another extension.

I think it's a good idea to be conservative in areas where we know it's a security issue, but do we have any idea about an attack vector at all for this?

Bootstrapping Angular.js from an extension in a normal web page was disabled in #15427. There is no reason to believe that extension pages are less affected by the potential issues that are faced by normal web pages.

Extension URLs can be loaded regardless of CSP (https://w3c.github.io/webappsec-csp/#extensions). So if extension A publishes a URL to an unsafe version of Angular.js, and extension B has a XSS vulnerability, then an attacker can misuse the unsafe Angular.js from extension A to bypass CSP in extension B, unless Angular.js blocks cross-origin extension resources (as I propose here).

These conditions are not too farfetched: I have reviewed hundreds of add-ons and XSS is not uncommon. And as you know, Angular.js has had its share in CSP-bypassing exploits. Together, there is some risk in allowing Angular.js to be loaded in a different extension.

If you want a concrete example of a CSP bypass with an extension resource, see: Synzvato/decentraleyes#130 (the CSP bypass was shown in a normal web page, but it would work equally well in an extension page).

[mprobst]

Ah I get it, you're talking about extensions protecting themselves using CSP. That's a fair point, this code wasn't intended to cover this but we might as well.

• can you add a comment explaining that this covers two cases, loading Angular from an extension into a website, and across extensions?
• can you add a test that demonstrates this, so we don't break it in the future?

Also a nit: can you move the var scriptProtocol and inline it into the switch below? We no longer need the separate variable here.

[Rob--W]

@mprobst Done. PTAL. I hope to have this patch included in 1.5.9 before Angular.js submissions are allowed in AMO.

[mprobst]

LGTM, thanks for the patch.

[petebacondarwin]

I'll merge this tomorrow morning unless @IgorMinar and @matsko do so as part of their syncing today.

[petebacondarwin]

@Rob--W this is failing a unit test on Travis. Can you take a look?

[mgol]

@Rob--W We've already released Angular 1.5.9 with a previous version of the patch so this will only be a part of 1.5.10.

[mgol]

@Rob--W For reference, this is what we released in 1.5.9: https://github.com/angular/angular.js/blob/v1.5.9/src/Angular.js#L1448-L1471.

[Rob--W]

The tests are failing because those browsers have different ideas about what a non-standard origin means. Chrome (and I guess Safari as well) view "resource://something"'s origin as "resource://" (as if it was a file:-URL). I'll fix the tests.

@Rob--W We've already released Angular 1.5.9 with a previous version of the patch... Can you still lift the block for Angular 1.5.9+ or is landing this PR a blocker for that?

I don't have the authority to say anything about that. I'm a volunteer editor at the Add-on review team, which is also how I learned about this bug. My feedback to the original PR and this PR here are actions independent of Mozilla.

Old-style Firefox add-ons don't have a CSP by default, so whether the patch is in 1.5.9 or not does not affect the security of those add-ons (if these add-ons have a XSS issue, then they're already hosed). So I think that the patch does not need to block the whitelisting of 1.5.9.
The patch is not without merit though: the new style Firefox add-ons (WebExtensions) do have a CSP by default. So I'm in favor of including this patch in 1.5.9.

[mprobst]

This shouldn't block the release. This is really just fixing a corner case of a corner case a bit more. To exploit the potential issue, you need an extension that has an XSS, and uses CSP in a way that's effective to block the problem, and need a user to have another extension installed that uses AngularJS, and then predict what extension that is. That's certainly a possibility, but really not a release blocker.

[mgol]

Do we need all those ifs? We're not using real extension URLs here anyway but we're mocking the document so I think resource: would be enough as it was before.

Especially that in real world on IE allowAutoBootstrap will always return true as there is no document.currentScript there.

[Rob--W]

The initial version of my patch used resource:-URLs, and that caused failures in IE (9, 10, 11), Safari and Chrome: https://travis-ci.org/angular/angular.js/jobs/179590522

An excerpt of what I said half an hour ago at #15428 (comment):

The tests are failing because those browsers have different ideas about what a non-standard origin means. Chrome (and I guess Safari as well) view "resource://something"'s origin as "resource://" (as if it was a file:-URL). I'll fix the tests.

[mgol]

I see. In that case this test shouldn't be run in IE at all as it will return true immediately in the very first document.currentScript check as it doesn't support this API. Maybe add:

if (msie) return true;

at the beginning of the test you added?

[mgol]

So I'm in favor of including this patch in 1.5.9.

As I said, we've already released 1.5.9 so we can't add anything to it. This will have to wait for 1.5.10.

[mgol]

@Rob--W The tests were previously failing because in the document mock you defined location.origin but not location.host and the code was using location.host for its comparisons. It meant the condition was never going to be true.

[Rob--W]

So I'm in favor of including this patch in 1.5.9.

As I said, we've already released 1.5.9 so we can't add anything to it. This will have to wait for 1.5.10.

Ok,1.5.10 it is.

[Rob--W]

@Rob--W The tests were previously failing because in the document mock you defined location.origin but not location.host and the code was using location.host for its comparisons. It meant the condition was never going to be true.

The very first version of my patch, with tests (and also the CI results I linked above) were using origin. Only after observing the test failure I switched to location.protocol/host to see whether it would change part of the test results, because I saw a comment on MDN about the invalidity of location.origin in early IE11 releases in Windows 10 - https://developer.mozilla.org/en-US/docs/Web/API/Window/location#Browser_compatibility

The currently running tests in CI are using location.origin

[mgol]

@Rob--W I see, thanks for the background. Normally we couldn't use location.origin as it doesn't exist in IE<11 at all and even in some IE 11 versions but no IE version supports document.currentScript so the allowAutoBootstrap function will return true on the very first check.

[Rob--W]

@mgol Done. Hopefully "msie" never becomes true when Edge is added to the tests (or did I misinterpret the test results? Edge seems not among the browsers of the unit tests, even though there is test code that branches based on the presence of Edge).

[mgol]

We haven't added Edge testing yet, see #14401. msie is basically the non-standard document.documentMode which exists only in IE (and not in Edge).

[mgol]

I'm thinking if there's another platform that doesn't support location.origin or anchorElement.origin but doesn't support document.currentScript but I can't find any so we might be good if tests pass (I had to restart them because of Safari 8 flakiness).

@Rob--W If we had to care about IE inside the allowAutoBootstrap then location.origin would be only our first problem, another one would be a broken anchor link parser. :)

[mgol]

LGTM but since the logic changed I'd like @mprobst to have one final look before we land it.

[petebacondarwin]

Thanks @mgol and @Rob--W for working through this. It is great to wake up in the morning to find everything fixed! I'll merge today and see if we can fast track this into 1.5.10. But as we have agreed with Mozilla this is not a requirement to get Angular 1.5.9 whitelisted.

[petebacondarwin]

Given that we are mocking out the document in order to setup the src is there any point in sniffing the browser to choose the extension type? Why not iterate through all the browser extension types and test them all on every browser? Is there anything in document.createElement() that changes between browsers for this test?

One last PTAL for you @mprobst.

[Rob--W]

@petebacondarwin Yes. URL parsing differs, see #15428 (comment)

[petebacondarwin]

@Rob--W - OK thanks for the clarification. LGTM

[mprobst]

LGTM

[petebacondarwin]

I'll merge tonight or tomorrow

[petebacondarwin]

Landed as 465d173 (and bdeb339).