Make indexDB CreateStorageConnection get file protocol handler directly

RESOLVED FIXED in Firefox 51

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

({csectype-race, sec-moderate})

Trunk
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

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

Attachments

(1 attachment)

Assignee

Description

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
Assignee

Updated

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

Updated

3 years ago
Group: core-security
Assignee

Updated

3 years ago
Group: network-core-security
Assignee

Updated

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

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+
Assignee

Updated

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
Assignee

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).
https://hg.mozilla.org/mozilla-central/rev/2e4d0d597bce
Status: ASSIGNED → RESOLVED
Closed: 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.