See <https://github.com/whatwg/storage/issues/31>.

We should fix this in the next release of Firefox (49).

Ehsan, David, can you help find an owner to work on this? We are heading into 49 beta 5 so there is not a lot of time to fix/uplift this for 49.

Jan's going to do this.

Actually maybe someone from Taipei wants take it, let me ask them ...

I re-read some emails about this and also the issue on github, but it's still not clear to me what we actually want to do to fix this bug. Do you guys have any concrete ideas ?

We need to pad and randomize the effective quota usage of opaque responses. So I think QuotaManager needs some API to say "AddOpaqueEntity()" and "RemoveOpaqueEntity()". For each opaque "entity" QM would artificially increase quota usage for the origin by some random amount. Ideally I think we should do this without actually consuming the user's disk space.

I think Ben described it better than I could!

Ben, I don't know much about DOM cache internals, I just quickly went through the code. StartStreamCopy() seems to invoke stream copying for responses, so we can check here if a response is opaque and pass this info to BodyStartWriteStream(). That would then call FileOutputStream::Create() with a new argument e.g. aAddOpaqueEntityOnClose FileQuotaStream<FileStreamBase>::Close() would call the proposed AddOpaqueEntity() That also means that Close() may fail because we reached storage limits (group or global limit) when we tried to artificially inflate usage for given origin. Does this sound good to you ? I need also figure out when to call RemoveOpaqueEntity() of course. The estimate() implementation which is in the works calculates usage by iterating over memory objects which will contain the padding (not by getting real file sizes on disk). So everything should work as expected I guess.

