IndexedDB: Investigate/review IndexedDB event handling

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: smaug, Assigned: bent.mozilla)

Tracking

unspecified
x86_64
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [sg:audit])

Attachments

(1 attachment, 2 obsolete attachments)

It seems like there could be some security problems because IDBTransaction
doesn't setup mScriptContext nor mOwner member variables.
Summary: Investigate/review IndexDB event handling → Investigate/review IndexedDB event handling
Blocks: 553412

Updated

9 years ago
Whiteboard: [sg:audit]
Bugs in the IDB event handling may lead to similar problems as 
https://bugzilla.mozilla.org/show_bug.cgi?id=403168
Summary: Investigate/review IndexedDB event handling → IndexedDB: Investigate/review IndexedDB event handling
blocking2.0: ? → final+
Assignee: nobody → bent.mozilla
Created attachment 462917 [details] [diff] [review]
Patch, v1
Attachment #462917 - Flags: review?(jonas)
Attachment #462917 - Flags: review?(Olli.Pettay)
Attachment #462917 - Attachment is obsolete: true
Attachment #462917 - Flags: review?(jonas)
Attachment #462917 - Flags: review?(Olli.Pettay)
Created attachment 462960 [details] [diff] [review]
Patch, v1
Attachment #462960 - Flags: review?(jonas)
Attachment #462960 - Flags: review?(Olli.Pettay)
Attachment #462960 - Attachment is patch: true
Attachment #462960 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #462960 - Flags: review?(Olli.Pettay) → review+
Sicking, review ping!
Status: NEW → ASSIGNED
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.
Attachment #462960 - Flags: review?(jonas) → review-
Created attachment 467847 [details] [diff] [review]
Patch, v2

Ok, this does that.
Attachment #462960 - Attachment is obsolete: true
Attachment #467847 - Flags: review?(jonas)
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)
(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.
The first two changes in the patch that I see are for IDBCursor::Update and IDBCursor::Remove
Right. I meant, it's not just factory.open that can't use a transaction as the argument to the request.
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 on attachment 467847 [details] [diff] [review]
Patch, v2

Need this to make sure we don't let IndexedDB become another source of XSS bugs.
Attachment #467847 - Flags: approval2.0?
Attachment #467847 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/a40d9d482ae7
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.