1663261 - (CVE-2020-26955) Fenix file download request shares private browsing mode cookie

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[Muneaki Nishimura]

Attachment: private_mode_bypass_by_download.gif

Similar to Bug 1657251, file download request in Fenix shares private mode cookie with normal mode.

Steps to reproduce (see also the attached demo)

1. Open http://csrf.jp/2020/a_download.html in private browsing mode
2. Tap "Download" link and start download (then private mode cookie is recorded by the download response)
3. Open http://csrf.jp/2020/a_download.html in normal browsing mode
4. Tap "Download" link again, then the cookie that was stored at 2) is sent through the request

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

NI to Chenxia to get an owner. It is a holiday weekend so this may not get much action until Tuesday Pacific time.

Comment 2

[Chenxia Liu [:liuche]]

Arturo, do you know if we're sending a private mode cookie (?) when downloading?

I guess this means we'd be leaking PB mode to the site, not sure what impact that has though.

Comment 3

[Arturo Mejia [:amejia]]

On ac, we don't manage cookies when downloading, GeckoView owns the cookies. I'm not sure how it manages cookies internally :owlish could help us to clarify. What I'm aware of it's that we are not indicating to the GeckoWebExecutor (the piece that does the download) that we are on a private mode. I can see a flag FETCH_FLAGS_PRIVATE but it's package private.

Another thing is that we are on the middle of moving away from this api see AC #8312. I'm not aware either how cookies are manage with this new api.

Comment 4

[Muneaki Nishimura]

I'm not familiar with the internal architecture but the HTTP request of <a href download> from Fenix has MozacFetch in User-Agent header (see the attached gif image in detail).
It seems the request is sent from the downloads feature of android-component.
https://github.com/mozilla-mobile/android-components/blob/master/components/feature/downloads/README.md

Comment 5

[Arturo Mejia [:amejia]]

(In reply to Muneaki Nishimura from comment #4)

I'm not familiar with the internal architecture but the HTTP request of <a href download> from Fenix has MozacFetch in User-Agent header (see the attached gif image in detail).
It seems the request is sent from the downloads feature of android-component.
https://github.com/mozilla-mobile/android-components/blob/master/components/feature/downloads/README.md

Yes, it's initiated from android-component but the underneath api that it is performing the download it's part of GeckoView see and GeckoWebExecutor.

Comment 6

[[:owlish] 🦉 PST]

Re: cookies. I am not clear what the question is exactly, Arturo, can you clarify? Also, I'd need some explanation on "private mode cookies", as I am not aware what they are. Is that how browsing mode works in Fenix?

Also, and this is a bit unrelated, but since this bug is about cookies, it would be nice to see the actual cookie exchange for the requests (I only see the requests' method, URIs and response status on the attached gif, and I don't see any other information) - like a HAR file, for example. At the moment, it's not clear to me what is going on.

Comment 7

[[:owlish] 🦉 PST]

Oh, I just accidentally scrolled down on the gif and saw the rest of the info in the gif. Apparently, we're talking about user=priv cookie? I am still not clear what that cookie or what sets it.

The way GV API works, the API users are supposed to set the FETCH_FLAGS_ANONYMOUS flag on the download. With that flag, the cookies are not supposed to be passed. Am I understanding the bug correctly that the flag is set but the cookies are still being passed?

Looking at the source code, I see the flag is set of CookiePolicy.OMIT flag is set but I can't trace where CookiePolicy.OMIT is set. Where is it being set, Arturo?

Also, Arturo, does the bug reproduce in the latest GVE? Does it reproduce in GVE at all? And clarification on the question and the browsing mode cookies is still needed :)

Comment 8

[Arturo Mejia [:amejia]]

The issue is that the user is navigating in private session, and as a result of downloading a file, a cookie get written (user=priv). This cookie must be only available on private browsing but it's getting shared with normal sessions. As if the download were performed on a normal session.

Comment 9

[Arturo Mejia [:amejia]]

The way GV API works, the API users are supposed to set the FETCH_FLAGS_ANONYMOUS flag on the download. With that flag, the cookies are not supposed to be passed. Am I understanding the bug correctly that the flag is set but the cookies are still being passed?

