1289273 - (CVE-2016-9073) windows.create schema doesn't specify "format": "relativeUrl"

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Will Bamberg [:wbamberg]]

Attachment: window-create.zip

I've attached a WebExtension, that does this:


chrome.browserAction.onClicked.addListener((tab) => {
  chrome.windows.create({
    url: "popups/popup.html"
  });
});


"popups/popup.html" is a path to a file packaged with the add-on. In Chrome, the document is loaded. In Firefox, A new window opens, and tries to load "http://www.popups.com/popup.html".

Comment 1

[Will Bamberg [:wbamberg]]

Sorry, this isn't a security-sensitive bug.

Comment 2

[Kris Maglione [:kmag]]

We don't currently do any resolution or security checks for these URLs. The most obvious problem this causes is that relative URLs don't work. But it also gives extensions the ability to load arbitrary URLs with the DANGEROUS_TO_LOAD flag, which is a much bigger problem.

Comment 3

[Andy McKay]

[Tracking Requested - why for this release]:

Comment 4

[Kris Maglione [:kmag]]

Attachment: Resolve URLs passed to windows.create relative to the caller

MozReview-Commit-ID: 3TUIK6EvO3q

Comment 5

[Kris Maglione [:kmag]]

Attachment: Add tests for attempted loads of unsafe URLs

MozReview-Commit-ID: LuswsfZ7CHH

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8798176 [details] [diff] [review]
Resolve URLs passed to windows.create relative to the caller

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily, without further details about the bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

This patch does not, but the follow-up patch with additional security tests does.

Which older supported branches are affected by this flaw?

All supported branches are affected.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply cleanly, or with minimal modification, to all non-ESR branches. ESR may require non-trivial modification.

How likely is this patch to cause regressions; how much testing does it need?

Not likely. The method that this changes is relatively well-tested, and the changes do not affect existing tests. Existing code should only be adversely affected if it is already passing illegal URLs, either maliciously or inadvertantly.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8798177 [details] [diff] [review]
Add tests for attempted loads of unsafe URLs

This makes sense as far as testing that common URLs rejected by checkLoadURL() aren't valid parameters for windows.create(), but I don't really understand what URI_DANGEROUS_TO_LOAD means and hence what the vulnerability in this is.  So either a review by somebody who does understand or a more detailed explanation here would be good.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8798176 [details] [diff] [review]
Resolve URLs passed to windows.create relative to the caller

sec-approval+ for trunk. As a sec-moderate, it doesn't technically require it but I'll give it.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92022bcd75830b63fb8247badbbffc16e6c34375
Bug 1289273: Resolve URLs passed to windows.create relative to the caller. r=aswan a=al

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8798176 [details] [diff] [review]
Resolve URLs passed to windows.create relative to the caller

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: This bug allows WebExtensions to load privileged URLs, and to potentially break out of their security sandbox.
[Describe test coverage new/current, TreeHerder]: The API that this change affects is covered by existing tests, and this patch adds additional tests for the changes. A separate patch adds tests for the security issues that this addresses.
[Risks and why]: Low. The method that this changes is relatively well-tested, and the changes do not affect existing tests. Existing code should only be adversely affected if it is already passing illegal URLs, either maliciously or inadvertantly.
[String/UUID change made/needed]: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92022bcd7583

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Whoops.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798176 [details] [diff] [review]
Resolve URLs passed to windows.create relative to the caller

Sec-mod, Aurora51+, Beta50+

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2714cdbc9db
https://hg.mozilla.org/releases/mozilla-beta/rev/35517b0212e4