Closed Bug 1161063 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox-esr38 41+ 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

People

(Reporter: aosmond, Assigned: janv)

References

Details

(Keywords: csectype-race, sec-high, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 file, 2 obsolete files)

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.
Depends on: AsyncIDB
Group: core-security
Flags: needinfo?(bent.mozilla)
Jan, what do you think we should do here?
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(Jan.Varga)
I think we should add a main process/main thread check to StructuredCloneWriteCallback() when trying to serialize a mutable file.
Flags: needinfo?(Jan.Varga)
(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.
Attached patch patch (obsolete) — Splinter Review
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
Attached patch interdiff (obsolete) — Splinter Review
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?
(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?
(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.
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.
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.
(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.
Attached patch new patchSplinter Review
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
Closed: 4 years ago
Flags: needinfo?(Jan.Varga)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Unless 40 is unaffected...
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)
(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.
Flags: needinfo?(Jan.Varga)
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?
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 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+
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)
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
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.