761040 - Offline cache entries are created for no-store entries

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See the testcase test_cacheForOfflineUse_no-store.js added in bug 760955.

These two checks in the test should be changed from "todo" to "do":

  todo_check_eq(status, Cr.NS_ERROR_CACHE_KEY_NOT_FOUND);
  todo_check_null(descriptor);
  ...
  todo_check_eq(buffer, "");

  chan.asyncOpen(new ChannelListener(checkNoStore, chan
                                     /*, TODO: CL_EXPECT_FAILURE*/), null);

Basically, these two statements are checking that, when we attempt to update an offline cache entry and we receive a Cache-Control no-store, that the cache entry isn't created and that the HTTP request should fail.

Currently, the channel successfully loads and an offline cache entry (an invalid one?) is created.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

"With the exception of "no-store" directive, HTTP cache headers and restrictions on caching pages served over TLS (encrypted, using https:) are overridden by manifests."

- http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#introduction-5
- http://www.w3.org/TR/html5/browsers.html#introduction-4

"[I]f the resource is labeled with the "no-store" cache directive, then create a task to fire a simple event that is cancelable named error at the ApplicationCache singleton of the Document for this entry, if there still is one, and append it to task list. The default action of this event must be, if the user agent shows caching progress, the display of some sort of user interface indicating to the user that the user agent failed to save the application for offline use."

- http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#downloading-or-updating-an-application-cache
- http://www.w3.org/TR/html5/browsers.html#downloading-or-updating-an-application-cache

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I think it would a *very* good idea to fix this before we ship B2G v1 because fixing it later will likely break some AppCache-based apps, including possibly our own Marketplace app.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We really must fix this before we fix bug 826309, because this no-store behavior is required for a safe implementation of cross-origin HTTPS offline resources.

Consider two sites: https://attacker.com and https://victim.com.

victim.com serves all its content over HTTPS with Cache-Control: no-store because it is sensitive content that should never be stored on disk. 

attacker.com creates an AppCache manifest that references content on https://victim.com. If our AppCache code were to ignore the no-store directives on the responses from https://victim.com, then we'd be allowing attacker.com to force us to store sensitive content from https://victim.com that we would otherwise not store.

Actually, the same attack is possible for http://victim.com, but it is less serious since http://victom.com is already making the sensitive content available to (passive) MitMs.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Hmm...actualy, the same kind of attack is possible even with the old same-origin-for-HTTPS-cache-manifests restriction; attacker.com would simply have to be non-TLS http://attacker.com instead of https://attacker.com.

Comment 5

[Andrew Overholt [:overholt]]

