579882 - IndexedDB: Investigate/review IndexedDB event handling

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

It seems like there could be some security problems because IDBTransaction
doesn't setup mScriptContext nor mOwner member variables.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Bugs in the IDB event handling may lead to similar problems as 
https://bugzilla.mozilla.org/show_bug.cgi?id=403168

Comment 2

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

Attachment: Patch, v1

Comment 3

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

Attachment: Patch, v1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 462960 [details] [diff] [review]
Patch, v1


>+IDBRequest::Generator::GenerateRequestInternal(JSContext* aCx,
>+                                               PRBool aWriteRequest)
> {
>-  NS_ASSERTION(aGenerator, "Null generator!");
>+  nsRefPtr<IDBRequest> request(new IDBRequest());
>+
>+  request->mGenerator = this;
>+  request->mWriteRequest = aWriteRequest;
>+
>+  if (!mLiveRequests.AppendElement(request)) {
>+    NS_ERROR("Append failed!");
>+    return nsnull;
>+  }
Shouldn't AppendElement happen after you've set request->mOwner


>+
>+  nsIScriptContext* context = GetScriptContextFromJSContext(aCx);
>+  if (context) {
>+    request->mScriptContext = context;
>+    nsCOMPtr<nsPIDOMWindow> window =
>+      do_QueryInterface(context->GetGlobalObject());
>+    if (window) {
>+      request->mOwner = window->GetCurrentInnerWindow();
>+    }
>+  }
Since this all can be used only in a web page context, we should, IMO
require that context and window are non-null.



>@@ -95,6 +96,17 @@ IDBTransaction::Create(IDBDatabase* aDat
>     return nsnull;
>   }
> 
>+  nsIScriptContext* context = GetScriptContextFromJSContext(aCx);
>+  if (context) {
>+    transaction->mScriptContext = context;
>+    nsCOMPtr<nsPIDOMWindow> window =
>+      do_QueryInterface(context->GetGlobalObject());
>+    if (window) {
>+      transaction->mOwner = window->GetCurrentInnerWindow();
>+    }
>+  }
Same here.

But since I'm not too familiar with this code, could someone who has
reviewed this code read this too.

Comment 5

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

Sicking, review ping!

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 462960 [details] [diff] [review]
Patch, v1

I don't think we need to remember the context on such a granular level. Lets just stick it in each transaction and reuse the information there for all requests. There should be no way for a transaction object to be passed across origins so there should be no risk of security problems due to using the wrong context.

Comment 7

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

Attachment: Patch, v2

Ok, this does that.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It looks like you're still sticking the context/window into each request. Why not just grab it out of the transaction when you're firing the event instead? (with exception of the factory.open request)

Comment 9

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

(In reply to comment #8)
> (with exception of the factory.open request)

It's not just factory.open... It's also create/removeObjectStore and create/RemoveIndex. Those will inherit a setVersion transaction someday soon, but not yet.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The first two changes in the patch that I see are for IDBCursor::Update and IDBCursor::Remove

Comment 11

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

Right. I meant, it's not just factory.open that can't use a transaction as the argument to the request.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why do you have to change the two function in comment 10, why can't the request created there get the needed objects from the transaction as they fire events?

Comment 13

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

Comment on attachment 467847 [details] [diff] [review]
Patch, v2

Need this to make sure we don't let IndexedDB become another source of XSS bugs.

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/a40d9d482ae7

Comment 15

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

Can we open this?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes