Closed Bug 1434308 Opened 2 years ago Closed 2 years ago
DB uses Same Process Same Thread structured clone scope
When looking into bug 1264642, I noticed that IndexedDB uses SameProcessSameThread in its DeserializeValue. Shouldn't that be DifferentProcess? Isn't this reading data off of disk? https://searchfox.org/mozilla-central/source/dom/indexedDB/IDBObjectStore.cpp#1333 I don't *think* this should be a security problem since I think we trust the IndexedDB data to not be attacker-controlled, and so should never have Transferable pointers or other pointers embedded, but I'm starting this bug out secure in case I'm wrong.
> https://searchfox.org/mozilla-central/source/dom/indexedDB/IDBObjectStore. > cpp#1333 You are right. It's not a real problem here because SharedBuffers are disabled and we don't support transferring of ArrayBuffer in IDB, but would be better to have DifferentProcess.
Assignee: nobody → amarchesini
Attachment #8946771 - Flags: review?(sphink)
Comment on attachment 8946771 [details] [diff] [review] idb.patch Review of attachment 8946771 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I don't see an attack vector.
Attachment #8946771 - Flags: review?(sphink) → review+
landing. This bug is sec-low.
This is actually interesting failure: Currently JS Structured clone algorirthm checks if the reading target scope and the written target scope match. If we apply this patch, IDB reading old data fails. I see three approaches: 1. keep the current code. We don't actually use any of the scope-related features (sharedArrayBuffer and transferring of arrayBuffer). 2. add a flag in JS_StructuredClone to ignore the changing of the scope. In this way IDB will continue to work when we are migrating DifferentProcess scope. 3. implement an IDB migration. asuth, feedback? I personally prefer the second option.
Flags: needinfo?(amarchesini) → needinfo?(bugmail)
(In reply to Andrea Marchesini [:baku] from comment #6) > This is actually interesting failure: Currently JS Structured clone > algorirthm checks if the reading target scope and the written target scope > match. If we apply this patch, IDB reading old data fails. For nit-picky clarity, as I read the comparison, it's `if (storedScope < allowedScope)`, where storedScope is from the serialized data and allowedScope is what we passed in. So this allows allowedScope=SameProcessSameThread to read data that was stored as storedScope=DifferentProcess. > 2. add a flag in JS_StructuredClone to ignore the changing of the scope. In > this way IDB will continue to work when we are migrating DifferentProcess > scope. I like this option too. I think given that the HTML spec 2.7 Safe passing of structured data now recognizes an explicit `forStorage` flag in 2.7.3 StructuredSerializeInternal, it makes sense to add StructuredCloneScope::SerializeForStorage and have the check explicitly allow this case of storedScope=SameProcessSameThread and allowedScope=SerializeForStorage. We'd also make sure there's a big comment like: For a long time, IndexedDB used SameProcessSameThread. IndexedDB upgrades are big hassle and also lead to serious downgrade problems, so we're explicitly allowing this and only this mismatch. 1: https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/js/src/vm/StructuredClone.cpp#2396 2: https://html.spec.whatwg.org/multipage/structured-data.html#safe-passing-of-structured-data 3: https://html.spec.whatwg.org/multipage/structured-data.html#structuredserializeinternal
Would that mean that the next time we need an IndexedDB upgrade for some other reason, we could get rid of this exception? That sounds fine to me. Other than handling this specific case, is there any difference between DifferentProcess and SerializeForStorage? I haven't been watching progress on the spec. It's weird to see how much it has changed to match our code. Except it has better names for things.
(In reply to Steve Fink [:sfink] [:s:] from comment #8) > Would that mean that the next time we need an IndexedDB upgrade for some > other reason, we could get rid of this exception? If the upgrade process completely re-wrote all the structured clones, yes. There isn't actually any existing logic to safely deserialize then reserialize everything, and if we added it, it would all have to happen on the parent process main thread unless we did a ton of new development. The easier solution would be if there's a simple byte manipulation we could perform via SQL, but it's still not that easy because as an optimization we store very large structured clones outside the database in separate blobs. (Still, less bad that deserialize-re-serialize.) And the problem with that is it introduces a much larger downgrade cliff. So, to summarize the current snafu re: bug 1246615 (profile from the future), sometimes users use the same profile across multiple channels for a variety of reasons, and until bug 1373244 is fixed so that different release channels have their own profile slot in profile.ini (like dev edition), this leads to people effectively upgrading and then downgrading in their profile, possibly multiple times. For example, when we were pushing non-dev-edition Firefox 57 beta to a lot of people, MoCo director-level people got their profiles harshed. If we change the code at all to persist anything other than SameProcessSameThread, we will be introducing a small downgrade cliff. Any put()/add() performed in IDB in a "future" profile will fail to deserialize in a "past" browser. If we wanted to avoid any downgrade cliff, we might do a stage rollout of this change: - When writing, write SameProcessSameThread for SerializeForStorage. But for reading, use the SerializeForStorage logic with its rules that allows SameProcessSameThread. - Let that ship until the logic is in the only currently supported ESR release. - Switch to writing SerializeForStorage, but still leave the carve-out. - Let that ship until it's the only currently supported ESR release. - Remove the carve-out. Obviously that's a lot of work and annoying, and hopefully bug 1373244 improves things a lot for everyone before all those years pass. Having thought about all of this a bit more thanks to your question, I'm somewhat tempted to suggested that we leave the status quo as-is until either bug 1373244 lands, allowing us to be less fearful of downgrades, or until we fix IDB and QuotaManager to handle "IDB from the future" better so that we can safely make this change with an accompanying change to the IDB schema version. We'd just end up blowing away the database on "profile from the future" instead of either throwing errors on "key/value from the future" or failure to open "database from the future". NI'ing :janv for his thoughts on this. The change in the JS code is small, but the downgrade impacts are large. > Other than handling this specific case, is there any difference between > DifferentProcess and SerializeForStorage? There should be in the future, there are specific WebIDL rules about [Serializable] and we want to get the spec and our implementation fixed up for that. Bug 1350254 covers that.
Sorry, I didn't mean to trigger that much work! This seems like a pretty minor issue, and shouldn't be the cause for a huge upgrade ordeal or anything. I just sort of assumed that upgrades happened by rewriting all stored clones, but it makes sense that they don't. I have no objection to the proposed #2 solution, and it doesn't sound like it's worth doing anything fancier just for this. It would be nice to at least remove the potential footgun that we have while we're still writing out SameProcessSameThread clones, given that the scope already influences the format of the data. (For now, it doesn't do anything that matters, as the only things that change -- SharedArrayBuffers and Transferred ArrayBuffers -- are prevented from being used by other mechanisms IIUC.) But option #2 does that.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2692e7aaefbd "IndexedDB uses SameProcessSameThread structured clone scope" [r=sphink]
Backout by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/85a77aec89fb Backed out changeset 2692e7aaefbd
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/07b3234cc1c0 "IndexedDB uses SameProcessSameThread structured clone scope" [r=sphink]
(This became safe by the concurrent landing of fixes on bug 1433642 temporarily eliminating the check that gets upset. See hunk at https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bbc2e04472#l2.86)
Depends on: 1433642
And for my personal future benefit, restating why this is all safe: The check removed by bug 1433642 allowed SameProcessSameThread to read DifferentProcess-serialized data because DifferentProcess was safer (no pointers). The opposite scenario is not safe because DifferentProcess reading SameProcessSameThread-serialized data could have pointers that will be corrupt/unusable in the reader's potentially different process. Firefox 61: Reads/writes DifferentProcess, doesn't check. Firefox 60: Reads/writes SameProcessSameThread, does check. So when Firefox 60 reads a 61-written structured clone, the check is: if (storedScope < allowedScope) which becomes: if (DifferentProcess < SameProcessSameThread) which roughly evaluates to (2 < 0) == false, so there is no downgrade cliff. When Firefox 61 reads a 60-written structured clone, we don't check, which is good because the pre-patch check would throw. (Which is why the comment 5 backout happened. We had test coverage that used a previously-generated IndexedDB database.)
Sorry, I should have explained what's going on here better. But you got it all, from what I can tell! Also note that bug 1456604 intends to add the check back in, using approach #2 from comment 6 (so it will still allow through the stored IndexedDB clones.)
You need to log in before you can comment on or make changes to this bug.