Ok, that is the issue, we are not passing this flag, when downloading that is causing the issue. We have to fix that.

Also, Arturo, does the bug reproduce in the latest GVE? Does it reproduce in GVE at all? And clarification on the question and the browsing mode cookies is still needed :)

I'm no sure we have to tests on it. Do you know if with the new GV API is aware of the type of session (private/nomal) where download is performed? Or from ac we have to do something special?

Comment 10

[Arturo Mejia [:amejia]]

Daniel could you please give us a security rating on this bug?

Comment 11

[[:owlish] 🦉 PST]

(In reply to Arturo Mejia from comment #9)

Do you know if with the new GV API is aware of the type of session (private/nomal) where download is performed? Or from ac we have to do something special?

On the new API you wouldn't have to do anything special, it should work automatically. You have to set these WebExecutor flags now because now you have to perform a separate request for the downloads. With the new API, the need in the second request is eliminated, and the info on the browsing mode should already be in the channel by the time the download starts.

Comment 12

[Arturo Mejia [:amejia]]

Thanks for the clarification!

Comment 13

[[:owlish] 🦉 PST]

Oh, Arturo, one clarification: as things like the "Save As" popup menu item will continue to use WebExecutor, those will need the flag still. NI'ing you to make sure you don't miss this info :)

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Arturo Mejia from comment #10)

Daniel could you please give us a security rating on this bug?

I agree with the sec-moderate rating Andrew gave it earlier.

The way GV API works, the API users are supposed to set the FETCH_FLAGS_ANONYMOUS flag on the download. With that flag, the cookies are not supposed to be passed. Am I understanding the bug correctly that the flag is set but the cookies are still being passed?

That doesn't sound right. A download might require a cookie to work and ANONYMOUS would break that. Private Browsing isn't about not having cookies at all (web sites break!), it's about not saving the cookies to disk and not mixing them with your "normal" cookies. There are a lot of things tied up in Private Browsing that you won't be able to emulate from the front end. It's a "mode" that has to be supported in APIs (usually hung off a browsing context) from GeckoView through Android Components to the front end.

Private Browsing is implemented similarly to "Containers" in Gecko [kinda, for some things] so if you have to change APIs keep that in mind because it would be killer to be able to support Containers in a mobile browser.

Comment 15

[[:owlish] 🦉 PST]

