Make indexDB CreateStorageConnection get file protocol handler directly

RESOLVED FIXED in Firefox 51



3 years ago
2 years ago


(Reporter: mayhemer, Assigned: mayhemer)


({csectype-race, sec-moderate})

Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)


(Whiteboard: [post-critsmash-triage][adv-main51+])


(1 attachment)



3 years ago
Example, on "Storage I/O" thread:

 	xul.dll!nsWeakReference::QueryReferent({...}, 0x1e6fefac) Line 127	C++
 	xul.dll!CallQueryReferent<nsIWeakReference,nsIProtocolHandler>(0x0ec0aac0, 0x1e6fefac) Line 30	C++
>	xul.dll!mozilla::net::nsIOService::GetCachedProtocolHandler(0x09e1700c, 0x1e6fefac, 0, 0) Line 495	C++
 	xul.dll!mozilla::net::nsIOService::GetProtocolHandler(0x09e1700c, 0x1e6fefac) Line 511	C++
 	xul.dll!mozilla::net::nsIOService::NewFileURI(0x11ab8760, 0x1e6ff094) Line 654	C++
 	xul.dll!NS_NewFileURI(0x1e6ff094, 0x11ab8760, 0x01067420) Line 147	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::GetDatabaseFileURL(0x11ab8760, PERSISTENCE_TYPE_PERSISTENT, {...}, {...}, 0, 0x1e6ff40c) Line 4115	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::CreateStorageConnection(0x11ab8760, 0x1283b1c0, {...}, PERSISTENCE_TYPE_PERSISTENT, {...}, {...}, 0, 0x1e6ff5c4) Line 4415	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::DoDatabaseWork() Line 20855	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::FactoryOp::Run() Line 20612	C++
 	xul.dll!nsThread::ProcessNextEvent(false, 0x1e6ff7d1) Line 1058	C++
 	xul.dll!NS_ProcessNextEvent(0x1e2d2740, false) Line 290	C++

This is a result of run with the patch from bug 956338.

Solution: let the indexedDB create the URL on the main thread (not ideal too.. our URLs are not threadsafe as well..)

Other, more universal way is to have a really thread safe weak reference implementation that can be used here.  Still, nsIOService::mWeakHandler array would also have to be protected by a mutex.
Group: core-security → network-core-security


3 years ago
Assignee: nobody → honzab.moz
Group: network-core-security
Summary: nsIOService::mWeakHandler array items used on multiple threads → Let indexDB CreateStorageConnection grab file handler on the main thread


3 years ago
Group: core-security


3 years ago
Group: network-core-security


3 years ago
Summary: Let indexDB CreateStorageConnection grab file handler on the main thread → Make indexDB CreateStorageConnection get file protocol handler directly

Comment 1

3 years ago
Problem here is that NS_NewFileUri goes to the IO service that inspects its internal weak array cache.  That array can only be access on the main thread, since 1) weak references are not thread safe 2) the array is not guarded.

Hence, getting the file protocol directly as a service is the correct thread safe way here.
Attachment #8779340 - Flags: review?(bent.mozilla)
Comment on attachment 8779340 [details] [diff] [review]
v1 (grab file protocol directly, not from IO service weak cache)

Jan, can you take this?
Attachment #8779340 - Flags: review?(bent.mozilla) → review?(jvarga)

Comment 3

3 years ago
Comment on attachment 8779340 [details] [diff] [review]
v1 (grab file protocol directly, not from IO service weak cache)

Review of attachment 8779340 [details] [diff] [review]:

Looks good, r=me

::: dom/indexedDB/ActorsParent.cpp
@@ +70,5 @@
>  #include "nsIAppsService.h"
>  #include "nsIEventTarget.h"
>  #include "nsIFile.h"
>  #include "nsIFileURL.h"
> +#include "nsIFileProtocolHandler.h"

Nit: should go before nsIFileURL.h

@@ +4120,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIFileProtocolHandler> fileHandler(
> +    do_QueryInterface(protocolHandler, &rv));

Nit: we seem to never check rv of do_QueryInterface, the check below can be just |if (NS_WARN_IF(!fileHandler)) ...|
Attachment #8779340 - Flags: review?(jvarga) → review+


3 years ago
Keywords: checkin-needed
This needs a security rating before it can be checked in. How bad of a security issue do you think this is in practice?
Flags: needinfo?(honzab.moz)
Keywords: csectype-race
Keywords: sec-audit
Flags: needinfo?(honzab.moz)
Group: network-core-security, core-security → dom-core-security
Component: Networking → DOM: IndexedDB
Keywords: sec-auditsec-moderate

Comment 6

3 years ago
only real problem here is concurrent access on the weak cache array in IO service, nothing else.  that's probably not likely to be exploitable.

use-after-free is of a very low if even any chance at all, since file protocol handler is a service and lives as long as xpcom.  and the code we are fixing here is triggered just during startup (AFAIU).
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.