(In reply to Brian Smith (:bsmith) from comment #4)
> Hmm...actualy, the same kind of attack is possible even with the old
> same-origin-for-HTTPS-cache-manifests restriction; attacker.com would simply
> have to be non-TLS http://attacker.com instead of https://attacker.com.

Does this mean we no longer need to fix this?  If we do fix this post-v1.0.0, is the migration path very painful for others?  How much work will this entail?  Is it a big risk to desktop or Android?

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I think it should be easy to fix this in Gecko (perhaps a 1-5 line code change; AFAICT by comment 0, the test is already written and it just needs s/todo/do/). But, Honza would know better than me. If Honza, Michal, Nick, and Jason all cannot get to it before the end of next week I might be able to get to it late next week.

It is low risk but we probably don't want to uplift it to -aurora or -beta for Firefox for Desktop or Android because we will need to give developers time to check and update their apps.

I think it is worth fixing for basecamp because if we fix it before we ship B2G 1.0, we don't have to worry about breaking apps from our own Marketplace with a post-1.0 change. This is a security problem so it is something we will not be able to avoid fixing in the name of backward compatibility.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1

- just fail InitOfflineCacheEntry on no-store
- added console log

https://tbpl.mozilla.org/?tree=Try&rev=04f707f7cd6c

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #0)
> See the testcase test_cacheForOfflineUse_no-store.js added in bug 760955.
> 
> These two checks in the test should be changed from "todo" to "do":
> 
>   todo_check_eq(status, Cr.NS_ERROR_CACHE_KEY_NOT_FOUND);
>   todo_check_null(descriptor);
>   ...
>   todo_check_eq(buffer, "");

Don't these tests need to be modified too?

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2

- entry is doomed on no-store
- test updated
- error from InitOfflineCacheEntry is returned only on NoStore() and not also when there is no response head at all

Comment 10

[Jason Duell]

Comment on attachment 698928 [details] [diff] [review]
v2

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

+r: if my comment below is a real issue, it's rare enough that we can fix in a followup.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3626,4 @@
>          CloseOfflineCacheEntry();
>  
> +        if (mResponseHead->NoStore()) {
> +            return NS_ERROR_NOT_AVAILABLE;

We ignore the return value from InitOfflineCacheEntry() function here:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1354

I was worried this would mean that we wouldn't cancel the update for this case (which AFAICT is only if we're storing a failed redirect body in the offline cache: why do we support this in the 1st place?  Who would want part of their app to be a 30X body?).  But I wrote a test for it (patch coming) and it still seems to work anyway, as this codepath isn't hit if the a redirect is canceled ('rv' is NS_FAILED).  Honza, I assume you already thought of all this?

Comment 11

[Jason Duell]

Attachment: addition to test to cover failed redirect case.

Honza: here's the test code I wrote.  

I'm confused as to why we're storing the body of 30x responses in the appcache, and when this actually happens--my test doesn't wind up hitting

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1354

and I'm sure not what would.

Comment 12

[Jason Duell]

As mentioned anything remaining here is followup work

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac464d73d83
https://hg.mozilla.org/releases/mozilla-b2g18/rev/aef3f740329f

Comment 13

[Jason Duell]

jst tells me to mark FIXED bugs that are on inbound and b2g18

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #10)
> We ignore the return value from InitOfflineCacheEntry() function here:
> 
>   
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#1354
> 
> I was worried this would mean that we wouldn't cancel the update for this
> case (which AFAICT is only if we're storing a failed redirect body in the
> offline cache: why do we support this in the 1st place?  Who would want part
> of their app to be a 30X body?).  But I wrote a test for it (patch coming)
> and it still seems to work anyway, as this codepath isn't hit if the a
> redirect is canceled ('rv' is NS_FAILED).  Honza, I assume you already
> thought of all this?

Yes, offline cache updates always cancel redirects, so we can never hit this code now.  I didn't write this particular piece of code, Dave Camp did.

I think we can remove it safely.  Thanks for the test.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1ac464d73d83

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 698928 [details] [diff] [review]
v2

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

Reason for approval request: This is a security/privacy issue that should be fixed, and the fix is very low risk and easy to uplift.
Testing/risk: It has been on m-c for about a week, which found one bug (bug 829858) which has been fixed.
String/UUID changes: none

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #16)
> Testing/risk: It has been on m-c for about a week, which found one bug (bug
> 829858) which has been fixed.

I should add that this is a standards-conformance issue. Also, there *is* some risk that we might break some existing applications that are relying on AppCache storing no-store content. On the other hand, as Mozilla Marketplace, and especially B2G, depend on AppCache, it is important to make sure that Firefox Desktop & Android become consistent with what B2G will do, well before B2G is released, since devs will be testing their B2G/marketplace apps with Firefox.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 698928 [details] [diff] [review]
v2

Approving for aurora/beta but holding off on ESR - this doesn't meet ESR criteria as is, do you have reason to think this would affect a lot of our enterprise users or can we wait and see if there is feedback on the list from those users before acting here?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5817ef2c4d33

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(Also, this is in my queue to land on Aurora once it reopens)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/689443806e79

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Lukas Blakk [:lsblakk] from comment #18)
> Approving for aurora/beta but holding off on ESR - this doesn't meet ESR
> criteria as is, do you have reason to think this would affect a lot of our
> enterprise users or can we wait and see if there is feedback on the list
> from those users before acting here?

It is a security issue that I don't think we should live with for 1 year or more when the fix is so very, very simple. I don't want Firefox 17 ESR's mis-handling of this case to push people to avoid AppCache as they develop apps, especially apps for B2G. AppCache has 99 problems but this shouldn't be one.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 698928 [details] [diff] [review]
v2

Approving for esr17 in that case, we'll be updating esr10 users to esr17 on this next release and there's no reason to encourage lingering on esr10.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/7a7bb804aeda

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 698928 [details] [diff] [review]
v2

Backed out for build bustage.
https://hg.mozilla.org/releases/mozilla-esr17/rev/cf989dcd7867

https://tbpl.mozilla.org/php/getParsedLog.php?id=19068162&tree=Mozilla-Esr17

HttpChannelChild.cpp
/builds/slave/m-esr17-lnx/build/netwerk/protocol/http/nsHttpChannel.cpp: In member function 'nsresult mozilla::net::nsHttpChannel::InitOfflineCacheEntry()':
/builds/slave/m-esr17-lnx/build/netwerk/protocol/http/nsHttpChannel.cpp:3663:33: error: 'class nsICacheEntryDescriptor' has no member named 'AsyncDoom'

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 698928 [details] [diff] [review]
v2

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3620,5 @@
>  
>      if (!mResponseHead || mResponseHead->NoStore()) {
> +        if (mResponseHead->NoStore()) {
> +            mOfflineCacheEntry->AsyncDoom(nullptr);
> +        }

I guess this should be a rare case (it's a spec violation anyway), so it should be OK to change the call from AsyncDoom() to Doom() for ESR 17.

Honza, Michal: do you agree?

Comment 27

[Honza Bambas (:mayhemer)]

We actually don't need to doom at all.  This will lead to update error that will completely discard all downloaded data.  So I think omitting the line completely for ESR17 is ok.

Comment 28

[Manuela Muntean [Away]]

Marking this as [qa-] since there is an automated test that verifies it. Please remove the tag and and "verifyme" keyword if QA help is needed.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Test landed on inbound. I'll uplift it to the branches when it goes green.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08863162779f

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 699799 [details] [diff] [review]
addition to test to cover failed redirect case.

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

r=honzab

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Re-landed on es17 minus the 3 lines from comment #26. a=bajaj IRL.
https://hg.mozilla.org/releases/mozilla-esr17/rev/73131b1549f0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Backed out again for build bustage. Looks like this patch depends on bug 807501 as well.
https://hg.mozilla.org/releases/mozilla-esr17/rev/a886fbd0a66c

https://tbpl.mozilla.org/php/getParsedLog.php?id=19097028&tree=Mozilla-Esr17

/builds/slave/m-esr17-lnx/build/uriloader/prefetch/nsOfflineCacheUpdate.cpp: In member function 'virtual nsresult nsOfflineCacheUpdateItem::OnStopRequest(nsIRequest*, nsISupports*, nsresult)':
/builds/slave/m-esr17-lnx/build/uriloader/prefetch/nsOfflineCacheUpdate.cpp:431:34: error: 'LogToConsole' was not declared in this scope

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Attachment: esr17 patch

OK, this actually builds locally.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #29)
> Test landed on inbound. I'll uplift it to the branches when it goes green.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/08863162779f

Backed out for crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a14253dc0a

https://tbpl.mozilla.org/php/getParsedLog.php?id=19098342&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | test failed (with xpcshell return code: 1), see following log:
Assertion failure: shared->activeUseCount == 0, at ../../../js/src/vm/RegExpObject.cpp:657
PROCESS-CRASH | /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | application crashed [@ js::RegExpCompartment::~RegExpCompartment()]

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 705999 [details] [diff] [review]
esr17 patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3660,5 @@
>  
>      if (!mResponseHead || mResponseHead->NoStore()) {
>          CloseOfflineCacheEntry();
>  
> +        if (mResponseHead->NoStore()) {

if (mResponseHead && mResponseHead->NoStore())

Make it one commit please...

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Rolled up with bug 829858.
https://hg.mozilla.org/releases/mozilla-esr17/rev/6523fbbd93e5

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures. Sorry Honza, I think it's best for you to sort this out at this point.
https://hg.mozilla.org/releases/mozilla-esr17/rev/6523fbbd93e5

https://tbpl.mozilla.org/php/getParsedLog.php?id=19103976&tree=Mozilla-Esr17

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | 0 == 2152398909 - See following stack:

Comment 38

[Honza Bambas (:mayhemer)]

We have to doom.  I've added asyncDoom since it is not sure the entry will be removed by some upper level logic (like offline cache update).  Usage of the channel the way in the test shows the channel has to be responsible for removing the entry from the cache.

I'll try how Doom() call is going to behave.

Michal, what is your opinion on using Doom() method of cache entry on the main thread?

Comment 39

[Michal Novotny [:michal]]

If I understand correctly you want to use Doom() on the main thread only in Esr17 branch, right? If so I'm OK with it since we are already doing it at other places in this tree.

Comment 40

[Honza Bambas (:mayhemer)]

Attachment: esr17 patch

https://hg.mozilla.org/releases/mozilla-esr17/rev/7682348c875d