(In reply to Daniel Veditz [:dveditz] from comment #14)

That doesn't sound right.

That is exactly why we have just replaced that API with the new one, that doesn't require the second request.

Comment 16

[Arturo Mejia [:amejia]]

Attachment: 0001-Improve-download-flow.patch

Comment 17

[Arturo Mejia [:amejia]]

Thanks!(In reply to [:owlish] 🦉 PST from comment #13)

Oh, Arturo, one clarification: as things like the "Save As" popup menu item will continue to use WebExecutor, those will need the flag still. NI'ing you to make sure you don't miss this info :)

Thanks, It's included as part of the patch.

(In reply to Daniel Veditz [:dveditz] from comment #14)

(In reply to Arturo Mejia from comment #10)

Daniel could you please give us a security rating on this bug?

I agree with the sec-moderate rating Andrew gave it earlier.

Sorry, I overlooked that it was already rated. Thanks for the additional information!

Sebastian, I attached a patch for review. The commit messages is not entirely related to the issue to not reveal the security issue until it's patched.

Comment 18

[Arturo Mejia [:amejia]]

Also, Arturo, does the bug reproduce in the latest GVE? Does it reproduce in GVE at all? And clarification on the question and the browsing mode cookies is still needed :)

:owlish I tested on Geckoview example and wasn't able to simulate the issue.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 9175880 [details] [diff] [review]
0001-Improve-download-flow.patch

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

So we are passing the `private` flag from the session to `DownloadState` so that we can control out fetch flags, which will set `FETCH_FLAGS_ANONYMOUS` for requests on behalf of a private tab when using `GeckoWebExecutor`. That sounds good to me. r+

But I do wonder if we have a related (for a new bug to file) problem here: If I am in a private tab and perform a download request, that we didn't get via the new API (e.g. for "save as", or when retrying a download) then we have no way of linking it to an existing, private `GeckoSession` to make use of the cookies available to that session. Is this something we would want to make possible or should at least discuss further in a separate bug?

::: components/browser/state/src/main/java/mozilla/components/browser/state/state/content/DownloadState.kt
@@ +43,4 @@
>      val skipConfirmation: Boolean = false,
>      val id: String = UUID.randomUUID().toString(),
>      val sessionId: String? = null,
> +    val isPrivate: Boolean = false,

nit: ContentState uses `private`. Maybe let's use the same name here.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

See question above.

Comment 21

[Arturo Mejia [:amejia]]

Thanks, for the review, I will update the patch fixing the nit!

Comment 22

[Arturo Mejia [:amejia]]

Attachment: 0001-Improve-download-flow.patch

New patch, fixing the nit mentioned on comment 19

Comment 23

[[:owlish] 🦉 PST]

(In reply to Sebastian Kaspari (:sebastian; :pocmo) from comment #19)

But I do wonder if we have a related (for a new bug to file) problem here:
If I am in a private tab and perform a download request, that we didn't get
via the new API (e.g. for "save as", or when retrying a download) then we
have no way of linking it to an existing, private GeckoSession to make use
of the cookies available to that session. Is this something we would want to
make possible or should at least discuss further in a separate bug?

I agree this needs further discussion. I will create a bug

Comment 24

[Daniel Veditz [:dveditz]]

        val cookiePolicy = if (download.private) {
            Request.CookiePolicy.OMIT
        } else {
            Request.CookiePolicy.INCLUDE
        }

I don't know the Fenix code, but "OMIT" sounds like no cookies at all rather than "use cookies from the PRIVATE cookie jar". That's certainly better than leaking the non-private cookies! But I'm pretty sure that's not web-compatible with some sites.

Comment 25

[Arturo Mejia [:amejia]]

I agree, we should improve the name, I filed the issue #8430 on AC for it. On ac Request.CookiePolicy.OMIT gets translated to GV as GeckoWebExecutor.FETCH_FLAGS_ANONYMOUS, I think ideally we should be using something like GeckoWebExecutor.FETCH_FLAGS_PRIVATE but we don't have access from AC as it's package private on GV.

Comment 26

[Arturo Mejia [:amejia]]

The patch landed in AC 61.0.0 see.

Comment 27

[Kevin Brosnan [Ex-Mozilla]]

Sorina would the Mobile testing team check to see if this is fixed on Fenix 83. It should be broken on 82 as that has AC 60.

Comment 28

[Oana Horvath [:ohorvath]]

Verified this by comparing Beta 83.0 with RC 82.1.1:
After downloading the test file in private mode there is a "user: priv" cookie set.
Going to normal mode, just by loading the test page on RC 82 you can see that cookie already set, while on Beta 83 the cookie is not showing up on the page request in normal mode.
When you download the file in normal mode (with or without having set cookies in private mode), the cookie created has the same name so it is hard to tell if it's being transferred or is a different one, but I suppose that is expected. @Arturo, can you confirm?
Otherwise, this looks like it is fixed on Fenix 83.0 Beta.

Comment 29

[Arturo Mejia [:amejia]]

When you download the file in normal mode (with or without having set cookies in private mode), the cookie created has the same name so it is hard to tell if it's being transferred or is a different one, but I suppose that is expected. @Arturo, can you confirm? Otherwise, this looks like it is fixed on Fenix 83.0 Beta.

Yes, it could be a bit confusing. As the site is not aware in which mode we are it just writes the cookie when the file is downloaded.
I tested on Fenix 83 Beta and I wasn't able to simulated the issue. If you would like to retest, one way to know that you are starting in clean state is by Deleting the browsing data or Clearing the app storage both should have the same effect as both clean all the data of Fenix.

If you need any additional help just let me know, happy to help :)

Comment 30

[Oana Horvath [:ohorvath]]

Thanks for your answer, I'll mark this as fixed now since it works as expected. Thanks!

Comment 31

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 32

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt