Closed
Bug 1244248
Opened 8 years ago
Closed 8 years ago
Once an add-on can execute code it can override the certificate checking code
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Keywords: sec-high, Whiteboard: [adv-main45-] sec-moderate in 44 (pref flip available), higher value bypass in later versions.)
Attachments
(4 files)
2.72 KB,
application/x-xpinstall
|
Details | |
2.65 KB,
patch
|
rhelmer
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
209.00 KB,
image/png
|
Details | |
6.70 KB,
patch
|
rhelmer
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The attached extension is a simple bootstrap script that makes use of the problem in bug 1244246. It is unsigned. If sideloaded the install script executes when first detected which allows it to override the certificate database that the add-ons manager uses to check certificates, forces the add-ons manager to re-scan installed add-ons thus making itself appear signed and then enables itself and loads a webpage. Other than sideloading this requires no user interaction.
Assignee | ||
Comment 1•8 years ago
|
||
There is a simple way to avoid the override of the certificate database which will mean the add-on can only run code until the user restarts Firefox. Bug 1244246 really needs to be fixed though.
Assignee: nobody → dtownsend
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8713835 -
Flags: review?(rhelmer)
Comment 3•8 years ago
|
||
Comment on attachment 8713835 [details] [diff] [review] patch This looks good to me, this should prevent attachment 8713757 [details] (I'll verify locally too). Is there any way for addons to cause AddonManager to restart?
Attachment #8713835 -
Flags: review?(rhelmer) → review+
Comment 4•8 years ago
|
||
(In reply to Robert Helmer [:rhelmer] from comment #3) > Comment on attachment 8713835 [details] [diff] [review] > patch > > This looks good to me, this should prevent attachment 8713757 [details] > (I'll verify locally too). > > Is there any way for addons to cause AddonManager to restart? They could call unloadModule and then re-Cu.import() it, though that would be pretty scary...
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #4) > (In reply to Robert Helmer [:rhelmer] from comment #3) > > Comment on attachment 8713835 [details] [diff] [review] > > patch > > > > This looks good to me, this should prevent attachment 8713757 [details] > > (I'll verify locally too). > > > > Is there any way for addons to cause AddonManager to restart? > > They could call unloadModule and then re-Cu.import() it, though that would > be pretty scary... It would probably break a lot of things but there is probably something there but that sort of attack can be stopped by bug 1244246.
Comment 6•8 years ago
|
||
This patch prevents the PoC for me locally - unsigned side-loaded add-on is disabled as expected.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Robert Helmer [:rhelmer] from comment #7) > Should we have a unit test here too? Probably yes, but I'm concerned that until bug 1244246 and this are fixed on all channels a test will make it very obvious how to take advantage of this. How do we normally deal with such situations Dan?
Flags: needinfo?(dtownsend) → needinfo?(dveditz)
Comment 9•8 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #8) > (In reply to Robert Helmer [:rhelmer] from comment #7) > > Should we have a unit test here too? > > Probably yes, but I'm concerned that until bug 1244246 and this are fixed on > all channels a test will make it very obvious how to take advantage of this. > How do we normally deal with such situations Dan? Leaving needinfo so Dan can confirm, but I think generally tests get written and reviewed, but landed after the fix has been released on all channels where we want to release it (and/or until channels where the bug exists but we might not uplift patches such as ESR are no longer supported). You probably also need sec-approval for the main patch if this is sec-high or sec-critical, which I think it arguably might be considered because of the potential for abuse/privilege escalation.
Updated•8 years ago
|
Group: firefox-core-security → toolkit-core-security
Flags: needinfo?(dveditz)
Assignee | ||
Comment 10•8 years ago
|
||
Not sure Dan meant to drop the needinfo there...
Flags: needinfo?(dveditz)
Comment 11•8 years ago
|
||
I didn't, thanks. Gijs is correct in comment 9. In this case, though, we don't care about back-porting to ESR 38 because signing isn't enforced, and while we do check signatures in Firefox 44 the disabling pref is still available so side-loaded add-ons could just flip that instead. Given all that it's probably fine to land the test with the patch, unless you think it hints too hard at bug 1244246. I think we're OK though. It's good to upload tests for security fixes as a separate patch so that we can think through that kind of thing during sec-approval ("go ahead and land with the tests" vs. "please don't check in the tests until after release"). We use the "in-testsuite?" flag to track tests that haven't landed for security bugs, or some people clone a separate "Check in tests for bug XXXXX" bug for it. Use whichever will remind you better. The other thing sec-approval will do is try to judge whether the patches are revealing enough to require landing closer to release day, or are too risky and should absolutely not land near the release. This patch looks really safe so the question will be how revealing is it?
status-firefox44:
--- → wontfix
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox-esr38:
--- → disabled
tracking-firefox45:
--- → +
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
Flags: needinfo?(dveditz) → in-testsuite?
Keywords: sec-high
Whiteboard: sec-moderate in 44 (pref flip available), higher value bypass in later versions.
Assignee | ||
Comment 12•8 years ago
|
||
Updated with a test
Attachment #8713835 -
Attachment is obsolete: true
Attachment #8715059 -
Flags: review?(rhelmer)
Updated•8 years ago
|
Attachment #8715059 -
Flags: review?(rhelmer) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8715059 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? The test code demonstrates how to override the certificate DB and exploit the issue but requires being able to run chrome privileged code first. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The test makes it obvious, the patch itself looks pretty innocuous. Which older supported branches are affected by this flaw? Just current release branches, ESR38 is unaffected. If not all supported branches, which bug introduced the flaw? Bug 1038068 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The existing patch should apply. How likely is this patch to cause regressions; how much testing does it need? Very unlikely.
Attachment #8715059 -
Flags: sec-approval?
Comment 14•8 years ago
|
||
Does this default to affected or disabled via pref in shipped 44? Normally, you don't get to check in tests for high and critical rated security bugs (unless no shipped version of the software is affected).
Flags: needinfo?(dtownsend)
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Al Billings [:abillings] from comment #14) > Does this default to affected or disabled via pref in shipped 44? > > Normally, you don't get to check in tests for high and critical rated > security bugs (unless no shipped version of the software is affected). I'd say affected since by default unsigned add-ons are disabled in 44.
Flags: needinfo?(dtownsend)
Comment 16•8 years ago
|
||
Comment on attachment 8715059 [details] [diff] [review] patch sec-approval+. Let's get nominations for Aurora and Beta as well.
Attachment #8715059 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Al Billings [:abillings] from comment #16) > Comment on attachment 8715059 [details] [diff] [review] > patch > > sec-approval+. > > Let's get nominations for Aurora and Beta as well. Just to be clear before I go ahead and land, should I land with the test or hold that back for now?
Flags: needinfo?(abillings)
Comment 18•8 years ago
|
||
Ah, thanks for asking. Land without test. The test shouldn't be landed until a week or two after we ship a release with the fix so we don't paint too much of a target on it.
Flags: needinfo?(abillings)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8713835 [details] [diff] [review] patch This is the same patch just without the test so what should be uplifted. Approval Request Comment [Feature/regressing bug #]: Signed add-ons [User impact if declined]: Hides a means to circumvent add-on signing requirements [Describe test coverage new/current, TreeHerder]: Landed on m-c, we should get plenty of verification from the nightly testers, they get upset when they can't install add-ons! [Risks and why]: Low risk, this just caches a service as a global variable rather than getting it fresh when needed [String/UUID change made/needed]: None
Attachment #8713835 -
Attachment is obsolete: false
Attachment #8713835 -
Flags: approval-mozilla-beta?
Attachment #8713835 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9799df240b37
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9799df240b37 Keeping this open for the test.
Comment 22•8 years ago
|
||
Andrei, can someone on your team verify this fix on m-c? Sylvestre, maybe we can uplift this to aurora and beta on Monday in time to make the beta 6 build.
Flags: qe-verify+
Flags: needinfo?(sledru)
Updated•8 years ago
|
Flags: needinfo?(andrei.vaida)
Updated•8 years ago
|
Flags: needinfo?(andrei.vaida)
Comment 24•8 years ago
|
||
Ugh, it seems the ni? flag was automatically removed when I CC'ed Paul here, who's going to have a look at this fix.
QA Contact: paul.silaghi
Comment 25•8 years ago
|
||
I reproduced the problem on Nightly 2016-01-19 Win 7, using the attached extension. Verified fixed on 47.0a1 (2016-02-14). I also installed top most popular addons from https://addons.mozilla.org/en-US/firefox/extensions/?sort=users and no problems found.
Comment 26•8 years ago
|
||
Comment on attachment 8713835 [details] [diff] [review] patch Fix verified on m-c, let's uplift this to aurora and beta.
Attachment #8713835 -
Flags: approval-mozilla-beta?
Attachment #8713835 -
Flags: approval-mozilla-beta+
Attachment #8713835 -
Flags: approval-mozilla-aurora?
Attachment #8713835 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 27•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/28f78de5ee6b https://hg.mozilla.org/releases/mozilla-beta/rev/60c12abdabe1
Comment 28•8 years ago
|
||
Looks like we can close this bug now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 29•8 years ago
|
||
We still have a test to land here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•8 years ago
|
Whiteboard: sec-moderate in 44 (pref flip available), higher value bypass in later versions. → [adv-main45-] sec-moderate in 44 (pref flip available), higher value bypass in later versions.
Assignee | ||
Comment 31•8 years ago
|
||
This is now in the current 45 release, can I go ahead and land the test?
Flags: needinfo?(abillings)
Comment 33•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2fbf81b363ef
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Group: toolkit-core-security → core-security-release
Comment 35•8 years ago
|
||
Verified fixed FX 46b10, 48.0a1 (2016-04-12) Win 7
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•