1532486 - Ensure media cache is memory-only when in Private Browsing Mode

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[richard (Tor Project)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0

Comment 1

[richard (Tor Project)]

To avoid disk leakage in Tor Browser, we set the media.cache_size pref to 0 to prevent disk caching of media. However, this resulted in bad video performance or broke video streaming altogether. The attached patch provides a memory-backed cache if it's determined that we are in Private Browsing Mode. See tor issue #29120 for details ( https://trac.torproject.org/projects/tor/ticket/29120 )

Comment 2

[richard (Tor Project)]

Attachment: 1532486_00.patch

Comment 3

[Gary Chen [:xeonchen]]

Attachment: Bug 1532486 - Ensure media cache is memory-only when in Private Browsing Mode

To avoid disk leakage in Tor Browser, we set the media.cache_size pref to 0 to prevent disk caching of media. However, this resulted in bad video performance or broke video streaming altogether. The attached patch provides a memory-backed cache if it's determined that we are in Private Browsing Mode. See tor issue #29120 for details ( https://trac.torproject.org/projects/tor/ticket/29120 )

Comment 4

[Jean-Yves Avenard [:jya]]

why do we need this in the first place?

What vulnerability or privacy concern is the cause?

"To avoid disk leakage in Tor Browser, we set the media.cache_size pref to 0"

When quitting private mode, the media cache is cleared in a secure fashion.

This can't become the default behaviour as it will cause poor performance and increased memory usage when using private browsing

Comment 5

[k114LKCBexSOZhTRhIMa]

"cleared in a secure fashion" is a misnomer when referring to modern physical storage mediums, it's simply not possible due to things like wear leveling and hardware caches. Anything that touches permanent storage should be considered as such, permanent, for any privacy related discussions.

I am the author of this patch (the original submitted for the bug @ Tor trac), there is no increase in memory usage compared to what is possible in Firefox already, it uses the exact same mechanism that already exists to assign a memory-backed cache, and respects the maximum cache size preference. It is possible there would be slightly degraded playback performance in certain circumstances due to the default media.memory_cache_size value, in the Tor patch this was raised to 16384 to allow more space for buffering media.

Comment 6

[k114LKCBexSOZhTRhIMa]

Apologies for the double post, I would like to add more context to my previous comment.

This patch only applies to media played directly in native HTML5 elements (<video>, <audio>). Media that uses something like MSE, which is used by most of popular video sites, is already stored in-memory while using private browsing mode, so this patch would not affect those sites at all.

Things that would be affect by this patch are any sites using native HTML5 elements to play media directly (<video src='...'>), or viewing media links directly in the browser window (https://.../file.mp4).

In very certain situations, this patch could lead to an extra 512mb of RAM being used (the value of media.memory_caches_combined_limit_kb), but that would only be the case if someone had 64 HTML5 videos loaded simultaneously, which I don't think is a realistic situation for the normal end user. In normal usage, it might increase RAM usage a few 10s of megabytes.

Comment 7

[Georg Koppen]

(In reply to Jean-Yves Avenard [:jya] from comment #4)

[snip]

This can't become the default behaviour as it will cause poor performance
and increased memory usage when using private browsing

Could you elaborate those points, in particular taking the last two comments into account?

Comment 8

[Jean-Yves Avenard [:jya]]

" it's simply not possible due to things like wear leveling and hardware caches. Anything that touches permanent storage should be considered as such, permanent, for any privacy related discussions."
At what cost?

I will not take those changes in the gecko tree as-is.
You're free to patch Tor and make this code conditional if you believe the downside of no longer caching properly outweigh the risk of someone scrapping your hardware to determine which blob of binary videos you watched even after randomly stored segments have been deleted.

FWIW, media playback in Tor is fundamentally broken as CanPlayType doesn't report accurate values in order to reduce fingerprinting.

Comment 9

[Georg Koppen]

(In reply to Jean-Yves Avenard [:jya] from comment #8)

" it's simply not possible due to things like wear leveling and hardware
caches. Anything that touches permanent storage should be considered as
such, permanent, for any privacy related discussions."
At what cost?

I will not take those changes in the gecko tree as-is.

I think that's fine. After all we had this bug open to work on a patch we came up with not knowing exactly what would be okay for Mozilla and what not. To understand the position here better I asked if you could elaborate on your points but all we got as an answer was "I will not take those changes in the gecko tree as-is." which is pretty unfortunate in my opinion as it does not help us to further improve our patch. Additionally, I don't see how it follows from "I will not take those changes in the gecko tree as-is." that the bug is automatically a WONTFIX. For instance, what about putting the patch behind a preference instead as we already have a bunch of special preferences to address our needs?

That said you wrote "When quitting private mode, the media cache is cleared in a secure fashion." in comment:4, which is great. However, Tor Browser is never quitting private mode as we are using permanent Private Browsing Mode. I am under the impression that the cache is not deleted in that case which is one of the reasons why we came up with the idea to have a cache in memory. I am happy to be proven wrong here, though.

Comment 10

[richard (Tor Project)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)

why do we need this in the first place?
What vulnerability or privacy concern is the cause?

So when you stream video, the stream is partially saved to disk as it comes in to ensure Firefox has enough data cached to play smoothly while accounting for normal variance in download speed. That's great for performance, but now you have video chunks floating around on your storage device which may persist. If someone has physical access to the storage device, they could determine whether the user had downloaded a particular video by examining the disk and searching for byte sequences from that video.

When quitting private mode, the media cache is cleared in a secure fashion.

In the golden path, this may work perfectly. However, lots of things can happen that interfere with this, even ignoring the comparatively complicated issue of ssds and wear leveling. Firefox could crash, the power could go out, the OS could panic, etc. When stored in volatile memory, the data would disappear in the event of power loss.

Also, if the clearing only happens once a user quits Private Browsing Mode then users with long-lived sessions (such as Tor Browser users, Firefox users using Private Browsing Mode as a separate cookie jar, or users bad at window management) will have these blobs of video persisting on their physical storage drives for potentially quite awhile. The longer the Private Browsing Mode session is active, the higher chance one of the above (or other) problems can occur and prevent the 'secure deleting' path. The safest way to delete something is to never save it to begin with.

This can't become the default behaviour as it will cause poor performance and increased memory usage when using private browsing

As the patch author mentions, this actually brings HTML5 video+audio in line with the already existing behaviour of MSE and friends.

Comment 11

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

Could we put this capability behind a pref? Also, adding some tests would be good.
jya, might that get around your objections to including it in gecko code?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Very unlikely we're going to do anything for ESR60 at this point given where it is in its life cycle.

Comment 13

[Paul Adenot (:padenot)]

Are we going to do something about this? The patch is simple and contained and I don't foresee any maintenance issue if we have this behind a pref so that downstream can have a behaviour that fits their need (I could see other downstream consumers wanting this without going all the way maybe?), and we can keep doing what we're doing.

Georg, Richard, do you need help to put this behind a pref, or are you good? We could write tests for this to make sure it doesn't regress/has the right behaviour, let me know if that would be of interest.

Comment 14

[Georg Koppen]

(In reply to Paul Adenot (:padenot) from comment #13)

Are we going to do something about this? The patch is simple and contained and I don't foresee any maintenance issue if we have this behind a pref so that downstream can have a behaviour that fits their need (I could see other downstream consumers wanting this without going all the way maybe?), and we can keep doing what we're doing.

Georg, Richard, do you need help to put this behind a pref, or are you good? We could write tests for this to make sure it doesn't regress/has the right behaviour, let me know if that would be of interest.

I am not sure who makes the decision here to get this code into mozilla-central. jva said clearly our current patch is not going to fly and then stopped communicating when I tried to come up with an approach that might be better for Mozilla. So we were not sure what we should do. That said, if Mozilla would take our patch behind the pref, then we can adapt what we have and we'd be happy to do so.

Comment 15

[Paul Adenot (:padenot)]

Sorry I should have been clearer.

I'll take a patch for mozilla-central do implement this, if it's behind a pref, so that both cache policy can coexist, and downstream users of the code can do what they want/need to do. I believe jya's problem was doing in-memory caching for all Firefox's distributions, when in private mode.

We (Mozilla) are not going to distribute a build where this pref is set to true (and will continue doing the same behaviour that we do today, knowing the issues outlined in comment 10, that, to me, are valid), but Tor Browser developers will then be able to implement the policy they want.

Also, having a pref to turn this on and off is useful to assess the possible performance problems that having in-memory media caching can have, and (looking at the patch provided), having a pref seems very simple to maintain.

With that said, I was also offering help, if that would be useful, because prefs code has recently changed (not knowing the level of familiarity of the developers here with the new pref code).

Comment 16

[richard (Tor Project)]

I've updated the patch and submitted for review on phabricator: https://phabricator.services.mozilla.com/D22337

This patch has the same behavior but adds a check for 'browser.privatebrowsing.forceMediaMemoryCache' as well as determining whether we are in Private Browsing Mode when determining which sort of cache to return.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:richard, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 18

[Paul Adenot (:padenot)]

Richard, this has been accepted, do you need help landing this?

Comment 19

[richard (Tor Project)]

(In reply to Paul Adenot (:padenot) from comment #18)

Richard, this has been accepted, do you need help landing this?

Hm yeah, adding 'checkin-needed' here no longer seems to work. What's the new magic for getting patches submitted?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d27b640d4ead
Ensure media cache is memory-only when in Private Browsing Mode r=jya,xeonchen

Comment 21

[Paul Adenot (:padenot)]

checkin-needed still works, I don't see it set here. In the new world of phabricator, if you open your revision, you can edit on the right, and add checking-needed in the Tags section, it should auto-complete. I don't think setting it on bugzilla still works, but I could be wrong.

I've triggered the landing for you in any case, https://hg.mozilla.org/integration/autoland/rev/d27b640d4ead12d854b43bc981d764cf302b5d7d. Thanks!

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d27b640d4ead