848344 - Media cache should be cleared when Recent History or HTTP cache cleared.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Anonymous]

Under certain conditions, the Firefox cache is not fully cleared when requested by the user. This has minor security implications as it provides a means to track a client across connections despite them supposedly having cleared their entire cache state.

Steps to reproduce:

1. Create a file with following HTML:

<audio autoplay>
<source type="audio/ogg" src="https://www.gstatic.com/youtube/media/harlem-shake.ogg">
</audio>

2. When the audio plays, the URL appears in ABP's blockable items list, in the LiveHTTPHeaders sidebar, in the Firefox Web Console, can be handled by proxy plugins, and can be observed by network traffic tools. Good.

3. Use Tools -> Clear Recent History. Select "Everything" as the time range and check all boxes.

4. Refresh the HTML file, possibly even using Ctrl+F5.

5. The URL does not appear in ABP's blockable items list, and is not observed in any of the other places. No traffic is sent over the network. about:cache states that the cache is totally empty. The audio is mysteriously playing anyway.

Comment 1

[Josh Matthews [:jdm]]

The OGG is likely being cached in the media cache, which is a separate beast from the regular cache. Perhaps we should hook up clearing it to the regular cache clearing as well.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should. I don't think it's a security issue though ... private browsing does handle this.

Comment 3

[Josh Matthews [:jdm]]

The cache clearing code is invoked from here:

http://hg.mozilla.org/mozilla-central/annotate/ee4879719f78/browser/components/preferences/advanced.js#l339

We should add an observer notification that the media cache watches for (similar to the existing last-pb-context-exited) and invoke it:

http://hg.mozilla.org/mozilla-central/annotate/ee4879719f78/content/media/MediaCache.cpp#l347

Comment 4

[Jason Duell]

Let's make sure we handle both "Clear Recent History" and manually clearing HTTP cache (UI in Prefs | advanced | network).  I.e. make sure they both travel through the codepath in comment 3.  I suspect Clear Recent History may be a different codepath.

Comment 5

[Josh Matthews [:jdm]]

I suppose the other option is to fire the notification from the cache-clearing code in netwerk/cache/ so that this affects any app/caller (Seamonkey, for example) that clears the cache. That's probably a better choice.

Comment 6

[Jason Duell]

roc:

If a user clears Prefs | advanced | network  |"Offline Web Content and Data", i.e we hit

   http://hg.mozilla.org/mozilla-central/annotate/ee4879719f78/browser/components/preferences/advanced.js#l352

