Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1485,12 +1485,14 @@ function allowAutoBootstrap(document) {
var src = document.currentScript.getAttribute('src');
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Same-origin resources are always allowed, even for non-whitelisted schemes.
return true;
}
switch (scriptProtocol) {
// Disabled bootstrapping unless angular.js was loaded from a known scheme used on the web.
// This is to prevent angular.js bundled with browser extensions from being used to bypass the
// content security policy in web pages and other browser extensions.
switch (link.protocol) {
case 'http:':
case 'https:':
case 'ftp:':
Expand Down
34 changes: 34 additions & 0 deletions test/AngularSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,7 +1683,41 @@ describe('angular', function() {
dealoc(appElement);
});

it('should bootstrap from an extension into an extension document for same-origin documents only', function() {
if (msie) return; // IE does not support document.currentScript (nor extensions with protocol), so skip test.

// Extension URLs are browser-specific, so we must choose a scheme that is supported by the browser to make
// sure that the URL is properly parsed.
var extensionScheme;
var userAgent = window.navigator.userAgent;
if (/Firefox\//.test(userAgent)) {
extensionScheme = 'moz-extension';
} else if (/Edge\//.test(userAgent)) {
extensionScheme = 'ms-browser-extension';
} else if (/Chrome\//.test(userAgent)) {
extensionScheme = 'chrome-extension';
} else if (/Safari\//.test(userAgent)) {
extensionScheme = 'safari-extension';
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

extensionScheme = 'browserext'; // Upcoming standard scheme.
}

var src = extensionScheme + '://something';
// Fake a minimal document object (the actual document.currentScript is readonly).
var fakeDoc = {
currentScript: { getAttribute: function() { return src; } },
location: {protocol: extensionScheme + ':', origin: extensionScheme + '://something'},
createElement: document.createElement.bind(document)
};
expect(allowAutoBootstrap(fakeDoc)).toBe(true);

src = extensionScheme + '://something-else';
expect(allowAutoBootstrap(fakeDoc)).toBe(false);
});

it('should not bootstrap from an extension into a non-extension document', function() {
if (msie) return; // IE does not support document.currentScript (nor extensions with protocol), so skip test.

var src = 'resource://something';
// Fake a minimal document object (the actual document.currentScript is readonly).
var fakeDoc = {
Expand Down