1440775 - (CVE-2018-5131) fetch() force-cache mode allows reading responses with cache-control:no-store and pragma:no-cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

STR:

1. Open nightly
2. Visit https://http-cache-wanderview.glitch.me/
3. Open network monitor and refresh to get it monitoring
4. Execute in console:

fetch('dontcache.txt', { mode: 'same-origin', cache: 'default' });
fetch('dontcache.txt', { mode: 'same-origin', cache: 'default' })
fetch('dontcache.txt', { mode: 'same-origin', cache: 'force-cache' })
fetch('dontcache.txt', { mode: 'same-origin', cache: 'only-if-cached' })

5. Go back to network monitor and check whether each request went to network or pulled from cache.

In nightly we go to network for the first two requests, but pull from cache for the second two.

Both edge and chrome go to network for the first three requests and fail the only-if-cached request.

This is a security issue because it could allow someone on a public lab computer to extract logged in page data from a previous user of the machine.

It seems we should not be able to see no-store/no-cache responses using fetch() force-cache/only-if-cached values.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

You can see how we map the fetch API values to our load flags here:

https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/netwerk/protocol/http/HttpBaseChannel.cpp#2885-2928

We are using LOAD_FROM_CACHE for both force-cache and only-if-cached.  Is there something else we can use to prefer the cache, but not access no-store/no-cache values?  Any ideas Honza?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Boris tested safari and it also has the same before as edge/chrome.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that this is the fetch() equivalent of bug 1171853 but without view-source involved.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Note that this is the fetch() equivalent of bug 1171853 but without
> view-source involved.

Yes, it is, but that bug is more general.

For sake of this bug we could use VALIDATE_NEVER instead of LOAD_FROM_CACHE for the force-cache case.  It will use a cached content only when no-store is NOT present in cache-control.  Otherwise will revalidate with the server.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> For sake of this bug we could use VALIDATE_NEVER instead of LOAD_FROM_CACHE
> for the force-cache case.  It will use a cached content only when no-store
> is NOT present in cache-control.  Otherwise will revalidate with the server.

Ok, but what about for only-if-cached case?  Would that be `VALIDATE_NEVER | LOAD_ONLY_FROM_CACHE`?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #5)
> (In reply to Honza Bambas (:mayhemer) from comment #4)
> > For sake of this bug we could use VALIDATE_NEVER instead of LOAD_FROM_CACHE
> > for the force-cache case.  It will use a cached content only when no-store
> > is NOT present in cache-control.  Otherwise will revalidate with the server.
> 
> Ok, but what about for only-if-cached case?  Would that be `VALIDATE_NEVER |
> LOAD_ONLY_FROM_CACHE`?

Should only-if-cached fail for no-store responses?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> (In reply to Ben Kelly [:bkelly] from comment #5)
> > Ok, but what about for only-if-cached case?  Would that be `VALIDATE_NEVER |
> > LOAD_ONLY_FROM_CACHE`?
> 
> Should only-if-cached fail for no-store responses?

I believe so, yes.  All other browsers implement it that way.

Comment 8

[Honza Bambas (:mayhemer)]

Then the `VALIDATE_NEVER | LOAD_ONLY_FROM_CACHE` will likely work.  If you are about to write a test (which I assume would be appropriate) you can then quickly verify.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> Then the `VALIDATE_NEVER | LOAD_ONLY_FROM_CACHE` will likely work.  If you
> are about to write a test (which I assume would be appropriate) you can then
> quickly verify.

I tested this with comment 0 steps and it works!  I will write a test.

I'm going to mark this sec-moderate.  It can reveal information about a previous user sharing the same profile, but is not an issue for a single user browsing the web.  The severity rating table suggests this should be sec-low, but since it includes access to potentially sensitive credential data (logged in bank page, etc), I'm moving it up to moderate.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemmer

https://treeherder.mozilla.org/#/jobs?repo=try&revision=080316685d0826b54b0e68e37c50cabd713a11a1

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wpt.patch

Here are my test changes, but I think I will land them separately via an upstream pull request.  I want to ask Youenn for review since he originally wrote the tests.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

Honza, this patch switches us to use VALIDATE_NEVER instead of LOAD_FROM_CACHE as you suggested.  I verified locally that this works as expected.

I have WPT tests written as well, but I intend to land them separately after getting upstream review of them.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8954884 [details] [diff] [review]
Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

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

thanks!

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2907,5 @@
>      // no-cache means always validate what's in the cache.
>      SetCacheFlags(mLoadFlags, VALIDATE_ALWAYS);
>      break;
>    case nsIHttpChannelInternal::FETCH_CACHE_MODE_FORCE_CACHE:
>      // force-cache means don't validate unless if the response would vary.

// or is no-store

@@ +2916,5 @@
>      // a network error if the document was't in the cache.
>      // The privacy implications of these flags (making it fast/easy to check if
>      // the user has things in their cache without any network traffic side
>      // effects) are addressed in the Request constructor which enforces/requires
>      // same-origin request mode.

// no-store responses are considered as non-existing in the cache and will fail with this fetch cache mode

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> // or is no-store

I'd like to leave the no-store comments out of the patch for now in the interest of not making the security flaw obvious on landing.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60fb09de57ec145923da102f856399d3323f632b

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954884 [details] [diff] [review]
Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1120715
[User impact if declined]: Public machines shared by separate users could expose information from logged out websites.  So if user A checks their bank account and logs out, then user B later using the same machine may be able to use this bug to see some of their bank account information.
[Is this code covered by automated tests?]: Yes, we have many WPT and mochitests for this feature.  I have a new test case to catch this problem, but I will be landing it separately.
[Has the fix been verified in Nightly?]: Not yet.  Its only been pushed to inbound so far, but I wanted to flag a bit early since we are getting close to the next merge date.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: The patch simply changes the code from using one necko load flag to another one that already exists in the tree.  Its also a contained, easily testable feature.  Our test coverage gives us confidence it does not regress the feature.
[String changes made/needed]: None

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Or maybe comment 16 should have been approval-mozilla-release? flag instead?  I was trying to get this in to FF59.

Comment 18

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

Comment on attachment 8954884 [details] [diff] [review]
Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

Yes, for 59 this would be m-r since the merge happened today.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/60fb09de57ec

Comment 20

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

Comment on attachment 8954884 [details] [diff] [review]
Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

A bit last minute for uplift but let's bring this to 59 RC since it looks like a simple fix, has test coverage, and protects user privacy on a shared computer.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8954884 [details] [diff] [review]
Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Public computers (like in a lab or library) can leak logged in information from a previous user to a later user.  For example, a user could try to see details from a previous user's banking session even if the previous user logged out.  I'm asking for uplift to ESR because I feel public computers are more likely to be running ESR in the first place.  I am holding off on landing WPT tests upstream until we can protect as many release channels as possible.
Fix Landed on Version: FF60
Risk to taking this patch (and alternatives if risky): Minimal.  Its flipping a necko flag from one value to another.  We have tests covering the feature to check for regression.
String or UUID changes made by this patch: None

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2bc7a269882 (FIREFOX_59b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/599641c7992d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8954884 [details] [diff] [review]
Make fetch API force-cache and only-if-cached use VALIDATE_NEVER instead of LOAD_FROM_CACHE. r=mayhemer

User impact sounds scary for enterprise environments, indeed. Taking for ESR 52.7.0.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3102fbb97b32

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

I'm going to open the upstream WPT test case pull request for this.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

https://github.com/w3c/web-platform-tests/pull/10094