Closed Bug 1434308 Opened 6 years ago Closed 6 years ago

IndexedDB uses SameProcessSameThread structured clone scope

Categories

(Core :: Storage: IndexedDB, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: sfink, Assigned: baku)

References

Details

(Keywords: sec-audit, Whiteboard: [adv-main61-])

Attachments

(1 file)

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.
Attached patch idb.patchSplinter Review
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.
Backed out for failing xpcshell at dom/indexedDB/test/unit/test_wasm_recompile.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6802806330e7931513022c38f721be9cb6e8c0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a5214fb4d5c755731ad4fab8e72f22ccafe507ca&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159413372&repo=mozilla-inbound

13:25:01     INFO -  TEST-START | xpcshell.ini:dom/indexedDB/test/unit/test_wasm_recompile.js
13:30:01  WARNING -  TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:dom/indexedDB/test/unit/test_wasm_recompile.js | Test timed out
13:30:01     INFO -  TEST-INFO took 300003ms
13:30:01     INFO -  >>>>>>>
13:30:01     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
13:30:01     INFO -  (xpcshell/head.js) | test pending (2)
13:30:01     INFO -  (xpcshell/head.js) | test pending (3)
13:30:01     INFO -  (xpcshell/head.js) | test MAIN run_test finished (3)
13:30:01     INFO -  running event loop
13:30:01     INFO -  "Installing profile"
13:30:01     INFO -  (xpcshell/head.js) | test finished (2)
13:30:01     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
13:30:01     INFO -  "Reading out contents of compiled blob"
13:30:01     INFO -  "Opening database"
13:30:01     INFO -  "Getting wasm"
13:30:01     INFO -  PID 6883 | JavaScript error: /Users/cltbld/tasks/task_1517345916/build/tests/xpcshell/head.js, line 224: bad serialized structured data (incompatible structured clone scope)
13:30:01     INFO -  "Verifying wasm module"
13:30:01     INFO -  PID 6883 | JavaScript error: /Users/cltbld/tasks/task_1517345916/build/tests/xpcshell/tests/dom/indexedDB/test/unit/test_wasm_recompile.js, line 81: InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable
13:30:01     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "bad serialized structured data (incompatible structured clone scope)" {file: "/Users/cltbld/tasks/task_1517345916/build/tests/xpcshell/head.js" line: 224}]"
13:30:01     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" {file: "/Users/cltbld/tasks/task_1517345916/build/tests/xpcshell/tests/dom/indexedDB/test/unit/test_wasm_recompile.js" line: 81}]"
13:30:01     INFO -  <<<<<<<

Had landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/a5214fb4d5c755731ad4fab8e72f22ccafe507ca before.
Flags: needinfo?(amarchesini)
Group: core-security → dom-core-security
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)`[1], 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[2] now recognizes an explicit `forStorage` flag in 2.7.3 StructuredSerializeInternal[3], 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
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(jvarga)
Priority: -- → P1
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.
Group: dom-core-security
Keywords: sec-low
Priority: P1 → P2
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2692e7aaefbd
"IndexedDB uses SameProcessSameThread structured clone scope" [r=sphink]
Pushed by sfink@mozilla.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
Flags: needinfo?(jvarga)
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.)
https://hg.mozilla.org/mozilla-central/rev/07b3234cc1c0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Keywords: sec-lowsec-audit
Whiteboard: [adv-main61-]
You need to log in before you can comment on or make changes to this bug.