Getting a stored MutableFile out of IndexedDB on a worker doesn't work

RESOLVED FIXED in Firefox 41, Firefox OS v2.2

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aosmond, Assigned: janv)

Tracking

({csectype-race, sec-high})

unspecified
mozilla41
csectype-race, sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox-esr3841+ fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 attachment, 2 obsolete attachments)

7.61 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
[PTO until July 27]
: approval-mozilla-esr38+
[PTO until July 27]
: sec-approval+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
While studying how web workers were implemented for IndexedDB, I believe I found a threading violation where a DOM method is executed on a worker but can trigger a method that can only be executed on the main thread. The call stack would be as follows:

IDBObjectStore::Add || IDBObjectStore::Put -- exposed on WebIDL
IDBObjectStore::AddOrPut -- asserts owning thread (due to Exposed=Worker, may not be main thread)
IDBObjectStore::GetAddInfo
[ KeyPath::ExtractOrCreateKey -> GetJSValFromKeyPathString ] -- optional
IDBObjectStore::GetAddInfoCallback
JSAutoStructuredCloneBuffer::write
JS_WriteStructuredClone
WriteStructuredClone
JSStructuredCloneWriter::write
JSStructuredCloneWriter::startWrite -- calls callback->write, see below
StructuredCloneWriteCallback
IDBDatabase::GetQuotaInfo -- asserts main thread

This looks to be a corner case; we are trying to store a mutable file where our reference to the database for the write mismatches that of the file. Hopefully I did not miss a dispatch to the main thread somewhere along the way.
(Reporter)

Updated

3 years ago
Depends on: 701634
Group: core-security
Flags: needinfo?(bent.mozilla)
Jan, what do you think we should do here?
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(Jan.Varga)
(Assignee)

Comment 2

3 years ago
I think we should add a main process/main thread check to StructuredCloneWriteCallback() when trying to serialize a mutable file.
Flags: needinfo?(Jan.Varga)
(Assignee)

Comment 3

