1454909 - (CVE-2018-12369) No check for privileged permissions for WebExtension experiments

Status: RESOLVED FIXED

(WebExtensions :: Experiments, defect, P1)

Description

[Jonathan Kingston [:jkt] he/him]

It appears there aren't any checks for bundled web extension experiments.

https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/toolkit/components/extensions/Extension.jsm#632 doesn't check for privileged permissions.

experimentsAllowed appears never to be used:
https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/toolkit/components/extensions/Extension.jsm#1355

I compiled the code with isPrivileged and experimentsAllowed hard coded to return false. I was still able to load an XPI built by web-ext build which was a bundled experiment through about:debugging.

This was able to set preferences etc. Is there something I am missing here?

Comment 1

[Jonathan Kingston [:jkt] he/him]

This appears that https://bugzilla.mozilla.org/show_bug.cgi?id=1454820 wouldn't be needed currently as the code isn't used in the evaluation of if an extension can have an experiment.

If the idea is that an extension is filtered out by AMO then perhaps this is fine too?

Comment 2

[:Gijs (he/him)]

Hopefully Andrew knows what's going on here.

Comment 3

[Jonathan Kingston [:jkt] he/him]

I just submitted an unlisted addon to AMO and got lint errors: "experiments.rollout is not supported" but it did not reject signing on that.

I was able to install this signed XPI to release 59.0.1

Comment 4

[Jonathan Kingston [:jkt] he/him]

* 59.0.2 sorry

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: http_dns-1.0-an+fx.xpi

Example XPI attached.

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

I filed bug 1454939 to deny submissions of webextension experiments on AMO to prevent them from getting signed.

Comment 7

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1454909.patch

This includes the check I had for Bug 1454820 however I couldn't find a way to check the isSystem from this bit of code.

However it seemed like it would be good to bundle the patches together.

Comment 8

[Jonathan Kingston [:jkt] he/him]

I just verified my patch still works as a temp addon in about:debugging but not installing through about:addons. (not that I am even sure if we want users to install an xpi as a temp addon and get this behaviour either)

Comment 9

[Kris Maglione [:kmag]]

Someone should check whether we've signed any extensions with bundled experiments.

Comment 10

[Kris Maglione [:kmag]]

Oh, yay, there's already a patch.

Comment 11

[Jonathan Kingston [:jkt] he/him]

:kmag Andreas suggested that only mine had been signed with code like this.

Comment 12

[Kris Maglione [:kmag]]

OK, then. Please request beta uplift before you land so we can land on both channels at once. And please land this patch as part of the bug to allow experiments in system add-ons. If you can add a stealth test, so much the better.

We can watch for new signatures while that rolls out.

Comment 13

[Johann Hofmann [:johannh]]

Is it possible to land the patch after bug 1454939 is resolved? How long would bug 1454939 take until it is deployed on AMO? The risk of detection would be much lower if AMO had been fixed up already.

Comment 14

[Kris Maglione [:kmag]]

At this point, this only affects nightly and beta users, so there's not too much risk of this being exploited. I'd rather land the fix sooner than wait for AMO. We can always block any experiment extensions if they're signed in the meantime.

Comment 15

[Jonathan Kingston [:jkt] he/him]

:kmag I was able to repro on stable?

Comment 16

[Kris Maglione [:kmag]]

Oh, did it land in 59? That's unfortunate. It might make sense to land the AMO fix first, then, but I'd still lean towards watching and blocklisting. We should be able to block before any add-ons get significant distribution.

Comment 17

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1454909.b.patch

Tests shouldn't be added to sec patches should they?

Comment 18

[Kris Maglione [:kmag]]

