Bug 1308688 (CVE-2017-5389)

Prevent WebExtensions from modifying requests to hosts with mozAddonManager permissions

RESOLVED FIXED in Firefox 51

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: kmag, Assigned: aswan)

Tracking

({csectype-priv-escalation, sec-high})

51 Branch
mozilla53
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50+ wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+] triaged)

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
+++ This bug was initially created as a clone of Bug #1295324 +++

As a follow-up to bug 1295324, we also need to prevent extensions from modifying requests to sites with elevated permissions, since they can gain the same elevated privileges by modifying CSP headers and then redirecting script requests to inject malicious content.
(Reporter)

Updated

3 years ago
See Also: → 1308689
Whiteboard: sec-high

Updated

3 years ago
Whiteboard: triaged
(Assignee)

Comment 1

3 years ago
MozReview-Commit-ID: JI3qMmCtYFT
(Assignee)

Comment 2

3 years ago
Posted patch webrequest.patch (obsolete) — Splinter Review
MozReview-Commit-ID: B9YbBr3fMuO
Attachment #8804457 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

3 years ago
The first patch was originally proposed and r+'ed on bug 1295324, but eventually discarded in favor of a different approach.
Since it touches webidl it needs a dom peer, but I'll wait until we've got agreement on the second patch.
Test case also coming shortly in a separate patch so it can be embargoed.
(Reporter)

Comment 4

3 years ago
Comment on attachment 8804457 [details] [diff] [review]
webrequest.patch

Review of attachment 8804457 [details] [diff] [review]:
-----------------------------------------------------------------

We also need to check the origin of the request so that it isn't possible to modify (e.g.) script load requests from AMO.
Attachment #8804457 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

3 years ago
Posted patch webrequest.patch (obsolete) — Splinter Review
MozReview-Commit-ID: B9YbBr3fMuO
Attachment #8804940 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

3 years ago
Attachment #8804457 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Posted patch tests (obsolete) — Splinter Review
MozReview-Commit-ID: 8DOZbPY7FZ9
Attachment #8804941 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 7

3 years ago
Comment on attachment 8804940 [details] [diff] [review]
webrequest.patch

Review of attachment 8804940 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/addons/WebRequest.jsm
@@ +605,5 @@
> +      if (!result || !opts.blocking
> +          || AddonManagerPermissions.isHostPermitted(uri.host)
> +          || Services.scriptSecurityManager.isSystemPrincipal(originPrincipal)
> +          || (originPrincipal.URI.host
> +              && AddonManagerPermissions.isHostPermitted(originPrincipal.URI.host))) {

We should probably just use the originPrincipal for these last two, and not worry about the triggeringPrincipal. The former matters much more, in this case.
Attachment #8804940 - Flags: review?(kmaglione+bmo) → review+
(Reporter)

Comment 8

3 years ago
Comment on attachment 8804941 [details] [diff] [review]
tests

Review of attachment 8804941 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_permission.html
@@ +25,5 @@
> +      return {};
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +
> +  }
> +      

Nit: Trailing space.

@@ +32,5 @@
> +      permissions: ["webRequest", "webRequestBlocking", "<all_urls>"],
> +    },
> +    background,
> +  };
> +        

Trailing space.
Attachment #8804941 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 9

3 years ago
Posted patch webrequest.patch (obsolete) — Splinter Review
MozReview-Commit-ID: B9YbBr3fMuO
(Assignee)

Updated

3 years ago
Attachment #8804940 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Posted patch testsSplinter Review
MozReview-Commit-ID: 8DOZbPY7FZ9
(Assignee)

Updated

3 years ago
Attachment #8804941 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8805669 - Flags: review+
(Reporter)

Updated

3 years ago
Attachment #8805670 - Flags: review+
(Assignee)

Comment 11