3 years ago
(In reply to Andrew Osmond [:aosmond] from comment #0)
> While studying how web workers were implemented for IndexedDB, I believe I
> found a threading violation where a DOM method is executed on a worker but
> can trigger a method that can only be executed on the main thread. The call
> stack would be as follows:
> 
> ...
> 
> This looks to be a corner case; we are trying to store a mutable file where
> our reference to the database for the write mismatches that of the file.
> Hopefully I did not miss a dispatch to the main thread somewhere along the
> way.

Is this just a theory or you have a test case for it ?
AFAIK, you can't get a mutable file on workers, so IDBObjectStore::Add on workers can't ever get it.

However I do see a problem elsewhere, when we deserialize data we don't handle mutable files correctly.
I have a fix for it.
(Assignee)

Comment 4

3 years ago
Created attachment 8601407 [details] [diff] [review]
patch
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Attachment #8601407 - Flags: review?(bent.mozilla)
Comment on attachment 8601407 [details] [diff] [review]
patch

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

Thanks!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +588,5 @@
> +      MOZ_ASSERT(mutableFile);
> +
> +      result = mutableFile->WrapObject(aCx, JS::NullPtr());
> +    } else {
> +      result = JS_NewObject(aCx, nullptr);

I think this should just return false, and we should log a warning in the error console
Attachment #8601407 - Flags: review?(bent.mozilla) → review+
Summary: IDBObjectStore::Add/Put may indrectly call main thread only method (IDBDatabase::GetQuotaInfo) on worker thread when adding mutable file → IDBObjectStore::Add/Put may indirectly call main thread only method (IDBDatabase::GetQuotaInfo) on worker thread when adding mutable file
(Assignee)

Comment 6

3 years ago
Created attachment 8602321 [details] [diff] [review]
interdiff

As we talked on IRC, NS_ERROR_DOM_DATA_CLONE_ERR needs to be whitelisted in IDBRequest::SetError()
Attachment #8602321 - Flags: review?(bent.mozilla)
Comment on attachment 8602321 [details] [diff] [review]
interdiff

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

Ok
Attachment #8602321 - Flags: review?(bent.mozilla) → review+
Although, maybe it would be nicer if we treated the filehandle as a blob if you somehow tried to get one out of a database on a worker?
(Assignee)

Comment 9

3 years ago
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #8)
> Although, maybe it would be nicer if we treated the filehandle as a blob if
> you somehow tried to get one out of a database on a worker?

Such blob would need to block write operations scheduled on the main thread. I'm not not sure if it's worth it.
Comment on attachment 8602321 [details] [diff] [review]
interdiff

I was too hasty here. We need better behavior because returning undefined and setting an error on the request is just bizarre and unprecedented.
Attachment #8602321 - Flags: review+ → review-
Updating the summary to cover the case that's actually a problem here.
Summary: IDBObjectStore::Add/Put may indirectly call main thread only method (IDBDatabase::GetQuotaInfo) on worker thread when adding mutable file → Getting a stored MutableFile out of IndexedDB on a worker doesn't work
Can we make this throw an exception when touching the 'result' property?
(Assignee)

Comment 13

3 years ago
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #12)
> Can we make this throw an exception when touching the 'result' property?

Ok, I'll try it.
(Assignee)

Comment 14

3 years ago
IndexedDB spec says:

3.2.1 The IDBRequest Interface

...

result of type any, readonly 

When the done flag is true, getting this property MUST return the result of the request. This is undefined when the request resulted in an error. When the done flag is false, getting this property MUST throw a DOMException of type InvalidStateError.



So, I'm not sure if we really want to change that to throw an exception even when the done flag is true.
Keywords: csectype-race, sec-high
Jan, have you had a chance to look at this again?
(In reply to Jan Varga [:janv] from comment #14)
> IndexedDB spec says:

I wouldn't worry about the spec here. We shouldn't ever have a case where we can't deserialize a value, so there's no reason for the spec to cover such a case. This is our own doing so we need to cover it as best we can.
(Assignee)

Comment 17

3 years ago
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #16)
> (In reply to Jan Varga [:janv] from comment #14)
> > IndexedDB spec says:
> 
> I wouldn't worry about the spec here. We shouldn't ever have a case where we
> can't deserialize a value, so there's no reason for the spec to cover such a
> case. This is our own doing so we need to cover it as best we can.

Ok, let me update the patch.
We're getting near the end of beta, and it would nice to get this fixed soon.
(Assignee)

Comment 19

3 years ago
Created attachment 8620920 [details] [diff] [review]
new patch

This is the simplest (and safest) I can do here. The exception is thrown when touching the 'result' property but an error is not set. For that I would have to split mHaveResultOrErrorCode into mHaveResult and mHaveErrorCode or something like that.
Attachment #8601407 - Attachment is obsolete: true
Attachment #8602321 - Attachment is obsolete: true
Attachment #8620920 - Flags: review?(bent.mozilla)
Comment on attachment 8620920 [details] [diff] [review]
new patch

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

Thanks!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +583,2 @@
>  
> +    if (IndexedDatabaseManager::IsMainProcess() && NS_IsMainThread()) {

How about:

  if (!condition) {
    return false;
  }

  // other stuff

::: dom/indexedDB/IDBRequest.cpp
@@ +361,5 @@
>  
>    JS::Rooted<JS::Value> result(cx);
>    nsresult rv = aCallback->GetResult(cx, &result);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return;

Should this only happen if the error is NS_ERROR_DOM_DATA_CLONE_ERR?

In any case this needs a big comment explaining why we're not setting an error object, and how eventually it means we will throw an exception

::: dom/indexedDB/test/test_filehandle_workers.html
@@ +88,5 @@
>        is(e.code, DOMException.DATA_CLONE_ERR, "Good error code.")
>      }
>      worker.terminate();
>  
> +    URL.revokeObjectURL(url);

Call this right after |new Worker()|

@@ +134,5 @@
> +
> +    is(event.data, "success", "Good response.");
> +    worker.terminate();
> +
> +    URL.revokeObjectURL(url);

here too
Attachment #8620920 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/be976c7cb4b1

This should have had security approval before landing IIUC (given references in the bug to the beta cycle, I assume you intend to backport this). Please request it retroactively ASAP and get branch approval nominations up.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
status-firefox41: --- → fixed
Flags: needinfo?(Jan.Varga)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Comment 23

3 years ago
Unless 40 is unaffected...
(Assignee)

Comment 24

3 years ago
Sorry for the delay, aurora and beta now contain the fix, but I think the current release 40 is affected. Should I request an approval for that ?
Flags: needinfo?(Jan.Varga)
We still need to know if this affects older releases as we still support Gecko <41.
Flags: needinfo?(Jan.Varga)
(Assignee)

Comment 26

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> We still need to know if this affects older releases as we still support
> Gecko <41.

Yes, it does affect older releases.
Flags: needinfo?(Jan.Varga)
Sounds like we probably need a retroactive sec-approval request and an esr38 approval request then.
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-firefox40: --- → wontfix
status-firefox-esr38: --- → affected
Flags: needinfo?(Jan.Varga)
tracking-firefox-esr38: --- → ?
(Assignee)

Comment 28

3 years ago
Comment on attachment 8620920 [details] [diff] [review]
new patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Already marked as sef-high.

User impact if declined: Main thread only methods get executed off main thread which can lead to variety of problems (read crashes).

Fix Landed on Version: The original patch landed on 2015-06-11, mozilla 41 at that time.

Risk to taking this patch (and alternatives if risky): Should be pretty safe, it's been on trunk/aurora/beta for a while.

String or UUID changes made by this patch: None
Flags: needinfo?(Jan.Varga)
Attachment #8620920 - Flags: approval-mozilla-esr38?
(Assignee)

Comment 29

3 years ago
Comment on attachment 8620920 [details] [diff] [review]
new patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very easy. One could just store a mutable file on the main thread and then open the same database on a DOM worker and try to get the mutable file.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yeah, the test and conversion of main thread assertions to checks make it obvious I think.

Which older supported branches are affected by this flaw?
We probably had this problem for long time, I believe it was exposed by enabling IndexedDB on workers which goes back to 37.

If not all supported branches, which bug introduced the flaw?
bug 701634 (indirectly)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I'm going to build an ESR with the patch to make sure it works correctly. If the patch slightly differs (doesn't apply cleanly), I'll attach a rebased patch for it.

How likely is this patch to cause regressions; how much testing does it need?
Since this is a retroactive sec-approval and the patch has been on trunk/aurora/beta for a while I guess it's pretty safe.
Attachment #8620920 - Flags: sec-approval?

Comment 30

3 years ago
Comment on attachment 8620920 [details] [diff] [review]
new patch

Giving approvals. I wish this would have gotten sec-approval before landing and landed without tests. We don't normally check in tests for security until after the fix is released.
Attachment #8620920 - Flags: sec-approval?
Attachment #8620920 - Flags: sec-approval+
Attachment #8620920 - Flags: approval-mozilla-esr38?
Attachment #8620920 - Flags: approval-mozilla-esr38+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/16d864d163de
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/16d864d163de
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: affected → fixed
status-b2g-v2.2r: affected → fixed
Hi Andrew, would you mind taking a look at a build of Fx41 and see if the issue has been fixed for you? Thanks.
Flags: needinfo?(aosmond)

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
tracking-firefox-esr38: ? → 41+

Updated

3 years ago
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
(Reporter)

Comment 34

3 years ago
Looks good to me, although I think my original filing was not what was fixed as that wasn't a problem, and it merely got the right eyes onto a related area which was a problem :).
Flags: needinfo?(aosmond)
Can we open this up now?
Flags: needinfo?(dveditz)
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.