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
11 months ago

People

(Reporter: kmag, Assigned: aswan)

Tracking

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

51 Branch
mozilla53
Dependency tree / graph
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

3 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

3 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: 3 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

3 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

3 years ago
MozReview-Commit-ID: IinAfopVsw8
Assignee

Updated

3 years ago
Attachment #8805669 - Attachment is obsolete: true
Assignee

Comment 26

3 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

3 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

3 years ago
MozReview-Commit-ID: Igw1YsSP2Fv
Assignee

Comment 39

3 years ago
MozReview-Commit-ID: IcH7YXuEJbW
Assignee

Updated

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

Comment 40

3 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

3 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

3 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

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

Updated

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

Comment 47

3 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

3 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

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