should we also clear media cache?  Media resources can be in appcache data, I assume?  Or is that overkill?  I'm not clear on whether we're likely to see resources from app cache in the media cache (and how much we would care about clearing them, as the very minor privacy leak in the HTTP case presumably doesn't exist in the app cache case).

Comment 7

[Jason Duell]

> That's probably a better choice.

+1

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jason Duell (:jduell) from comment #6)
> If a user clears Prefs | advanced | network  |"Offline Web Content and
> Data", i.e we hit
> 
>   
> http://hg.mozilla.org/mozilla-central/annotate/ee4879719f78/browser/
> components/preferences/advanced.js#l352
> 
> should we also clear media cache?  Media resources can be in appcache data,
> I assume?  Or is that overkill?  I'm not clear on whether we're likely to
> see resources from app cache in the media cache (and how much we would care
> about clearing them, as the very minor privacy leak in the HTTP case
> presumably doesn't exist in the app cache case).

That sounds like overkill. The media cache is transient to the session anyway so the only reason to clear it is for privacy.

Comment 9

[Kalle]

Same bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=850891

If you fix this bug please also add reloading of media if no-cache flag is set for the media or if the cache has timed out for the media.

Comment 10

[Srinath N [:srinath]]

Attachment: Bug 848344 - clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE

Comment 11

[Srinath N [:srinath]]

Attachment: Network Request after applying the patch

Comment 12

[Srinath N [:srinath]]

Attachment: Network Request before applying the patch

Comment 13

[Srinath N [:srinath]]

Flagging as per Josh's request

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 745027 [details] [diff] [review]
Bug 848344 - clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE

Sorry for the delay here. You'll want to clean up the indentation in EvictEntries, since that's using tabs instead of spaces. Flagging Jason and Robert for respect netwerk and content/media changes.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 745027 [details] [diff] [review]
Bug 848344 - clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE

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

::: content/media/nsMediaCache.cpp
@@ +347,5 @@
>    if (strcmp(aTopic, "last-pb-context-exited") == 0) {
>      nsMediaCache::Flush();
>    }
> +  if (strcmp(aTopic, "network-clear-cache-stored-anywhere") == 0) {
> +      nsMediaCache::Flush();

fix indent

::: netwerk/cache/nsCacheService.cpp
@@ +1517,5 @@
> +		{
> +			obsvc->NotifyObservers(nullptr, "network-clear-cache-stored-anywhere", nullptr);
> +		}
> +
> +	}

Fix indent

Comment 16

[Jason Duell]

Comment on attachment 745027 [details] [diff] [review]
Bug 848344 - clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE

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

Looks fine but I want to run it by Michal to make this is the only call site that needs changing.

::: netwerk/cache/nsCacheService.cpp
@@ +1509,5 @@
>  
>  
>  NS_IMETHODIMP nsCacheService::EvictEntries(nsCacheStoragePolicy storagePolicy)
>  {
> +	if(storagePolicy == nsICache::STORE_ANYWHERE)

Indenting in this file should be 4 spaces per indent.  Looks like you're using 8. Please fix.

Also, put a space in between if and (, i.e. "if (".

@@ +1512,5 @@
>  {
> +	if(storagePolicy == nsICache::STORE_ANYWHERE)
> +	{
> +		nsCOMPtr<nsIObserverService> obsvc = mozilla::services::GetObserverService();
> +		if(obsvc)

also "if ("

@@ +1514,5 @@
> +	{
> +		nsCOMPtr<nsIObserverService> obsvc = mozilla::services::GetObserverService();
> +		if(obsvc)
> +		{
> +			obsvc->NotifyObservers(nullptr, "network-clear-cache-stored-anywhere", nullptr);

if line is still longer than 80 chars after you fix indenting, break it into two lines.

Comment 17

[Michal Novotny [:michal]]

Comment on attachment 745027 [details] [diff] [review]
Bug 848344 - clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE

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

::: netwerk/cache/nsCacheService.cpp
@@ +1514,5 @@
> +	{
> +		nsCOMPtr<nsIObserverService> obsvc = mozilla::services::GetObserverService();
> +		if(obsvc)
> +		{
> +			obsvc->NotifyObservers(nullptr, "network-clear-cache-stored-anywhere", nullptr);

nsCacheService::EvictEntries() can be called on any thread but nsObserverService::NotifyObservers() can be called only on main thread. AFAICS we don't call it off the main thread but some addon can do this...

Comment 18

[Jason Duell]

> nsCacheService::EvictEntries() can be called on any thread but 
> nsObserverService::NotifyObservers() can be called only on main thread.

OK, so then I assume the right fix here is to check NS_IsMainThread() and if not, dispatch an event to the main thread to fire off the observer event: ie. something like

  NS_DispatchToMainThread(
    NS_NewRunnableMethod(this, &nsCacheService:FireCacheStoredAnywhereNotification));

is that right Michal?

Comment 19

[Michal Novotny [:michal]]

Yes, that should work.

Comment 20

[Srinath N [:srinath]]

Thanks for the comments and info Jason, Michal and roc. I will submit the updated patch shortly.

Comment 21

[Srinath N [:srinath]]

Attachment: Clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE (Addressing review comments)

Comment 22

[Jason Duell]

Comment on attachment 759640 [details] [diff] [review]
Clearing media cache from evictEntries when storage policy is set to STORE_ANYWHERE (Addressing review comments)

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

Michal should have final say here, but other than a couple nits this looks ready to me.

::: netwerk/cache/nsCacheService.cpp
@@ +1521,5 @@
>  
>  NS_IMETHODIMP nsCacheService::EvictEntries(nsCacheStoragePolicy storagePolicy)
>  {
> +    if (storagePolicy == nsICache::STORE_ANYWHERE) {
> +        //if not called on main thread, dispatch the notification to the main thread to notify observers

nit: put space after '//'

@@ +1526,5 @@
> +        if (!NS_IsMainThread()) { 
> +            nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(this,
> +                                                               &nsCacheService::FireClearNetworkCacheStoredAnywhereNotification);
> +            nsresult rv = NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +            NS_ENSURE_SUCCESS(rv, rv);

I'd remove the NS_ENSURE_SUCCESS: if for some reason we can't dispatch an event to the main thread (which is really very unlikely) I'd rather see us proceed to EvictEntriesForClient than return w/o that.

@@ +1528,5 @@
> +                                                               &nsCacheService::FireClearNetworkCacheStoredAnywhereNotification);
> +            nsresult rv = NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +            NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +        FireClearNetworkCacheStoredAnywhereNotification(); //else you're already on main - notify observers

put comment above line if this is >80 chars (my screen is narrow--can't tell).  Also space after //

Comment 23

[Srinath N [:srinath]]

Attachment: Updating patch based on feedback from jduell

Comment 24

[Jason Duell]

Comment on attachment 760748 [details] [diff] [review]
Updating patch based on feedback from jduell

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

Looks good.  As I said I'd like michal to have official review here.

Comment 25

[Michal Novotny [:michal]]

Comment on attachment 760748 [details] [diff] [review]
Updating patch based on feedback from jduell

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

::: netwerk/cache/nsCacheService.cpp
@@ +1528,5 @@
> +                                                               &nsCacheService::FireClearNetworkCacheStoredAnywhereNotification);
> +            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +        }
> +        // else you're already on main - notify observers
> +        FireClearNetworkCacheStoredAnywhereNotification(); 

"else" mentioned in the comment is missing in the code.

Comment 26

[Srinath N [:srinath]]

Attachment: Addressing michal's review comments

Comment 27

[Josh Matthews [:jdm]]

This got lost in the shuffle, sorry!

https://hg.mozilla.org/integration/mozilla-inbound/rev/433d91a460b3

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/433d91a460b3