3 years ago
Comment on attachment 8804454 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Ben, you r+'ed this patch earlier for a different bug before we discarded it for a different approach.  But just in time for Halloween, it is rising from the grave...
Attachment #8804454 - Flags: review?(bkelly)
Comment on attachment 8804454 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Review of attachment 8804454 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp
@@ +33,5 @@
> +        host.Equals("addons-dev.allizom.org") ||
> +        host.Equals("discovery.addons-dev.allizom.org") ||
> +        host.Equals("testpilot.stage.mozaws.net") ||
> +        host.Equals("testpilot.dev.mozaws.net") ||
> +        host.Equals("example.com")) {

I think all of these should be `EqualsLiteral()`.  Also, do you need handle mixed case since host names are case insensitive?  If so, use `LowerCaseEqualsLiteral()`.

@@ +58,3 @@
>    }
>  
>    nsCString host;

This could be nsAutoCString to avoid memory allocation.

@@ +62,5 @@
>    if (NS_FAILED(rv)) {
>      return false;
>    }
>  
> +  if (IsValidHost(host)) {

This can just be `return IsValidHost(host);`.  No need for the if-statement and separate return statements.
Attachment #8804454 - Flags: review?(bkelly) → review+
(Assignee)

Comment 13

3 years ago
Comment on attachment 8805669 [details] [diff] [review]
webrequest.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Tests are in a separate patch.  The changes in this patch point in the general direction of the vulnerability but one needs a deeper understanding of the vulnerability to construct an exploit.  Moreover, anybody who understands the vulnerability and is paying attention has probably already seen bug 1295324 so they're unlikely to figure out much new from seeing this one.

Which older supported branches are affected by this flaw?
Everything since mozAddonManager shipped in 48.

If not all supported branches, which bug introduced the flaw?
1245571

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I haven't actually tried it but the changes should backport with little difficulty.

How likely is this patch to cause regressions; how much testing does it need?
Tests are included.  This could cause existing addons that use WebRequest.jsm to stop working when used on sites like AMO, but I don't think there's any way to avoid that (we had the same situation with bug 1295324 which lead to bug 1310082...)
Attachment #8805669 - Flags: sec-approval?
(Assignee)

Comment 14

3 years ago
MozReview-Commit-ID: JI3qMmCtYFT
(Assignee)

Updated

3 years ago
Attachment #8804454 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Comment on attachment 8805704 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

carrying over r+ from bkelly after applying his suggested changes.
Attachment #8805704 - Flags: review+
This is waaaaay too late for 50, which is on its final beta already.

This needs to go in two weeks after we ship, which is now Nov 15, making this a "sec-approval+ for checkin on 11/29" patch. Tests can't go in until after we ship the fix in a final release build so this approval is only for the non-tests patch.

Once it is on trunk, we'll want to backport it to Aurora and Beta.
Whiteboard: triaged → [checkin on 11/29] triaged
Attachment #8805669 - Flags: sec-approval? → sec-approval+
Comment on attachment 8805669 [details] [diff] [review]
webrequest.patch

Just a drive by comment.

- loadingPrincipal can be null, are we certain it is never null in this case?
- I've been looking at WebRequest.jsm as a base library with access to all, and cutting off access in ext-webrequest for things we don't want webextensions to see.  That's just my approach, not sure it really matters.
(Assignee)

Comment 19

2 years ago
I basically needed to rewrite the patch to accomodate all the changes since this was originally approved, I added a check that loadingPrincipal is not null.
On the second point, we don't pass enough information to the handlers to make the decision inside ext-webrequest.js.  Instead of changing that interface, I took the simpler route, partly since this will become irrelevant soon enough when non-webextension addons disappear.
Should we be considering this for the 50.1 release?
Flags: needinfo?(aswan)
Whiteboard: [checkin on 11/29] triaged → triaged
(Assignee)

Comment 21

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Should we be considering this for the 50.1 release?

Perhaps but I think the security folks are better equipped to make that decision.
Flags: needinfo?(aswan) → needinfo?(abillings)
https://hg.mozilla.org/mozilla-central/rev/dfb42f181ef9
https://hg.mozilla.org/mozilla-central/rev/72ff2080ca12
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please do nominate this for Aurora/Beta approval ASAP so we can hopefully get it uplifted in time for 51b6 at least.
(Assignee)

Comment 24

2 years ago
Comment on attachment 8805704 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Approval Request Comment
[Feature/Bug causing the regression]:
mozAddonManager (bug 1245571)

[User impact if declined]:
The patches here address a security vulnerability, without these patches, the vulnerability will be present.

[Is this code covered by automated tests?]:
Yes, though they are in a separate patch which will presumably remain embargoed for some time

[Has the fix been verified in Nightly?]:
other than automated testing, not yet

[Needs manual test from QE? If yes, steps to reproduce]: 
steps to reproduce are described at a high level in the original bug.  Kris or I could probably produce a working extension to use for testing

[List of other uplifts needed for the feature/fix]:
just the other patch attached to this bug

[Is the change risky?]:
[Why is the change risky/not risky?]:
this patch is not at all risky it just adds a new function exposed to javascript.  the second patch is riskier since it changes the behavior of the webextensions webRequest api but the changes are covered by unit tests.

[String changes made/needed]:
none
Attachment #8805704 - Flags: approval-mozilla-beta?
Attachment #8805704 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

2 years ago
MozReview-Commit-ID: IinAfopVsw8
(Assignee)

Updated

2 years ago
Attachment #8805669 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Comment on attachment 8815593 [details] [diff] [review]
webrequest.patch

This is the original webrequest.patch that had r+ from kmag and sec-approval from abillings rebased onto inbound.  see comment 24 for uplift details.
Attachment #8815593 - Flags: approval-mozilla-beta?
Attachment #8815593 - Flags: approval-mozilla-aurora?
Comment on attachment 8805704 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 6.
Attachment #8805704 - Flags: approval-mozilla-beta?
Attachment #8805704 - Flags: approval-mozilla-beta+
Attachment #8805704 - Flags: approval-mozilla-aurora?
Attachment #8805704 - Flags: approval-mozilla-aurora+
Attachment #8815593 - Flags: approval-mozilla-beta?
Attachment #8815593 - Flags: approval-mozilla-beta+
Attachment #8815593 - Flags: approval-mozilla-aurora?
Attachment #8815593 - Flags: approval-mozilla-aurora+
Tracking 53- as this is resolved fixed.
needs rebasing for beta
Flags: needinfo?(aswan)
(Assignee)

Comment 31

2 years ago
(In reply to Carsten Book [:Tomcat] from comment #30)
> needs rebasing for beta

If it was just the second patch that failed to apply, the original patch should apply cleanly:
https://bug1308688.bmoattachments.org/attachment.cgi?id=8805669

I'm sorry I have to step away for a moment and can't actually verify this myself right now, but I can do so later today if the above doesn't work or the sheriffs don't have the time or interest in trying that patch.
Flags: needinfo?(aswan)
Hello guys, do we need this in 50.1.0? Just checking since it's a sec-high.
Flags: needinfo?(dveditz)
Flags: needinfo?(aswan)
Ritu, this and bug 1320057 should go into 50.1, unless we're ok with doing a gofaster push, but that seems like a pain when a release is 2 weeks away.
arg.  sorry, that last message got on the wrong bug.
Tracking 53+ for this sec high issue.
(Assignee)

Comment 38

2 years ago
MozReview-Commit-ID: Igw1YsSP2Fv
(Assignee)

Comment 39

2 years ago
MozReview-Commit-ID: IcH7YXuEJbW
(Assignee)

Updated

2 years ago
Attachment #8816198 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 40

2 years ago
So the original patch was too aggressive in blocking header modifications for loads without a loading principal.  This was responsible for the mochitest failures on beta, and a user separately reported that this broke top level loads with user agent switcher.  But this suggests that we lost some test coverage with bug 1314492.  Shane, can you look into that?
Anyhow, I've updated new beta patches and will add a follow-up for central and aurora momentarily.
As for whether we want uplift to release, I think that's up to security, clearing my needinfo for that.
(Reporter)

Comment 41

2 years ago
Comment on attachment 8816198 [details] [diff] [review]
webrequest-beta.patch

Review of attachment 8816198 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also add a test to [1] in m-c and Aurora to make sure this doesn't regress again?

[1]: http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_webRequest.js
Attachment #8816198 - Flags: review?(kmaglione+bmo) → review+
Andrew, loads without a loadingPrincipal are top level loads, so yeah, they shouldn't be blocked.  I missed that aspect of the patch in my driveby.  Bug 1273138 broke top level loads, bug 1318800 (pending uplift to aurora) fixed that as well as fixing tests for that situation.  I think landing this bug may have beat bug 1318800, thus the tests didn't catch it.
(Assignee)

Comment 43

2 years ago
But from a quick glance, it looks like bug 1318800 covers passive listeners, but not blocking listeners that make changes to headers.  We had coverage of that prior to bug 1314492 but we apparently don't have it now.  That's what Kris is asking for in comment 41.
Flags: needinfo?(aswan)
(Assignee)

Comment 44

2 years ago
Posted patch followupSplinter Review
MozReview-Commit-ID: GPPJm1Hlx88
Attachment #8816222 - Flags: review?(kmaglione+bmo)
(Reporter)

Updated

2 years ago
Attachment #8816222 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 47

2 years ago
(In reply to Kris Maglione [:kmag] from comment #41)
> Can you also add a test to [1] in m-c and Aurora to make sure this doesn't
> regress again?

This bug is getting awfully cluttered, taking that to bug 1321615
(In reply to Andrew Swan [:aswan] from comment #21)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> > Should we be considering this for the 50.1 release?
> 
> Perhaps but I think the security folks are better equipped to make that
> decision.

What is the argument for needing this in 50.1 versus taking it in 51 (which is the default). I'm loathe to take non-desperate changes in 50.1 due to the fact that there are zero betas for 50.1.
Flags: needinfo?(abillings)
Flags: needinfo?(aswan)
(Assignee)

Comment 50

2 years ago
(In reply to Al Billings [:abillings] from comment #49)
> (In reply to Andrew Swan [:aswan] from comment #21)
> > (In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> > > Should we be considering this for the 50.1 release?
> > 
> > Perhaps but I think the security folks are better equipped to make that
> > decision.
> 
> What is the argument for needing this in 50.1 versus taking it in 51 (which
> is the default). I'm loathe to take non-desperate changes in 50.1 due to the
> fact that there are zero betas for 50.1.

Nothing specific other than the fact it was classified sec-high.  Sounds like leaving it as is (fixed in 51 and newer but nothing else) would be reasonable.
Flags: needinfo?(aswan)
Group: toolkit-core-security → core-security-release
Flags: qe-verify-
Whiteboard: triaged → [post-critsmash-triage]triaged
Whiteboard: [post-critsmash-triage]triaged → [post-critsmash-triage][adv-main51+] triaged
Alias: CVE-2017-5389
(Assignee)

Comment 51

2 years ago
What's the timeline for landing the tests for this bug?
Flags: needinfo?(abillings)
I'd suggest next week. We'll have had 51 out for two weeks then.
Flags: needinfo?(abillings)
(Assignee)

Comment 57

2 years ago
Last question on this one, the tests are now landed in central, should they be uplifted and if so, how far?
Flags: needinfo?(abillings)
We obviously have to land any fixups for existing tests (I don't think I saw any on this one).

We don't /have/ to uplift the unit tests for this fix, but then we will need to manually verify the fix on those branches. Sometimes uplifted fixes don't work because they built (unawares) on some more recent behavior that's not in the branches.
Flags: needinfo?(abillings)
Group: core-security-release
Flags: needinfo?(dveditz)

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.