Closed
Bug 1308688
(CVE-2017-5389)
Opened 8 years ago
Closed 8 years ago
Prevent WebExtensions from modifying requests to hosts with mozAddonManager permissions
Categories
(WebExtensions :: Request Handling, defect, P1)
Tracking
(firefox-esr45 unaffected, firefox50+ wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)
RESOLVED
FIXED
mozilla53
People
(Reporter: kmag, Assigned: aswan)
References
Details
(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [post-critsmash-triage][adv-main51+] triaged)
Attachments
(6 files, 5 obsolete files)
6.97 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
aswan
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.72 KB,
patch
|
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
Details | Diff | Splinter Review | |
1.26 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
+++ 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.
Updated•8 years ago
|
Keywords: csectype-priv-escalation,
sec-high
Whiteboard: sec-high
Updated•8 years ago
|
status-firefox52:
--- → affected
tracking-firefox52:
--- → +
Updated•8 years ago
|
Whiteboard: triaged
Assignee | ||
Comment 1•8 years ago
|
||
MozReview-Commit-ID: JI3qMmCtYFT
Assignee | ||
Comment 2•8 years ago
|
||
MozReview-Commit-ID: B9YbBr3fMuO
Attachment #8804457 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 3•8 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•8 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•8 years ago
|
||
MozReview-Commit-ID: B9YbBr3fMuO
Attachment #8804940 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8804457 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
MozReview-Commit-ID: 8DOZbPY7FZ9
Attachment #8804941 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Comment 7•8 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•8 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•8 years ago
|
||
MozReview-Commit-ID: B9YbBr3fMuO
Assignee | ||
Updated•8 years ago
|
Attachment #8804940 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
MozReview-Commit-ID: 8DOZbPY7FZ9
Assignee | ||
Updated•8 years ago
|
Attachment #8804941 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8805669 -
Flags: review+
Reporter | ||
Updated•8 years ago
|
Attachment #8805670 -
Flags: review+
Assignee | ||
Comment 11•8 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 12•8 years ago
|
||
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•8 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•8 years ago
|
||
MozReview-Commit-ID: JI3qMmCtYFT
Assignee | ||
Updated•8 years ago
|
Attachment #8804454 -
Attachment is obsolete: true
Assignee | ||
Comment 15•8 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+
Comment 16•8 years ago
|
||
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.
status-firefox50:
--- → wontfix
status-firefox51:
--- → affected
status-firefox-esr45:
--- → unaffected
tracking-firefox51:
--- → +
Whiteboard: triaged → [checkin on 11/29] triaged
Updated•8 years ago
|
Attachment #8805669 -
Flags: sec-approval? → sec-approval+
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb42f181ef9ada2ebce56d4f1beb0eb857b5487
Bug 1308688 Expose mozAddonManager allowed hosts to chrome r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/72ff2080ca120c16fd3b035c54b9ae8957fe83ed
Bug 1308688 r=kmag
Assignee | ||
Comment 19•8 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.
Comment 20•8 years ago
|
||
Should we be considering this for the 50.1 release?
status-firefox53:
--- → affected
tracking-firefox50:
--- → ?
tracking-firefox53:
--- → ?
Flags: needinfo?(aswan)
Whiteboard: [checkin on 11/29] triaged → triaged
Assignee | ||
Comment 21•8 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)
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dfb42f181ef9
https://hg.mozilla.org/mozilla-central/rev/72ff2080ca12
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 23•8 years ago
|
||
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•8 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•8 years ago
|
||
MozReview-Commit-ID: IinAfopVsw8
Assignee | ||
Updated•8 years ago
|
Attachment #8805669 -
Attachment is obsolete: true
Assignee | ||
Comment 26•8 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 27•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8815593 -
Flags: approval-mozilla-beta?
Attachment #8815593 -
Flags: approval-mozilla-beta+
Attachment #8815593 -
Flags: approval-mozilla-aurora?
Attachment #8815593 -
Flags: approval-mozilla-aurora+
Comment 29•8 years ago
|
||
Assignee | ||
Comment 31•8 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)
Comment 33•8 years ago
|
||
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.
Comment 34•8 years ago
|
||
arg. sorry, that last message got on the wrong bug.
Comment 35•8 years ago
|
||
Comment 36•8 years ago
|
||
Backed out from Beta for test_ext_webrequest.html failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/b28660caae81
https://treeherder.mozilla.org/logviewer.html#?job_id=2040832&repo=mozilla-beta
Assignee | ||
Comment 38•8 years ago
|
||
MozReview-Commit-ID: Igw1YsSP2Fv
Assignee | ||
Comment 39•8 years ago
|
||
MozReview-Commit-ID: IcH7YXuEJbW
Assignee | ||
Updated•8 years ago
|
Attachment #8816198 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 40•8 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•8 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+
Comment 42•8 years ago
|
||
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•8 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•8 years ago
|
||
MozReview-Commit-ID: GPPJm1Hlx88
Attachment #8816222 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Updated•8 years ago
|
Attachment #8816222 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 45•8 years ago
|
||
Comment 46•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/df4d31969686
https://hg.mozilla.org/releases/mozilla-beta/rev/7f9a7d1ed122
https://hg.mozilla.org/releases/mozilla-beta/rev/df6600c5e97d
Flags: in-testsuite+
Assignee | ||
Comment 47•8 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
Comment 48•8 years ago
|
||
Comment 49•8 years ago
|
||
(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)
Updated•8 years ago
|
Flags: needinfo?(aswan)
Assignee | ||
Comment 50•8 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)
Updated•8 years ago
|
Group: toolkit-core-security → core-security-release
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: triaged → [post-critsmash-triage]triaged
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]triaged → [post-critsmash-triage][adv-main51+] triaged
Updated•8 years ago
|
Alias: CVE-2017-5389
Assignee | ||
Comment 51•8 years ago
|
||
What's the timeline for landing the tests for this bug?
Flags: needinfo?(abillings)
Comment 52•8 years ago
|
||
I'd suggest next week. We'll have had 51 out for two weeks then.
Flags: needinfo?(abillings)
Assignee | ||
Comment 53•8 years ago
|
||
Comment 54•8 years ago
|
||
Assignee | ||
Comment 55•8 years ago
|
||
Comment 56•8 years ago
|
||
Assignee | ||
Comment 57•8 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)
Comment 58•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(abillings)
Updated•7 years ago
|
Group: core-security-release
Updated•7 years ago
|
Flags: needinfo?(dveditz)
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•