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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jan, what do you think we should do here?

Comment 2

[Jan Varga [:janv]]

I think we should add a main process/main thread check to StructuredCloneWriteCallback() when trying to serialize a mutable file.

Comment 3

[Jan Varga [:janv]]

(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.

Comment 4

[Jan Varga [:janv]]

Attachment: patch

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 6

[Jan Varga [:janv]]

Attachment: interdiff

As we talked on IRC, NS_ERROR_DOM_DATA_CLONE_ERR needs to be whitelisted in IDBRequest::SetError()

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8602321 [details] [diff] [review]
interdiff

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

Ok

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 9

[Jan Varga [:janv]]

(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 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Updating the summary to cover the case that's actually a problem here.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Can we make this throw an exception when touching the 'result' property?

Comment 13

[Jan Varga [:janv]]

(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.

Comment 14

[Jan Varga [:janv]]

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.

Comment 15

[Andrew McCreight [:mccr8]]

Jan, have you had a chance to look at this again?

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 17

[Jan Varga [:janv]]

(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.

Comment 18

[Andrew McCreight [:mccr8]]

We're getting near the end of beta, and it would nice to get this fixed soon.

Comment 19

[Jan Varga [:janv]]

Attachment: 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.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 21

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be976c7cb4b1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Unless 40 is unaffected...

Comment 24

[Jan Varga [:janv]]

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 ?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

We still need to know if this affects older releases as we still support Gecko <41.

Comment 26

[Jan Varga [:janv]]

(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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Sounds like we probably need a retroactive sec-approval request and an esr38 approval request then.

Comment 28

[Jan Varga [:janv]]

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

Comment 29

[Jan Varga [:janv]]

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.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/737875ff03ed

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/16d864d163de
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/16d864d163de

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Andrew, would you mind taking a look at a build of Fx41 and see if the issue has been fixed for you? Thanks.

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

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 :).

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we open this up now?