1528335 - (CVE-2019-11723) `InstallTrigger` and `mozAddonManager` leaking cookies in private browsing mode

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

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

In private browsing mode, websites that call InstallTrigger to trigger the install of a download leak cookies that were set outside of private browsing mode in the request to fetch the add-on file.

The same counts for AMO, that is using mozAddonManager: When installing an add-on from AMO in private browsing mode, the non-private browsing cookies from AMO are included in the request to fetch the file.

exemplary STR for AMO:

1. Open AMO in non-private browsing mode
2. Open the devtools and make note of the "sessionid" cookie for requests.
3. Clear your cache (important!)
4. Open AMO in private browsing mode.
5. In the web console, execute:

navigator.mozAddonManager.createInstall({ url: "https://addons.mozilla.org/firefox/downloads/file/1672871/ublock_origin-1.18.4-an+fx.xpi", hash: "sha256:b7aa57bd91943a2d3dd3621872e8700b2f59665db5c92fc54bca5049780cc91c" }).then((install) => { install.install(); })

(This is pretty much what clicking on the "Add to Firefox" button would do).

6. In the network panel, inspect the request to get that file and check the cookies. The "sessionid" cookie should be identical to the one in (2).

Comment 1

[:Gijs (he/him)]

Is this a regression? I can confirm beta is affected; have you tried release?

Comment 2

[:Gijs (he/him)]

Attachment: Tentative patch ideas

I vaguely recall we tried to do something like this before, but we didn't go through with it. I suspect it's because we didn't serialize principals sent from JS at the time (we do now, bug 1515863 fixed in 66 and later), though I could be wrong. Anyway, this code seems to work for me (only tried with the example in comment #0, haven't tried InstallTrigger, haven't run any tests, etc.).

Dumb idea? Am I missing anything?

(Christoph, do these flags for the actual channel look approximately right?)

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9044524 [details] [diff] [review]
Tentative patch ideas

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

Yeah, the approach you take and the flags you send seem correct to me - go for it!

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 9044524 [details] [diff] [review]
Tentative patch ideas

Not seeing any obvious problem, but I'm a bit outdated on this code, kmag is probably more up to date.

Comment 5

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

(In reply to :Gijs (he/him) from comment #1)

Is this a regression? I can confirm beta is affected; have you tried release?

I can reproduce this in 65.0.1.

Comment 6

[Daniel Veditz [:dveditz]]

Not too worried about mozAddonManager since it's only available to our own sites, but this means InstallTrigger() could be used as a sneaky tracking mechanism. Would be noisy though since by default users will get a prompt that a site it trying an install before we send anything.

Comment 7

[:Gijs (he/him)]

There's no way I'm getting to polishing this up, writing tests, getting review, and uplift to 66, in the next week or so. Too much other stuff. Marking wontfix for 66 given that, and the fact that this is sec-low.

Comment 8

[:Gijs (he/him)]

I expect this has just always been broken...

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1528335 - tests, r?aswan

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1528335, r?aswan

Depends on D25774

Comment 11

[:Gijs (he/him)]

Landed as:

https://hg.mozilla.org/integration/autoland/rev/acce10271d62
https://hg.mozilla.org/integration/autoland/rev/fd9468269591

backed out as

https://hg.mozilla.org/integration/autoland/rev/182738789fa9

"for test_ext_privacy_update.js failures CLOSED TREE"

Comment 12

[:Gijs (he/him)]

Turns out AddonManager.getInstallForURL / XPIInstall.getInstallForURL is used a lot, and requiring principals there doesn't seem like a good fix... poking at this some more.

Comment 13

[:Gijs (he/him)]

Andrew figured out the leaks \o/

bug 1541577

Landing this patchset with the test disabled on debug for now:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c032b60a872e3946caa7308f120b91ed477103b9

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/ee208901e86a56aedd617948d652b1134a72824c
https://hg.mozilla.org/integration/autoland/rev/c032b60a872e3946caa7308f120b91ed477103b9
https://hg.mozilla.org/mozilla-central/rev/ee208901e86a
https://hg.mozilla.org/mozilla-central/rev/c032b60a872e

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Gijs, since 67 is affected, do you want to uplift this fix to Beta67? Thanks!

Comment 16

[:Gijs (he/him)]

I'm not 100% sure about uplifting... Andrew, thoughts?

Comment 17

[Andrew Swan [:aswan]]

Its a tough call. I trust the patch and we have pretty thorough existing tests to catch other regressions it might cause. But the patch touches a decent amount of code and this is a sec-low which makes me lean toward "no".

Comment 18

[:Gijs (he/him)]

Alright, let's call it.

Comment 19

[Hani Yacoub, Desktop QA]

Could you please provide more details from where exactly I should get the "sessionid" from?

I tried to compare the "amo-request-id" from headers tab and the "id" from Cookies tab but there was no match, but I might be looking in the wrong direction.

Thanks.

Comment 20

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

The sessionid cookie is set by addons.mozilla.org (AMO). Just browse any AMO page.

Comment 21

[Hani Yacoub, Desktop QA]

Attachment: sessionid.png

I didn't find any cookies of the requests in the private browsing mode.

After executing the script in the web console using private mode, I didn't find the request of adding "Ublock" in network panel, I even went through all the request and I the cookies panel was empty for all the request.

I'm not sure what I'm not doing wrong. I attached a screenshot of the cookies section for non-private browsing mode.

Comment 22

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

Can you check the request for the page?

Comment 23

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

Also, you might have to log in into AMO.

Comment 24

[:Gijs (he/him)]

Hani: for checking the request for ublock, you probably want the network panel in the browser toolbox, not the "normal" developer tools. See https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox .

Comment 25

[Hani Yacoub, Desktop QA]

Attachment: private mode sessionid.png

In the screenshot attached is displayed the only "sessionid" displayed in private browsing mode and the sessionid is not matching the sessionid when using the non-private mode. Cookies panel for Ublock request is displayed empty.

Comment 26

[:Gijs (he/him)]

Andreas, can you verify this please, as you reported? Seems the simplest. Thanks.

Comment 27

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

Tested and verified on Nightly (20190515092556).