Closed Bug 1440775 (CVE-2018-5131) Opened 2 years ago Closed 2 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 59+ fixed
firefox58 --- wontfix
firefox59 + fixed
firefox60 + fixed

People

(Reporter: bkelly, Assigned: bkelly)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main59+][adv-esr52.7+])

Attachments

(2 files, 1 obsolete file)

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.
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?
Flags: needinfo?(honzab.moz)
Boris tested safari and it also has the same before as edge/chrome.
Note that this is the fetch() equivalent of bug 1171853 but without view-source involved.
(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.
Flags: needinfo?(honzab.moz)
(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`?
Flags: needinfo?(honzab.moz)
(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?
Flags: needinfo?(honzab.moz) → needinfo?(bkelly)
(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.
Flags: needinfo?(bkelly)
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.
(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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Keywords: sec-moderate
Attached patch wpt.patchSplinter Review
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.
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.
Attachment #8954863 - Attachment is obsolete: true
Attachment #8954884 - Flags: review?(honzab.moz)
Group: core-security → dom-core-security
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
Attachment #8954884 - Flags: review?(honzab.moz) → review+
(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 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
Attachment #8954884 - Flags: approval-mozilla-beta?
Or maybe comment 16 should have been approval-mozilla-release? flag instead?  I was trying to get this in to FF59.
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.
Attachment #8954884 - Flags: approval-mozilla-release?
https://hg.mozilla.org/mozilla-central/rev/60fb09de57ec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Attachment #8954884 - Flags: approval-mozilla-release?
Attachment #8954884 - Flags: approval-mozilla-release+
Attachment #8954884 - Flags: approval-mozilla-beta?
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.
Attachment #8954884 - Flags: approval-mozilla-esr52?
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.
Attachment #8954884 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main59+][adv-esr52.7+]
Alias: CVE-2018-5131
Group: dom-core-security → core-security-release
I'm going to open the upstream WPT test case pull request for this.
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.