Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

@Rob--W
Copy link
Contributor

commented Nov 24, 2016

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

@mprobst @petebacondarwin

@googlebot googlebot added the cla: yes label Nov 24, 2016
@@ -1454,8 +1454,7 @@ function allowAutoBootstrap(document) {
var link = document.createElement('a');
link.href = src;
var scriptProtocol = link.protocol;
var docLoadProtocol = document.location.protocol;
if (docLoadProtocol === scriptProtocol) {
if (document.location.origin === link.origin) {

This comment has been minimized.

Copy link
@mprobst

mprobst Nov 24, 2016

Contributor

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?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 24, 2016

Member

+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.

This comment has been minimized.

Copy link
@Rob--W

Rob--W Nov 24, 2016

Author Contributor

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).

This comment has been minimized.

Copy link
@mprobst

mprobst Nov 28, 2016

Contributor

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.

@Narretz Narretz added this to the Ice Box milestone Nov 24, 2016
@petebacondarwin petebacondarwin modified the milestones: Purgatory, Ice Box Nov 28, 2016
@Rob--W Rob--W force-pushed the Rob--W:patch-1 branch 2 times, most recently from b20e3ab to db6d4a9 Nov 28, 2016
@Rob--W

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2016

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

@mprobst

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2016

LGTM, thanks for the patch.

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

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

@Rob--W Rob--W force-pushed the Rob--W:patch-1 branch from db6d4a9 to 7618ca8 Nov 28, 2016
@mgol

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

@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

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

@Rob--W

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2016

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 comment has been minimized.

Copy link
Contributor

commented Nov 28, 2016

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.

@Rob--W Rob--W force-pushed the Rob--W:patch-1 branch from 7618ca8 to 774273e Nov 28, 2016
extensionScheme = 'safari-extension';
} else if (/Trident/.test(userAgent)) {
return; // IE does not support extensions with a standard protocol, so skip the test.
} else {

This comment has been minimized.

Copy link
@mgol

mgol Nov 28, 2016

Member

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.

This comment has been minimized.

Copy link
@Rob--W

Rob--W Nov 29, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@mgol

mgol Nov 29, 2016

Member

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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

@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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

@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 Rob--W force-pushed the Rob--W:patch-1 branch from 774273e to 9bfcdc0 Nov 29, 2016
@Rob--W

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

@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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

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
mgol approved these changes Nov 29, 2016
@mgol

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

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

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

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

@mprobst

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2016

LGTM

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

I'll merge tonight or tomorrow

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

Landed as 465d173 (and bdeb339).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.