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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox44 --- wontfix
firefox45 + fixed
firefox46 + verified
firefox47 + verified
firefox48 --- verified
firefox-esr38 --- disabled

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)

Attached file PoC
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.
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
Attached patch patchSplinter Review
Attachment #8713835 - Flags: review?(rhelmer)
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+
(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...
(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.
This patch prevents the PoC for me locally - unsigned side-loaded add-on is disabled as expected.
Should we have a unit test here too?
Flags: needinfo?(dtownsend)
(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)
(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.
Group: firefox-core-security → toolkit-core-security
Flags: needinfo?(dveditz)
Not sure Dan meant to drop the needinfo there...
Flags: needinfo?(dveditz)
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?
Flags: needinfo?(dveditz) → in-testsuite?
Keywords: sec-high
Whiteboard: sec-moderate in 44 (pref flip available), higher value bypass in later versions.
Attached patch patchSplinter Review
Updated with a test
Attachment #8713835 - Attachment is obsolete: true
Attachment #8715059 - Flags: review?(rhelmer)
Attachment #8715059 - Flags: review?(rhelmer) → review+
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?
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)
(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 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+
(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)
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)
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?
 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)
Flags: needinfo?(andrei.vaida)
Sure, I will approve it once verified
Flags: needinfo?(sledru)
Flags: needinfo?(andrei.vaida)
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
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 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+
Looks like we can close this bug now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
We still have a test to land here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
This is now in the current 45 release, can I go ahead and land the test?
Flags: needinfo?(abillings)
Yes, go ahead and land.
Flags: needinfo?(abillings)
https://hg.mozilla.org/mozilla-central/rev/2fbf81b363ef
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: toolkit-core-security → core-security-release
Depends on: 1259854
Verified fixed FX 46b10, 48.0a1 (2016-04-12) Win 7
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.