(In reply to Jonathan Kingston [:jkt] from comment #17)
> Created attachment 8968909 [details] [diff] [review]
> bug-1454909.b.patch
> 
> Tests shouldn't be added to sec patches should they?

In general they shouldn't, but I want this landed as part of bug 1454820, with tests. We should be able to add tests for all of the cases there without drawing attention to this one.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8968909 [details] [diff] [review]
bug-1454909.b.patch

>diff --git a/toolkit/mozapps/extensions/internal/XPIProvider.jsm b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
>@@ -3873,6 +3873,7 @@ var XPIProvider = {
>         resourceURI: getURIForResourceInFile(aFile, ""),
>         signedState: aAddon.signedState,
>         temporarilyInstalled: installLocation == TemporaryInstallLocation,
>+        systemInstalled: installLocation == BuiltInInstallLocation,

This should probably be something like installLocation.isSystem. Though maybe this makes more sense, since system add-ons in other location shoud be signed, anyway.

Still needs a test. Please submit that for review in bug 1454820. I'd rather not draw atrention to that by reviewing out of band.

Comment 20

[Daniel Veditz [:dveditz]]

Could argue this is sec-high rather than sec-critical because there's a prompt, but you can make an extension look pretty innocuous.

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8968909 [details] [diff] [review]
bug-1454909.b.patch

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

Not 100% certain how Kris wants this patch to interact with the one in bug 1454820. It sounds like a good idea to me to submit all of the code for review in bug 1454820 and just leave this bug without patches.

Comment 22

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1454909.c.patch

I spent a crazy amount of time failing to get any tests to work which I'm not really sure why.
Anyway my plan is to throw this into the Bug 1454820 when we are happy with it to minimize noise in that bug.

Comment 23

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1454909.d.patch

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8970105 [details] [diff] [review]
bug-1454909.d.patch

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

This looks fine to me (and solves the issue in my testing) but I'm not very experienced with that code. I'll leave the final sign-off to Kris.

::: toolkit/components/extensions/test/xpcshell/test_ext_experiments.js
@@ +124,5 @@
> +    {isPrivileged: true, temporarilyInstalled: false, shouldPass: true},
> +    {isPrivileged: false, temporarilyInstalled: true, shouldPass: true},
> +    {isPrivileged: false, temporarilyInstalled: false, shouldPass: false},
> +  ];
> +  for (let i = 0; i < testCases.length; i++) {

nit: could simplify this to for (let testCase of testCases)

@@ +127,5 @@
> +  ];
> +  for (let i = 0; i < testCases.length; i++) {
> +    let testCase = testCases[i];
> +    async function background(shouldPass) {
> +      if (shouldPass) {

nit: shouldPass is a bit weird as a name, maybe something like shouldHaveExperiments?

Comment 25

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1454909.e.patch

Comment 26

[Johann Hofmann [:johannh]]

CC'ing Lizzard to make sure this probable late 60 uplift is noted. Liz, there are plans to include this patch in the one for bug 1454820 (and then uplift together, I suppose).

I would also suggest downgrading this to sec-high since bug 1454939 was resolved and it's no longer a problem on release.

Comment 27

[Kris Maglione [:kmag]]

With bug 1454939 resolved, I think this is only sec-moderate.

Comment 28

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8970141 [details] [diff] [review]
bug-1454909.e.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not very, we now have mitigations in AMO

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No

Which older supported branches are affected by this flaw?

59+

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I plan to uplift, it should cleanly apply to 60.

How likely is this patch to cause regressions; how much testing does it need?

The chance of regressions is low, we have more tests and the only "risk" is with system addons with experiments which currently don't exist.

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8970141 [details] [diff] [review]
bug-1454909.e.patch

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

::: toolkit/components/extensions/test/xpcshell/test_ext_experiments.js
@@ +122,5 @@
> +  let testCases = [
> +    {isPrivileged: true, temporarilyInstalled: true, shouldHaveExperiments: true},
> +    {isPrivileged: true, temporarilyInstalled: false, shouldHaveExperiments: true},
> +    {isPrivileged: false, temporarilyInstalled: true, shouldHaveExperiments: true},
> +    {isPrivileged: false, temporarilyInstalled: false, shouldHaveExperiments: false},

Should have separate cases for system SIGNEDSTATE_SYSTEM and SIGNEDSTATE_PRIVILEGED.

Comment 30

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1454909.f.patch

Comment 31

[Jonathan Kingston [:jkt] he/him]

Still at least a sec-high due to: Bug 1455266

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

jonathan, and aswan, does this resolve the issues mentioned by aswan in https://bugzilla.mozilla.org/show_bug.cgi?id=1454820#c3 ? 

Are you planning to land patches with uplift requests in both this bug and that other bug?  

It's late in beta but we still have the beta 16 build and next week's RC so I'd like to get this into 60.0b16 if Al gives sec approval.

Comment 33

[Andrew Swan [:aswan]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32)
> jonathan, and aswan, does this resolve the issues mentioned by aswan in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1454820#c3 ? 

Yes, the recent patch addresses that issue.

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8970489 [details] [diff] [review]
bug-1454909.f.patch

sec-approval+

Comment 35

[Jonathan Kingston [:jkt] he/him]

My plan was now to move this patch into bug 1454820 and mark as uplift there, not mentioning anything security there in public view.

> does this resolve the issues mentioned by aswan...

Yeah I will override the patch there with this one.

So I will ask for :johannh's approval tomorrow on the other bug with the exact same patch, as he will be in the same room and we have the correct approvals here anyway.

Unless :lizzard it would be better to move this public as close to 60.0b16 as possible?

Comment 36

[Kris Maglione [:kmag]]

Just post the patch to the public bug now. It doesn't do anything to draw more attention it than it already has, and the routes to exploit have been closed by the AMO patches anyway.

Comment 37

[Jonathan Kingston [:jkt] he/him]

Ah I didn't see Bug 1455266 was fixed already :) will do now thanks.

Comment 38

[Kris Maglione [:kmag]]

Just to clarify bug 1454820 comment 12, which I left intentionally vague:

It's no longer possible to exploit this bug in release channels, since AMO will no longer sign extensions with embedded experiments, and release channels won't load unsigned extensions. So I don't think it's necessary to uplift the fix to 60.

That said, if we do plan to ship system add-ons with bundled experiments to 60, doing so without uplifting that patch would look suspicious, so in that case the uplift would probably still make sense.

Updating the severity to reflect the current state of affairs. This is no longer exploitable without an exploit on the AMO side which would cause it to sign these extensions, which makes it sec-moderate.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Is there something to uplift to ESR60 for 60.1 still?

Comment 40

[Jonathan Kingston [:jkt] he/him]

Attachment: rb237414.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This would be a high if it were not for mitigations placed in our AMO code. 
User impact if declined: User may end up with extensions that have permissions to do what the browser can.
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): Low, some addons could break if we haven't signed them correctly (for example ohnoreflow was in this category).
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

This patch was taken from: https://reviewboard.mozilla.org/r/237414/diff/7#index_header this was the landed patch.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8983593 [details] [diff] [review]
rb237414.patch

Fix for possible privilege escalation via malicious extensions. Approved for ESR 60.1.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

This is going to need some rebasing. There's some conflicts with the patch and test failures if those get resolved.
https://treeherder.mozilla.org/logviewer.html#?job_id=182351552&repo=mozilla-esr60

Comment 43

[Jonathan Kingston [:jkt] he/him]

The patch also needed to include: Bug 1457718. It looks like Bug 1467113 has already been applied.

Comment 44

[Jonathan Kingston [:jkt] he/him]

Attachment: 1454909-esr.patch

As mentioned this includes the patch from 1454820 and 1457718 applied for ESR combined.

Sorry for the delay Ryan, I was travelling monday.

Comment 45

[Julien Cristau [:jcristau]]

Thanks Jonathan!

https://hg.mozilla.org/releases/mozilla-esr60/rev/24df9f92227a

Comment 46

[Andrei Vaida [:avaida]]

Please leave this string in place, inside the whiteboard, as it helps QA track what has and hasn't been triaged.