(In reply to Jan Varga [:janv] from comment #8) > Ben, I don't know much about DOM cache internals, I just quickly went > through the code. > StartStreamCopy() seems to invoke stream copying for responses, so we can > check here if a response is opaque and pass this info to > BodyStartWriteStream(). That would then call FileOutputStream::Create() with > a new argument e.g. aAddOpaqueEntityOnClose > FileQuotaStream<FileStreamBase>::Close() would call the proposed > AddOpaqueEntity() > That also means that Close() may fail because we reached storage limits > (group or global limit) when we tried to artificially inflate usage for > given origin. > Does this sound good to you ? So, I was thinking dom/cache would just call AddOpaqueEntity() before it started writing. So the limit is already baked in before it streams to disk. If it hits an error during writing it has to call RemoveOpaqueEntity() again. > I need also figure out when to call RemoveOpaqueEntity() of course. We can just call RemoveOpaqueEntity() when the record is removed from sqlite. That way any orphaned body files that are cleaned up on browser restart don't have to it called. > The estimate() implementation which is in the works calculates usage by > iterating over memory objects which will contain the padding (not by getting > real file sizes on disk). So everything should work as expected I guess. Are you going to persist the random pad amount? Or recalculate the random pad every browser restart? Just curious. I think either way could work.

Hm, yeah maybe we can do it before writing, I'll see ... Regarding persistence, even when we just recalculate the random pad we need to know which origins actually contain padding and how many of them. One of the partial projects for the next-gen local storage is to persist all the quota info we have in memory, since it's very likely that local storage is used much more often than IndexedDB and DOM cache. So scanning hundreds of directories after every restart would be bad. For this bug we don't need full caching only a simplified version of it fortunately. I'm not sure we'll be able to fix this for 49, but I'll do my best. Anyway, is this so big security problem given the fact we haven't shipped estimate() implementation yet and responses are stored compressed ?

I don't really have much to add. I asked Google to chime in with their solution.

(In reply to Jan Varga [:janv] from comment #10) > Hm, yeah maybe we can do it before writing, I'll see ... > > Regarding persistence, even when we just recalculate the random pad we need > to know which origins actually contain padding and how many of them. > One of the partial projects for the next-gen local storage is to persist all > the quota info we have in memory, since it's very likely that local storage > is used much more often than IndexedDB and DOM cache. So scanning hundreds > of directories after every restart would be bad. > For this bug we don't need full caching only a simplified version of it > fortunately. As long as QuotaManager can track how many opaque things it needs to pad for, I'm fine with the pad changing on every restart. > I'm not sure we'll be able to fix this for 49, but I'll do my best. > Anyway, is this so big security problem given the fact we haven't shipped > estimate() implementation yet and responses are stored compressed ? The issue is that content script can infer the size of opaque responses through the quota mechanism itself. For example, add items until you hit quota and writing fails. Then back off a little and try adding an opaque response. If it fits, then you have a bound on its size. Rinse and repeat to determine its exact size. So its not directly related to estimate().

I'm not that familiar with the codebase, but I'd just like to point out that for this defence to work properly, the random padding should be associated with a Response, and the only way a new random padding value can be generated, is to fetch the resource again (which takes a considerable amount of time, and is noticeable at the server side). By generating enough random padding values, it's possible to reliably infer the initial value. If an attacker can generate a new random padding value every time an opaque response is stored (approximately every ms), he can gather enough samples in a few seconds to render the added randomness obsolete.

Do you think it is realistic to fix this for 49? We are heading into beta 8 build today (momentarily) and beta 9 later in the week. I thnk we should aim for 50 here at the soonest. But we can still consider uplift if you land a patch.

I was on vacation last week, sorry. Well, I don't think this is realistic for 49, uplifting seems like a good option.

This isn't going to happen in 50.

Marking 52 as affected as well.

Talked with Andrew, we won't ship this in 51 and likely not 52, but it still is on the table for a future project.

[Tracking Requested - why for this release]: This isn't going to happen for 52 or 53 (well, we could *maybe* uplift to 53).

Tracking 54+ to keep this on the radar.

Shawn, I think we should keep tracking this somehow. It became a little hard to find after you removed that link.

(In reply to Anne (:annevk) from comment #21) > Shawn, I think we should keep tracking this somehow. It became a little hard > to find after you removed that link. Originally I thought this would even happen without exposing storage api, so I removed it from storage api meta bug.

Mark 52 fix-optional as there are no actions for the moment.

Tracking 55+ per Comment 21.

Tom, are you going to take this bug ?

Sure, I'll take it after finishing Bug 1348050 if you don't mind that.

Anne, do you think we need to fix this before we ship the Storage API (bug 1254428)?

If anything this should have blocked shipping navigator.storage.estimate(), which shipped in January per https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate. persist() and persisted() don't change anything here.

(In reply to Anne (:annevk) from comment #29) > If anything this should have blocked shipping navigator.storage.estimate(), > which shipped in January per > https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate. > persist() and persisted() don't change anything here. estimate() is disabled for release now. We planed to flip the preference together with persist(), persisted() and UI. So looks we should fix this before enabling estimate() by default on release.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #30) > (In reply to Anne (:annevk) from comment #29) > > If anything this should have blocked shipping navigator.storage.estimate(), > > which shipped in January per > > https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate. > > persist() and persisted() don't change anything here. > > estimate() is disabled for release now. We planed to flip the preference > together with persist(), persisted() and UI. So looks we should fix this > before enabling estimate() by default on release. Are we tracking shipping that in bug 1254428?

(In reply to Andrew Overholt [:overholt] from comment #31) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #30) > > (In reply to Anne (:annevk) from comment #29) > > > If anything this should have blocked shipping navigator.storage.estimate(), > > > which shipped in January per > > > https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate. > > > persist() and persisted() don't change anything here. > > > > estimate() is disabled for release now. We planed to flip the preference > > together with persist(), persisted() and UI. So looks we should fix this > > before enabling estimate() by default on release. > > Are we tracking shipping that in bug 1254428? Not exactly. There are some nice to have enhancements blocking bug 1254428 per my best understanding. In the storage project, we use the whiteboard [storage-v1] to indicate shipping blockers.

Hi Ben and Jan: I put things that I've investigated so far below. Please feel free to correct me if I'm wrong. Thanks! Things I need to do: - Generate the random padding size while receiving an opaque response from the Network[1]. The reason is that we don't want to make attackers generate enough random padding value easily base on comment 13. - Create two function QM::AddOpqueEntity(uint size), QM::RemoveQpaqueEntity(uint size). - AddOpqueEntity() is called by Cache while we are starting to write[2]. - RemoveQpaqueEntity() is called by Cache while the cache is failed to write[3] or when the record for that response is removed[4]. - Padding size for an opaque response goes with internalResponse and is stored with cacheResponse - Total padding size for an origin is tracked by QuotaManager and it will be recorded it in the metadata file. - For QuotaManager, it needs to read padding sizes for each origin from metadata file and add it to its originInfo when we start the Firefox and start to calculate the usage[5]. Besides, it also needs to remove the size when the origin is evicted or cleared. Things I need to figure out: - Which kind of random method should I use? Seed & Mean? - Async problems (e.g. cache writes/deletes successfully, but fails to write in metadata on QM or somethings like this). [1] http://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#377 [2] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#782 [3] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#935 [4] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#1016 [5] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5047

I don't think the metadata file is the right place, only as a last resort. There's storage.sqlite in the profile directory, currently only used for checking storage version. Have you investigated it as a place for storing paddings ?

(In reply to Jan Varga [:janv] from comment #34) > I don't think the metadata file is the right place, only as a last resort. > There's storage.sqlite in the profile directory, currently only used for > checking storage version. Have you investigated it as a place for storing > paddings ? Actually, that is one things I want to discuss with you tomorrow. :) Since it is in the profile directory, I wonder we need to store at least two more columns (origins, padding size) for this bug. The reason is that we might need a simple way to remove storage for origins (don't need to query padding sizes for specific origins). Therefore, I proposed to store it in metadata or somewhere else under origin directory.

There can be none, 1 or a thousand of padding sizes for an origin, right ? So this info is rather dynamic and the metadata file is a plain binary file. Changing metadata format is hard and it usually causes a major version bump. Adding a new SQL table to storage.sqlite would only require minor version bump (which is a backward compatible change). The database will be rather small, so quering/deleting padding sizes for an origin shouldn't be slow. But yes, we will have to be careful about correct synchronization, crash recovery, etc.

(In reply to Jan Varga [:janv] from comment #36) > There can be none, 1 or a thousand of padding sizes for an origin, right ? > So this info is rather dynamic and the metadata file is a plain binary file. > Changing metadata format is hard and it usually causes a major version bump. > Adding a new SQL table to storage.sqlite would only require minor version > bump (which is a backward compatible change). > The database will be rather small, so quering/deleting padding sizes for an > origin shouldn't be slow. > But yes, we will have to be careful about correct synchronization, crash > recovery, etc. Oh, I see. Thanks for the feedback!!

(In reply to Tom Tung [:tt] from comment #33) > - Padding size for an opaque response goes with internalResponse and is > stored with cacheResponse Do you mean CacheStorage needs to put something in its sqlite for the padding? That doesn't seem necessary if we just are adjusting pad values for the overall quota origin. Seems like we just need to tell the QM when we add/remove opaque things.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #38) Thanks for the feedback! > Do you mean CacheStorage needs to put something in its sqlite for the > padding? That doesn't seem necessary if we just are adjusting pad values Yes, that's what I meant. > for the overall quota origin. Seems like we just need to tell the QM when > we add/remove opaque things. Got it! There is one more thing need to be clarified: when should I random the pad value? Generating a new pad value for an origin when Cache API writes an opaque response into disk seems not a good idea based on comment 13. Generating a new pad value for an origin when someone fetches another cross-origin request with no-cors mode seems better. I'm thinking to generate a new pad value for an origin when fetching. And, make InternalResponse keep it. Then, apply the value and update it into storage.sqlite once Cache API writes the response successfully.

Hmm, I guess I didn't understand comment 13 before. That does change things a bit in my mind. That suggests we do need to store the pad amount in CacheStorage. Sorry for my confusion. So yea, when we create an opaque response the bad amount would get added to InternalResponse. That value would then get persisted in and out of CacheStorage so it won't change over time. CacheStorage can then include the pad amount in its QuotaClient::GetUsageForOrigin() call. I think this means you don't need to store the pad amount in QM storage.sqlite. Right? Can we then just add an argument to FileOutputStream::Create() that specifies the pad amount? I guess what I don't understand about QM is if it keeps an in-memory running total of quota used or if it recalculates from disk all the time. Clearly GetUsageForOrigin() can include pad for the "recalculate from disk" approach, but if it keeps a running total in memory then we need to get the pad included there. Jan, does any of that make sense?

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #40) > CacheStorage can then include the pad amount in its > QuotaClient::GetUsageForOrigin() call. I think this means you don't need to > store the pad amount in QM storage.sqlite. Right? We had a meeting today and I mentioned to Tom that there's another option how to store the pad amount. For each quota tracked file we would create another file with the pad amount stored in it. InitOrigin()/GetUsageForOrigin() would then detect it and calculate file size/usage correctly. QuotaManager::GetQuotaObject() needs to be modified too to correctly calculate file size (to include the pad amount). > > Can we then just add an argument to FileOutputStream::Create() that > specifies the pad amount? Maybe, but we can also add something special like QuotaManager::CreatePaddingFor(nsIFile* aFile) I'm not sure about this, I'll let Tom investigate what would be the best way to do it. > > I guess what I don't understand about QM is if it keeps an in-memory running > total of quota used or if it recalculates from disk all the time. Clearly > GetUsageForOrigin() can include pad for the "recalculate from disk" > approach, but if it keeps a running total in memory then we need to get the > pad included there. For default and temporary storage, we cache usage in memory objects. These objects are created by calling QuotaClient::InitOrigin(). GetUsageForOrigin() needs to be updated too, to correctly report origin usage in page info dialog and new storage preferences.

Just to add some notes: (In reply to Jan Varga [:janv] from comment #41) > (In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment > #40) > > CacheStorage can then include the pad amount in its > > QuotaClient::GetUsageForOrigin() call. I think this means you don't need to > > store the pad amount in QM storage.sqlite. Right? > > We had a meeting today and I mentioned to Tom that there's another option > how to store the pad amount. > For each quota tracked file we would create another file with the pad amount > stored in it. > InitOrigin()/GetUsageForOrigin() would then detect it and calculate file > size/usage correctly. > > QuotaManager::GetQuotaObject() needs to be modified too to correctly > calculate file size (to include the pad amount). It's better to let QM know the current usage including padding in-memory (OriginInfo or QuotaObject), so that it don't need to ask for getting opaque responses' padding size each time when QM wants to check the usage (e.g. MaybeUpdateSize checks the current usage to ensure the usage don't exceed the quota, LockedCollectOriginsForEviction check the usage for inactive origin). As Jan mentioned in that meeting, for storing padding sizes in disk, I will try to find out the appropriate way to store it in QM (storing in storage.sqlite or storing in another new file). Right now, I may have one more option which is just storing it CacheStorage and QM gets the overall padding sizes when it's initiating. Besides, I also need to ensure we get the same usage from estimate() and preferences UI. > > Can we then just add an argument to FileOutputStream::Create() that > > specifies the pad amount? > > Maybe, but we can also add something special like > QuotaManager::CreatePaddingFor(nsIFile* aFile) > I'm not sure about this, I'll let Tom investigate what would be the best way > to do it. Sure, I'll figure it out.

Also there are patches to simulate the attack (getting the anti-CSRF token) in Bug 1350072. I can reproduce it by running them.

For completeness: the size of the response headers (more precisely Content-Length) can also be leveraged to exploit BREACH-like attacks (explained in a bit more detail in the second paragraph here: https://tom.vg/2016/08/request-and-conquer/#compression-based-attacks).

Current Progress: P1: Get opaque response's padding length when we get it from the network. P2: Store it into Disk as Cache SQLite P3: Modify MaybeUpdateSize to make it be able to calculate padding as usage rather than just usage. P4: Make OriginInfo, GroupInfo, ... etc. to be able to calculate padding, too. P5: Make Cache API be able to notify QuotaManager response's padding. ToDo: 1. Consider storing a copy(overall padding size for an origin) for QuotaManger. 2. Investigate random method, mean and seed (may refer to [1], but needs to consider how to get size of opaque response in FetchDriver.) 3. Get overall padding for origins when initializing QuotaManager. 4. Testcases [1] https://github.com/whatwg/storage/issues/31

Hi Ben, I'm working on updating padding length for an opaque response and utilize FileOutputStream to update it. I found that we'll create two different directories and files if we use Cache:Add to add the same request twice. And, I was thinking to utilize the mechanism for adding the same request via Cache:Add/Cache:Put twice to update padding length. However, I found that we only update the usage to QuotaManager when Cache API is about to write something into the disk [1]. We don't update the usage to QuotaManager when we delete files and directories (file->Remove() at [2-4]). e.g. caches.open("test").then(c => c.add(url)); // first // create file xxxx/cache/morgue/206/{92412019-df4e-be4b-9e6f-56645876f0ce}.tmp <- update padding to QM through MaybeUpdateSize()[1] // .tmp -> .final caches.open("test").then(c => c.add(url)); // second // create file xxxx/cache/morgue/217/{5825fda4-50dd-1f48-94a3-f455d03bcfd9}.tmp <- update padding to QM through MaybeUpdateSize() [1] // delete file xxxx/cache/morgue/206/{92412019-df4e-be4b-9e6f-56645876f0ce}.tmp <- update padding to QM ??[2] // .tmp -> .final Did we do that for any reasons? If so, could you tell me why we did that? If not, I'll file a bug for it and try to resolve it first. Thanks! [1] http://searchfox.org/mozilla-central/source/dom/quota/FileStreams.cpp#81 [2] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#66 [3] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#250 [4] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#324 [5] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#465

I thought QuotaManager wrapped nsIFile and automatically adjusted the size when Remove() is called. Jan, do we have to do something else to explicitly tell QM about removals?

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #52) > I thought QuotaManager wrapped nsIFile and automatically adjusted the size > when Remove() is called. Jan, do we have to do something else to explicitly > tell QM about removals? No, there's nothing like a wrapping around nsIFile. Here's what needs to be done: rv = file->GetFileSize(&fileSize); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } rv = file->Remove(false); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } quotaManager->DecreaseUsageForOrigin(type, group, origin, fileSize);

Ok. I find it confusing that if you use a writable stream, its wrapped, but if you use the nsIFile you get from QM its not wrapped. So Tom, can you please fix that as part of this as well? Thanks.

Well, you could use a stream (with io flags PR_TRUNCATE) to truncate a file, then usage updating would be done automatically. That might be even nicer way how to do it.

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #54) > So Tom, can you please fix that as part of this as well? Thanks. Sure!

Comment on attachment 8870372 [details] [diff] [review] patch for updating usage for dom cache Move this patch to bug 1367309

This is on going, but more likely to happen since 56. Considering this has been here for quite a while and it's late in 55, mark fix-optional for 55.

Todo: generate real random number.

Next: - Calculate overall padding from cache.sqlite when initializing QuotaManager - Note: Be careful about only one connection is allowed to cache.sqlite at the same time (make sure which thread access that SQLite) based on meeting(5/24) with Jan. - Random method...

Based previous service worker meeting, I should find some other way to store paddings rather than using cache.sqlite file. I had used cache.sqlite to store paddings for each response, and it takes too much overhead/time when initializing each origin to get the usage. This approach needs to spend additional time to initial cache.sqlite for getting overall paddings for each origin. So, I list what should be taking care below: 1. For each opaque/opaque-redirect response is stored by Cache.add()/Cache.addAll()/Cache.put(), its padding length needs to be stored and updated to the QuotaManger. 2. When the QuotaManger is initializing or the user checks overall usage from preference UI, we need to get padding length for origin from the file.

Hi Ben and Jan, I'm finding a way out to store padding for the opaque response. I'm used to considering to store it in the cache.sqlite file. But, it takes too much time to initialize cache.sqlite file when QM gets usage from quota clients. Right now, I try to make each opaque response create a pad file to record its padding length. The pad file is named ".pad" and stores with the body file (e.g. /morgue/{end-of-id}/{id}.final, /morgue/{end-of-id}/.pad). Though this method takes extra time on creating/accessing a file to store/read padding length, it saves time to initialize cache.sqlite file. Before starting to finalize the patches and send them to review, I'd like to ask for your suggestions for this approach. I put my current implementation and questions below inline. Could you help me to check if there are problems with it I need to fix or something I need to take care or I miss anything? Thanks! - Implementation For DOM Cache 1. While creating opaque and opaque redirect responses [1, 2], generate padding length. [1] http://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#65 [2] http://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#68 2. Before writing the body to OS filesystem [3], create a file to store padding length of the response if it's an opaque or opaque redirect response (padding length > 0). And, update the padding length to QM. [3] http://searchfox.org/mozilla-central/source/dom/cache/FileUtils.cpp#143 3. When Cache gets the response from OS filesystem(e.g. cache.match[4], cache.matchAll[5]), read the padding length from the pad file (file to store padding length) as well to get its padding length. [4] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#511 [5] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#577 4. When removing the body file (BodyDeleteFiles()[6]), remove the file for recording padding length as well and update decreasing padding length. So that, if a failure happens while writing the body to file, the padding length will be updated to the original length by BodyDeleteFiles(). [6] http://searchfox.org/mozilla-central/source/dom/cache/FileUtils.cpp#254-284 5. When it's the first time to call EnsureOriginInitialized(), the QuotaManager gets usage from the quota clients. I'll calculate all the pad files while traversing the directory in GetBodyUsage()[7]. [7] http://searchfox.org/mozilla-central/source/dom/cache/QuotaClient.cpp#38-61 Note: - Pad file is created on target thread, but it can be read from target thread (when cache API is called) and background thread (when checking usage). For QM 1. Add "mPaddingLength" attributes for OringinInfo and GroupInfo and "mTemporaryStoragePaddingLength" variable to track overall padding lengths for an origin, a group and all. 2. When returning overall origin usage (GetUsageOp[9], GetOriginUsageOp[10]), count the padding length as usage. [9] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#6857 [10] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#7010 - Question For DOM Cache 1. Is there a method to estimate how many usages will take to store an opaque response before we actually store it into filesystem? The reporter suggested it's better to make the mean relate to the size of the response. 2. I'm thinking to only store pad file when the response owns padding. It may expose the cached response type is opaque/opaque-redirect or not, but I think it's fine to do that. Do I need to create this file for all the cached body like a metadata file? For QM 1. CacheQuotaClient::GetUsageForOrigin() live in the backgroundThread. Since QM provides an API to calculate overall usage by calling this function at runtime, I'm a little afraid the result for current usage might be incorrect. Or it's okay to do that since we just need to have a snapshot of overall usage? If so, I think it's fine for getting padding in the background thread as well. 2. Do we need to distinguish padding length and usage in QM? I think it's better to do that for checking purpose. I store them separately for now, but I cannot find any other strange reason to do this and I'm not sure it's a good idea or not.

(In reply to Tom Tung [:tt] from comment #67) > - Question > For DOM Cache > 1. Is there a method to estimate how many usages will take to store an > opaque response before we actually store it into filesystem? The reporter > suggested it's better to make the mean relate to the size of the response. I don't think so. We don't know the full size on disk until you have written the temp file. You can create the pad metadata before moving that to its final location, though. > 2. I'm thinking to only store pad file when the response owns padding. It > may expose the cached response type is opaque/opaque-redirect or not, but I > think it's fine to do that. Do I need to create this file for all the cached > body like a metadata file? Please only create these for opaque responses. (Is opaque-redirect a concern here? I didn't expect body data there.) One reason not to store this for every body file is that these files will each take up one disk block. This is typically 4KB which is a fair amount of overhead for such a small amount of data.

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #68) Thanks for answering! > I don't think so. We don't know the full size on disk until you have > written the temp file. You can create the pad metadata before moving that > to its final location, though. Oh, I see. That might be a good solution, but there are some issues need to be figured out. Such as: checking if we can write the padding into virtual space (it acquire QM mutex lock twice) since it updates padding after writing the temp file. (Currently, we check the quota when writing the temp file.) Anyway, I'll try to find a way to do that. If I cannot find a good solution, I will file a follow-up bug for it. > Please only create these for opaque responses. (Is opaque-redirect a > concern here? I didn't expect body data there.) Sorry, I didn't think too much about this. I thought the different between opaque-redirect and opaque response is redirected or not. I didn't notice opaque-redirect response don't have a body data. > One reason not to store this for every body file is that these files will > each take up one disk block. This is typically 4KB which is a fair amount > of overhead for such a small amount of data. Got it!

In today's meeting, - Jan suggested that I can think about to save padding length in the database(cache.sqlite), but recording overall padding length for an origin in a file cache directory (xxxOrigin/cache/.padding) to avoid taking too much time when initializing QM. And, it takes about the same time to get the file size and read this file content. - Besides, the file should only take 8 bytes to store overall padding length. We don't count this file as part of usage. We only count its content(padding) as part of usage. - For consistent problem (having different overall padding length on sqlite and .padding file), I can reference from indexedDB. It makes sure that it takes care all the unsync problems for a file and database. Basically, I need to update the padding length to QM and file just before executing Commit() to cache.sqlite database. - I don't need to treat usage and padding as two different things in QM.

(In reply to Tom Tung [:tt] from comment #70) Jan, please correct me if I wrote anything wrong or I forgot to put anything. That's why I don' cancel the ni flag.

Yeah, you understood it correctly. We'll need to talk about the consistency problem a bit more, but for now you don't have to worry about it too much.

Hi Ben, If I create a metadata file to track current overall padding length for an origin in DOM Cache at cache directory(e.g. "<profile>/storage/default/http+++origin/cache/.paddings"). Also, I record each padding length on cache.sqlite file at the same time. So, the advantage of this method is it only takes eight more bytes and one more column on the cache.sqlite database to track the padding for DOM Cache. When calculating usage from each quota client, DOM Cache only needs to check the .padding file to get the overall usage of this origin. It takes about the same time to get the file size. Besides, compared to create a .padding for each cache.put() an opaque response into the disk, it won't create too many files in the cache directory. (How many cached opaque responses do a normal user have in average? If they are not so many, maybe we could have a .padding file with each cached opaque response like I said in comment 67) Moreover, it won't need extra read when cache.match()/cache.key() to get a cached opaque response. We get its padding length from cache.sqlite when we get the header. The disadvantage is that I need to implement this mechanism carefully to avoid inconsistency problem between the .padding file and cache.sqlite file. Does it sound good to you?

Sound good to me. Thanks!

Below are the current thoughts after discussing with Jan Each origin should have a .padding file to ensure something like someone wouldn't remove padding file manually. -> it means we will need to increase 8 bytes for every cache directory The biggest problem for now is that how to deal with the version compatibility. -> solution: major upgrade? [Execute cache operation] Current step A. BodyStartWriteStream(): Write the body to the temp body file (can be any thread) Current step B. OnAsyncCopyComplete(): After finishing writing body file (target IO thread) 1. Get the size of body file and calculate the padding size for the opaque response 2. Update padding size to QM by creating quotaObject (path: the body file's path) Current step B.1. Change temp body file to the final body file (BodyFinalizeWrite()) (target IO thread) 3. Copy the .padding file to .padding-tmp file 4. Read the overall padding size from the .padding-tmp file(or calculate it from cache.sqlite file?) 5. Update the overall padding size to the .padding-tmp Current step B.2. Update the cache.sqlite (commit) (target IO thread) 6. Rename the .padding-tmp file to the .padding file *Fail -> remove .padding-tmp [Upgrade] 1. Search opaque responses from each cache.sqlite and generate padding size for each of them 2. Calculate overall padding size for an origin and write it to the cache directory padding file (Still do this even the padding size for the origin is zero) 3. Update the QuotaManager's schema (Major upgrade -> Need to consider more) http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#129 [InitOrigin for each quota client] 1. Find .padding files from the cache directory - If we can find it, get the padding size from the .padding files - If we cannot find it or we cannot read it, we need to access the cache.sqlite to get all the recording padding size from the database and write it to the .padding files. (Need to recalculate the overall padding size if the tmp file is found, becasue we cannot distingulish if we crash before commiting the sqlite or not)

Hi Ben, I planned to add padding for all the existing opaque responses while upgrading the cache.sqlite's version. However, I'm not sure if we need to do this or not. I mean the opaque responses which have been stored in the database before upgrading to the newest version. So, I wanted to add padding for them to avoid exposing the existing opaque response size. However, I'm not pretty if this would happen. Besides, I'm a little worried about what to do if the generating padding makes the usage exceed the limit when upgrading the database. Could you give me some suggestions for this? Thanks!

I don't think we need to go back and add it to existing opaques responses. We can just add it to new opaque responses. As long as the code is smart enough to understand there might be an old opaque response without any padding set.

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #77) I see. Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=969b8d4de3b07788e7162fe69a658f717c8f15dc&selectedJob=115567389

Uhg, I'm so bad at statistics. I think the mechanism in the patch allows leaking the size after a number of observed measurements. The current mechanism: a) Picks a random number R between 0 and 102400 b) Adds the real size to R c) Rounds R up to a multiple of 20483 (It's not clear to me why a prime number was picked though...) If you make enough measurements, you will observe that the smallest and the largest bucket occur less frequently than the others. Based on the frequency of the smallest and largest bucket, and the knowledge that it's a uniform distribution of random numbers, it seems like you can calculate the real size? I know we are restricting the 'pick a random padding' call only to resource download to prevent this from being too easy, but I'm still concerned. I wonder if we can eliminate the vulnerability entirely...? What if on browser startup we choose a cryptographic random number and use it in conjunction with an HMAC scheme. Calculate the padding to be used as a function of HMAC(key, size-of-resource) where the key is calculated as hash(crypto-random_number || resource uri). (We need to include the resource uri in the key. This prevents an attacker from building a lookup table of "This real size maps to this padding value.") This way, re-requesting a resource will always return the same (padded) size for as long as the browser is running - without actually having to remember the decision if the item is removed from storage. (We don't attempt to defend against attacks occurring across browser restarts.) I'm not certain this is a safe approach, nor am I certain the current approach is wrong - would love to get feedback.

I thought about this more. 1) The HMAC with the resource URI as part of the key might work if there was one and only one representation of a URI, but there are infinite (querystrings.) This puts us back to the statistical analysis. Attempting to sanitize the URI a) wouldn't work much of the time and b) in some web applications would leak the exact size after only two queries. 2) Using the origin instead of the resource URI would break in a small number of queries if the attacker can create a lookup table of sizes on that origin. For things like twitter this is easily possible with attacker controlled data. So the HMAC idea is out I think. The attack I see with the current design - assuming it works - is based around the idea that the occurrence of the buckets is non-uniform, because the first and last buckets occur less frequently. If we used a weighted distribution - instead of uniform - to make those buckets appear as frequently as the others, that should go away. But are we hiding anything by _having_ buckets? I don't think we are. The real size is always somewhere in the middle bucket. So I think the 'add a random number' part of 'add a random number and then round up' should be removed. That eliminates the bucket-occurance attack and the false security in having multiple buckets a size can be in.

(In reply to Tom Ritter [:tjr] from comment #92) Thanks for giving feedback! > The current mechanism: > a) Picks a random number R between 0 and 102400 > b) Adds the real size to R > c) Rounds R up to a multiple of 20483 (It's not clear to me why a prime > number was picked though...) Sorry, I don't have much sense with security things... So, I have no idea how to pick up an appropriate number. I just arbitrary pick up (102400, 20483) for now to test locally. I reference from the solution provided in [1]. 102400 is rMax and 20483 is D in [1] (For values of rMax and D, tomvangoethem suggests 100kb and 20kb respectively). Thus, these two magic numbers should be much more meaningful and should be changed after gaining more feedback. [1] https://github.com/whatwg/storage/issues/31 > If you make enough measurements, you will observe that the smallest and the > largest bucket occur less frequently than the others. Based on the frequency > of the smallest and largest bucket, and the knowledge that it's a uniform > distribution of random numbers, it seems like you can calculate the real > size? Just discuss with Tim Huang, we find out it's probably to get the real size by collecting tons of data and knowing it's a uniform distribution. (In reply to Tom Ritter [:tjr] from comment #93) > So I think the 'add a random number' part of 'add a random number and then > round up' should be removed. That eliminates the bucket-occurance attack and > the false security in having multiple buckets a size can be in. If we do this, I guess we may need to prevent the roundUp number, says D, is related to the real size, says f. For example, it might be easy to perform the attack (utilize we will have smaller size after zip the file) in Bug 1350072 when D = f + 1 (It makes the result size become even more obviously between guessing the correct secret key and not).

Update patch to make response copy its padding size while clone().

Update patch for making part of code be initialied once.

Update patch.

Insert a patch for adding a mutex lock to protect the directory lock from accessing from different threads.

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

Since comment 92 and comment 93 (the way to create padding size; P5), I would like to only send P1 ~ P4 to review first.

Comment on attachment 8888698 [details] [diff] [review] P1: Make InternalResponse and CacheResponse be able to keep padding size. Hi Ben, Sorry for sending review request when you are in PTO... :( But, I think you and Jan are the most appropriate reviewers to review this. In this bug, I only pad the opaque response by adding virtual padding size on it. So, basically, I update more size to QM while writing body files & SQLite entries and decrease more when removing body files & SQLite entries. Besides, to prevent from spending too much time on initializing directories, I add a directory padding file on each DOM Cache directories to avoid QM needs to open "cache.sqlite" when initializing. However, because of QM gets quota client's usage on Quota thread and DOM Cache do most of the things on its IO threads, I add a mutex lock to avoid read/write the same directory files at the same time. Unfortunately, since I add a table on cache.sqlite to store padding size and make QM add directory file for every existing DOM Cache directory, I need to upgrade both SQLite files. It means we won't support backward compatibility after applying my patch. This patch is mainly to add padding size attribute on InteranlResponse and CacheResponse to track response's padding.

Comment on attachment 8887435 [details] [diff] [review] P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite. This patch is mainly to upgrade cache.sqlite to add zero padding size for existing opaque responses

Comment on attachment 8887436 [details] [diff] [review] P3: Record padding size into cache.sqlite This patch is mainly to store padding size into cahce.sqlite

Comment on attachment 8887437 [details] [diff] [review] P4: Update padding size to the QuotaManager This patch is mainly to update padding size to QM. I increase the padding size to QM between finishing writing the temporary body file and finalizing the body file. For the timing to decrease the padding size, I choose to do that after DOM Cache deletes the orphaned body file. Hi Jan, Could you help me to review this part as well? Thanks!

The thing we're trying to solve here is to make it *sufficiently* hard for an attacker to get the size of a response. More precisely, it should not be possible for an adversary to get a better estimate than with other known (wont-fix?) techniques (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1211669 or just network-based timing attacks). These other techniques can easily differentiate between resources that have ~5kB size difference, so we should aim to prevent better attacks. As Tom Ritter mentioned: the HMAC approach looks interesting, but I don't think it's feasible in real-world applications because of bogus URL parameters that can be added. Origin-based also won't work, because it would only take a single resource with a fixed size to figure out the padding that's being used. If I understand correctly, Tom Ritter also suggested not adding a random number and just rounding up. I would advise against that: when there is content on the resource that is reflected from the request (or otherwise under the control of the attacker), the attacker can add padding to the response until it reaches the tipping point of a bucket size. I.e. the attacker would see the response size grow from n * bucketSize to (n+1) * bucketSize. With this, he can compute the exact response size (although this can be application-specific, I think the consequences can be much more severe). Regarding to having a non-uniform distribution so all buckets would occur equally as often: I think that this is subject to the same issues as I mentioned above, i.e. for a certain amount of reflected content the smallest observed size (including padding) will be (n+1) * bucketSize. This will require slightly more requests from the attacker, but I think it's still feasible. If all buckets have the same probability of being drawn, then the probability of getting the lowest one is 1/numBuckets. The approach I originally suggested does not suffer as badly from this attack: the closer the response gets to tipping point, the less likely it is that the smallest buckets gets chosen. Since I am definitely not a statistician, I can't say for certain which approach is the best. But I think it's good to make sure that there's no easy way to determine the exact size of a resource. To avoid any arduous statistical analysis, I've tried to assess approaches by means of simulation, i.e. try to think of the best way to attack the defence and quickly implement that to see how well it works. This is the code I wrote for the suggested defence: https://gist.github.com/tomvangoethem/eaf9312401ab6098282eb7631bff8547

Comment on attachment 8888698 [details] [diff] [review] P1: Make InternalResponse and CacheResponse be able to keep padding size. Review of attachment 8888698 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/cache/TypeUtils.cpp @@ +203,5 @@ > + > + if (aIn.Type() == ResponseType::Opaque) { > + aOut.paddingSize() = aIn.GetPaddingSize(); > + } else { > + aOut.paddingSize() = InternalResponse::UNKNOWN_PADDING_SIZE; Can we rewrite this as: MOZ_DIAGNOSTIC_ASSERT(aIn.Type != ResponseType::Opaque || aIn.GetPaddingSize() != InternalResponse::UNKNOWN_PADDING_SIZE); aOut.paddingSize() = aIn.GetPaddingSize(); @@ +296,5 @@ > case ResponseType::Default: > break; > case ResponseType::Opaque: > ir = ir->OpaqueResponse(); > + ir->SetPaddingSize(aIn.paddingSize()); Lets just copy the padding size unconditionally and assert its only non-UNKNOWN if its opaque. Similar to the proposed snippet above. ::: dom/fetch/InternalResponse.h @@ +229,5 @@ > + int64_t > + GetPaddingSize() > + { > + MOZ_ASSERT(Type() == ResponseType::Opaque); > + MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE || mPaddingSize >= 0); Can you make these MOZ_DIAGNOSTIC_ASSERT() please? @@ +240,5 @@ > + // Only pad the opaque response. > + MOZ_ASSERT(Type() == ResponseType::Opaque); > + // An opaque response's padding size cannot be reset once set. > + MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE); > + MOZ_ASSERT(aPaddingSize >= 0); MOZ_DIAGNOSTIC_ASSERT() for these as well. They are all cheap. @@ +243,5 @@ > + MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE); > + MOZ_ASSERT(aPaddingSize >= 0); > + > + mPaddingSize = aPaddingSize; > + } Also, can you move these implementations into the .cpp file? I know we have some methods in the header already, but I am encouraging folks to put things in the cpp unless there is measured performance reason to inline in the header. It makes it easier to find the code and compiles are faster.

Comment on attachment 8887435 [details] [diff] [review] P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite. Review of attachment 8887435 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/DBSchema.cpp @@ +148,5 @@ > +const char* const kTableOpaqueResponsePaddings = > + "CREATE TABLE opaque_response_paddings (" > + "padding_size INTEGER NOT NULL, " > + "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE" > + ")"; Is there a reason this is a separate table? I think we can just add nullable padding_size to the entry table. When NULL this column will take 1 byte per row. In contrast, storing as a separate table requires storing the entry_id, row length, row ID, foreign key, etc. If we were sure we would always have many many non-opaque compared to only a few opaque this might give some disk storage gains, but it will not be too large. I think for now we should just put padding_size inline in the entry table.

Comment on attachment 8887436 [details] [diff] [review] P3: Record padding size into cache.sqlite Review of attachment 8887436 [details] [diff] [review]: ----------------------------------------------------------------- Dropping this for now since I requested a DBSchema change in the previous patch.

Comment on attachment 8887437 [details] [diff] [review] P4: Update padding size to the QuotaManager Review of attachment 8887437 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/FileUtils.cpp @@ +268,5 @@ > + int64_t fileSize = 0; > + rv = bodyFile->GetFileSize(&fileSize); > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > + > + if (*aPaddingSizeOut == /* Unset padding size */ -1) { Can we use the InternalResponse constant for this instead of the bare -1? @@ +598,5 @@ > > rv = aFile->Remove( /* recursive */ false); > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > + // Size of Marker file is 0 I'm not sure what this comment means... I guess that if the file being removed is empty it might have a zero size? ::: dom/cache/Manager.cpp @@ +1032,1 @@ > int64_t mDeletedPaddingSize; Maybe add comments for these two explaining a bit. The mDeletedPaddingSize tracks any pad amount associated with overwritten entries. The mUpdatedPaddingSize tracks how much pad amount has been added for new entries so that it can be removed if an error occurs. Right?

(In reply to Tom Van Goethem from comment #111) > If I understand correctly, Tom Ritter also suggested not adding a random > number and just rounding up. I would advise against that: when there is > content on the resource that is reflected from the request (or otherwise > under the control of the attacker), the attacker can add padding to the > response until it reaches the tipping point of a bucket size. I.e. the > attacker would see the response size grow from n * bucketSize to (n+1) * > bucketSize. With this, he can compute the exact response size (although this > can be application-specific, I think the consequences can be much more > severe). Okay, having the 'add a random value' makes it harder to perform that attack. You still learn the size as soon as you observe a new bucketsize, but the odds you'll observe that new bucketsize are reduced because of the additional random value. Yes, that makes sense. > Regarding to having a non-uniform distribution so all buckets would occur > equally as often: I think that this is subject to the same issues as I > mentioned above, i.e. for a certain amount of reflected content the smallest > observed size (including padding) will be (n+1) * bucketSize. This will > require slightly more requests from the attacker, but I think it's still > feasible. If all buckets have the same probability of being drawn, then the > probability of getting the lowest one is 1/numBuckets. > The approach I originally suggested does not suffer as badly from this > attack: the closer the response gets to tipping point, the less likely it is > that the smallest buckets gets chosen. Hm. I agree, if the attacker controls the content. If the attacker can control the size of the response, the size is leaked as soon as the attacker can say with confidence "If I add X bytes I will sometimes get the smallest bucketsize A, if I add Y bytes I will never get bucketsize A". If the attacker cannot control the size of the response, the size is leaked as soon as the attacker can say with confidence "The smallest bucketsize S occurs with <this> frequency. That means the size is offset into bucket S+3 this amount." (I think.) Changing from a uniform distribution to what I'll call the 'equalized' distribution making all bucket occurrences the same would make it _easier_ for the attacker who can control the response length to leak the response size! (I think that's what you said Tom.) BUT. Changing from a uniform distribution to a normal distribution seems like it would make the attacker-controlled case take more queries, because it is harder for the attacker to hit the low bucket. It would make the attacker-uncontrolled case take the same amount though - the probability is known, it doesn't matter what it is. Maybe the correct approach here is to open a new bug to perform a hard analysis on the padding technique, and tweak it based on the outcome of that.

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #112) Thanks for the review! > Comment on attachment 8888698 [details] [diff] [review] > P1: Make InternalResponse and CacheResponse be able to keep padding size. > > Review of attachment 8888698 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comments addressed. > > ::: dom/cache/TypeUtils.cpp > @@ +203,5 @@ > > + > > + if (aIn.Type() == ResponseType::Opaque) { > > + aOut.paddingSize() = aIn.GetPaddingSize(); > > + } else { > > + aOut.paddingSize() = InternalResponse::UNKNOWN_PADDING_SIZE; > > Can we rewrite this as: > > MOZ_DIAGNOSTIC_ASSERT(aIn.Type != ResponseType::Opaque || > aIn.GetPaddingSize() != > InternalResponse::UNKNOWN_PADDING_SIZE); > aOut.paddingSize() = aIn.GetPaddingSize(); I think you mean this? MOZ_DIAGNOSTIC_ASSERT(aIn.Type() == ResponseType::Opaque || aIn.GetPaddingSize() == InternalResponse::UNKNOWN_PADDING_SIZE); Note: For InternalResponse -> CachedResponse, there is two conidtions for opaque reponse. The first is that the internalResponse is just fetch from network, and we have not generated padding size for it yet. The second is that the internalResponse had been cached before so it have a "persisted" padding size. > @@ +296,5 @@ > > case ResponseType::Default: > > break; > > case ResponseType::Opaque: > > ir = ir->OpaqueResponse(); > > + ir->SetPaddingSize(aIn.paddingSize()); > > Lets just copy the padding size unconditionally and assert its only > non-UNKNOWN if its opaque. Similar to the proposed snippet above. Sure. So, I'll write an assertion like below. Note: For CacheResponse -> InteranlResponse, opaque response won't have the unknown padding size. We should have generated a padding size for it. MOZ_DIAGNOSTIC_ASSERT((aIn.type() == ResponseType::Opaque) != (aIn.GetPaddingSize() == InternalResponse::UNKNOWN_PADDING_SIZE)); > ::: dom/fetch/InternalResponse.h > @@ +229,5 @@ > > + int64_t > > + GetPaddingSize() > > + { > > + MOZ_ASSERT(Type() == ResponseType::Opaque); > > + MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE || mPaddingSize >= 0); > > Can you make these MOZ_DIAGNOSTIC_ASSERT() please? Sure. > @@ +240,5 @@ > > + // Only pad the opaque response. > > + MOZ_ASSERT(Type() == ResponseType::Opaque); > > + // An opaque response's padding size cannot be reset once set. > > + MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE); > > + MOZ_ASSERT(aPaddingSize >= 0); > > MOZ_DIAGNOSTIC_ASSERT() for these as well. They are all cheap. Sure > @@ +243,5 @@ > > + MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE); > > + MOZ_ASSERT(aPaddingSize >= 0); > > + > > + mPaddingSize = aPaddingSize; > > + } > > Also, can you move these implementations into the .cpp file? I know we have > some methods in the header already, but I am encouraging folks to put things > in the cpp unless there is measured performance reason to inline in the > header. It makes it easier to find the code and compiles are faster. Sure, I see.

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #113) > ::: dom/cache/DBSchema.cpp > @@ +148,5 @@ > > +const char* const kTableOpaqueResponsePaddings = > > + "CREATE TABLE opaque_response_paddings (" > > + "padding_size INTEGER NOT NULL, " > > + "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE" > > + ")"; > > Is there a reason this is a separate table? I thought it better to store it in a seperate table since we only need to store padding size for opaque response. It takes less space if there is not so many opaque resposnes. > I think we can just add nullable padding_size to the entry table. When NULL > this column will take 1 byte per row. > > In contrast, storing as a separate table requires storing the entry_id, row > length, row ID, foreign key, etc. If we were sure we would always have many > many non-opaque compared to only a few opaque this might give some disk > storage gains, but it will not be too large. > > I think for now we should just put padding_size inline in the entry table. I see. I'll change the wey to store padding size.

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #115) > ::: dom/cache/FileUtils.cpp > @@ +268,5 @@ > > + int64_t fileSize = 0; > > + rv = bodyFile->GetFileSize(&fileSize); > > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > + > > + if (*aPaddingSizeOut == /* Unset padding size */ -1) { > > Can we use the InternalResponse constant for this instead of the bare -1? Sure > @@ +598,5 @@ > > > > rv = aFile->Remove( /* recursive */ false); > > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > > > + // Size of Marker file is 0 > > I'm not sure what this comment means... I guess that if the file being > removed is empty it might have a zero size? Oops, that is self-note for reminding me there is a case that we delete a zero size file (marker file). I'll remove that. > ::: dom/cache/Manager.cpp > @@ +1032,1 @@ > > int64_t mDeletedPaddingSize; > > Maybe add comments for these two explaining a bit. The mDeletedPaddingSize > tracks any pad amount associated with overwritten entries. The > mUpdatedPaddingSize tracks how much pad amount has been added for new > entries so that it can be removed if an error occurs. > > Right? Yes, you are right. I'll add comments for them to make its readable

ni myself for comment #111 and comment #116

Move the assertions to getter/setter function.

Addressed the comment (add a column instead of a new table).

Resend the review request since I have addressed the comment for P2

Address comment, so resend the review request.

(In reply to Tom Ritter [:tjr] from comment #116) > Maybe the correct approach here is to open a new bug to perform a hard > analysis on the padding technique, and tweak it based on the outcome of that. I file the bug 1383656 for following up. I think maybe we could implement a basic way to generate the padding in this bug and tweak the value/distribution in that bug later?

Comment on attachment 8889316 [details] [diff] [review] P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite. Review of attachment 8889316 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8889319 [details] [diff] [review] P3: Record padding size into cache.sqlite Review of attachment 8889319 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/DBSchema.cpp @@ -630,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > rv = state->Execute(); > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > - nit: Can you leave this blank line? @@ +1451,5 @@ > + // It's possible to have null padding size for non-opaque response > + rv = state->GetIsNull(3, &isNull); > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > + > + if (!isNull) { Should we set the value to UNKNOWN_SIZE if its NULL? Or to zero? I think we should probably set it to something in case the caller forgets to initialize it. @@ +2052,5 @@ > + int64_t paddingSize = 0; > + rv = state->GetInt64(6, &paddingSize); > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > + > + aSavedResponseOut->mValue.paddingSize() = paddingSize; Can we add a MOZ_DIAGNOSTIC_ASSERT() here that paddingSize is >= 0? ::: dom/cache/Manager.cpp @@ +786,5 @@ > } > } > > + // XXXtt: Write .padding file to the cache directory > + // UpdateDirectoryFile(mDBDir); Is this handled in a later patch?

(In reply to Ben Kelly [:bkelly] from comment #128) > ::: dom/cache/DBSchema.cpp > @@ -630,5 @@ > > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > > > rv = state->Execute(); > > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > - > > nit: Can you leave this blank line? Sure > @@ +1451,5 @@ > > + // It's possible to have null padding size for non-opaque response > > + rv = state->GetIsNull(3, &isNull); > > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > + > > + if (!isNull) { > > Should we set the value to UNKNOWN_SIZE if its NULL? Or to zero? I think > we should probably set it to something in case the caller forgets to > initialize it. Sorry, I'm not sure what you mean. I guess you were talking about what should I do in ReadResponse()? In ReadResponse(), if the column "response_padding_size" is null, it's set to UNKNOWN_SIZE. There is an assertion to ensure column is null only when the reading respnose is not type opaque. In DeleteEntries(), I don't think I need to set any value. I calculate the overall padding size to be removed and pass them out in order to decrease it from QM (P4). > @@ +2052,5 @@ > > + int64_t paddingSize = 0; > > + rv = state->GetInt64(6, &paddingSize); > > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > + > > + aSavedResponseOut->mValue.paddingSize() = paddingSize; > > Can we add a MOZ_DIAGNOSTIC_ASSERT() here that paddingSize is >= 0? Sure. > ::: dom/cache/Manager.cpp > @@ +786,5 @@ > > } > > } > > > > + // XXXtt: Write .padding file to the cache directory > > + // UpdateDirectoryFile(mDBDir); > > Is this handled in a later patch? Yes, I implement it in P10

Update P1 since fail on try (I should put assertion for setting padding size after set the response type)

Address comment except the quesion I put in comment 129

Update patches for padding file

Fix for try failure (mozilla::DebugOnly<bool> should use with MOZ_ASSERT)

(In reply to Tom Tung [:tt] from comment #129) > In ReadResponse(), if the column "response_padding_size" is null, it's set > to UNKNOWN_SIZE. There is an assertion to ensure column is null only when > the reading respnose is not type opaque. > > In DeleteEntries(), I don't think I need to set any value. I calculate the > overall padding size to be removed and pass them out in order to decrease it > from QM (P4). Can you at least set it to zero at the top of the method here? So that way if nothing with padding gets deleted zero is always returned. Otherwise we are depending on the caller initializing to zero first.

(In reply to Ben Kelly [:bkelly] from comment #142) > Can you at least set it to zero at the top of the method here? So that way > if nothing with padding gets deleted zero is always returned. Otherwise we > are depending on the caller initializing to zero first. Thanks! I see. I need to take care case [1] when the deleting entries are too big as well. [1] http://searchfox.org/mozilla-central/source/dom/cache/DBSchema.cpp#1359-1360

Update patch

Comment on attachment 8889857 [details] [diff] [review] P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. Hi Ben, Jan, The following patches are the implementation of the directory padding file. The file is stores in each DOMCache base directory (profile/storage/origin/cache/.padding). There are two types of padding file: temporary_file and file (.padding-tmp .padding) to make sure the content inside the file is correct (If the temporary file exists, it means the content inside the file is wrong). Since the current cache operations live in cache IO threads (per manager/origin) and CacheQuotaClient calculates the overall usage in Quota IO thread, I implement a mutex lock to protect the padding file, and it is held by CacheQuotaClient. The reason is CacheQuotaClient is initialized in [1], and the QM holds it. This ensures the mutex lock must live earlier and longer than the every executing cache runnable. [1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#3617 P6: For the existing cache directroy, I implement a major upgrade for QM from v2 to v3 to create a padding file with the 0 padding size inside. P7: Create the mutex lock in CacheQuotaClient, so that we can protect the padding file from multi-thread access P8: Implement utility functions in FileUtils.cpp to read/write/delete/check the padding file. P9: Implement a function to get overall padding size, so that we can restore the padding file. P10: The main logic for accessing padding file on cache operations (cache.put/cache.delete/...). P11: Add thread assertion on CacheQuotaClient to ensure its functions run as expected P12: Get the padding size in initOrigin() and getUsageForOrigin(). Besides, I rewrite the implementation inside, since it's easier to restore padding file in initOrigin() than getUsageForOrigin(). The reason is that is also guaranteed to be executed before every cache operations. P13: Access the database in initOrigin(), so that we can restore the file.