Closed Bug 579882 Opened 14 years ago Closed 14 years ago

IndexedDB: Investigate/review IndexedDB event handling

Categories

(Core :: Storage: IndexedDB, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: smaug, Assigned: bent.mozilla)

References

Details

(Whiteboard: [sg:audit])

Attachments

(1 file, 2 obsolete files)

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: IndexedDB
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
Attached patch Patch, v1 (obsolete) — Splinter Review
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)
Attached patch Patch, v1 (obsolete) — Splinter Review
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-
Attached patch Patch, v2Splinter Review
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+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: DOM → DOM: IndexedDB
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: