Closed Bug 1285476 Opened 8 years ago Closed 7 years ago

AppCache can be used to bypass CORS

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: annevk, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

See https://github.com/whatwg/html/issues/1427 for details and a potential solution (double caching).

This is a security issue, but it's made public already.
Assignee: nobody → honzab.moz
Whiteboard: [necko-active]
See Also: → 687758
The test case confirms we suffer from the issue.  On reload of the evil site (from [1]) we load the anonymous xhr request from appcache.  Appcache doesn't recognize the 'anonymous' flag.  It means we serve non-anonymously loaded content to anonymous requests.  That is bad.

However, there used to be an issue from times we were honoring the anonymity flag - bug 687758.  Honoring it will break some sites again, but I personally don't care at all.  Appcache is a dead technology.

[1] https://github.com/jakearchibald/appcache-credentials
Status: NEW → ASSIGNED
Attachment #8775149 - Flags: review?(michal.novotny)
Note that an alternative that should not break sites is double requests and double caching for each entry in AppCache. Not ideal obviously.
(In reply to Anne (:annevk) from comment #4)
> Note that an alternative that should not break sites is double requests and
> double caching for each entry in AppCache. Not ideal obviously.

I know.  But I don't want to allocate resources for that right now.  OTOH, I'm spending recently quite a lot of time on appcache already, so that I may fix this along...
Mutually exclusive with the v1 patch.  This is a roughly manually tested stuff.
Attachment #8775295 - Flags: review?(michal.novotny)
Comment on attachment 8775149 [details] [diff] [review]
v1 (hard-forbid serving from appcache to anon requests)

Doesn't make sense to deal with this patch.
Attachment #8775149 - Attachment is obsolete: true
Attachment #8775149 - Flags: review?(michal.novotny)
Comment on attachment 8775295 [details] [diff] [review]
v2 (double caching)

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

Michal, feel free to pass to Jason, he has some experience with the uriloader parts.
Comment on attachment 8775295 [details] [diff] [review]
v2 (double caching)

Jason will be better reviewer for this.
Attachment #8775295 - Flags: review?(michal.novotny) → review?(jduell.mcbugs)
Comment on attachment 8775295 [details] [diff] [review]
v2 (double caching)

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

> This is a roughly manually tested stuff.

Patch makes sense to me.  Can we test it any better manually before we ship it?
Attachment #8775295 - Flags: review?(jduell.mcbugs) → review+
(In reply to Jason Duell [:jduell] (needinfo me) from comment #10)
> Comment on attachment 8775295 [details] [diff] [review]
> v2 (double caching)
> 
> Review of attachment 8775295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > This is a roughly manually tested stuff.
> 
> Patch makes sense to me.  Can we test it any better manually before we ship
> it?

Do you want me to block on this before do more testing here?  I did of course a check this does what's expected and anon resources load and also that the test case from https://github.com/jakearchibald/appcache-credentials no longer show the symptoms.

IMHO, this enough to go.
Flags: needinfo?(jduell.mcbugs)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e56fc8ee868
Let appcache double-cache resources that are cross origin, r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e56fc8ee868
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
> IMHO, this enough to go.

Yes, that's fine
Flags: needinfo?(jduell.mcbugs)
Depends on: 1619673
You need to log in before you can comment on or make changes to this bug.