Closed
Bug 1505678
(CVE-2019-9799)
Opened 6 years ago
Closed 6 years ago
ExternalHelperAppParent doesn't validate arguments (potential sandbox issue)
Categories
(Core :: Networking, defect, P3)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla67
People
(Reporter: pauljt, Assigned: Alex_Gaynor)
References
Details
(Keywords: sec-high, Whiteboard: [necko-triaged][post-cristsmash-triage][adv-main66+])
I've been looking for variants of bug 1438425, using static analysis to find similar cases.I wrote Semmle query for parent IPDL handlers which take an nsCString parameter called "data". Mostly these related to remoting various nsiStreamListeners. Overall the architecture concerns me: there seems to be a lot of parameters passed back and forth between parent and child, and there are a lot of paramters which are just taken at face value in the parent. BUT I haven't found a concrete issue yet, so I'm filing this to get other opinions.
Take ExternalHelperAppParent::RecvOnDataAvailable[1] as an example. It receives `nsCString& data`, and also an offset and count from the child. These are passed into:
DebugOnly<nsresult> rv = NS_NewByteInputStream(getter_AddRefs(stringStream), data.get(), count, NS_ASSIGNMENT_DEPEND);
It's hard to trace, but as far as I can tell, these values end up in nsExternalAppHandler::OnDataAvailable [2], where the stream is written to a temporary file. Now this on it's own isn't too serious - you can maybe stream a bunch of memory into a file. But thats a guess, and if you can then read the file, then we have at least an info leak.
My more general question is: "is it safe to pass untrusted length values to NS_NewByteInputStream. If the answer is no, then there are half a dozen other places that we need to add checks (Recv*DataAvailable mostly). And I assume the fix here is just to check that the count < data.length()
[1] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/uriloader/exthandler/ExternalHelperAppParent.cpp#156
[2] https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/uriloader/exthandler/nsExternalHelperAppService.cpp#1939
[3] https://searchfox.org/mozilla-central/search?q=Recv.*dataAvailable&case=false®exp=true&path=parent
Reporter | ||
Updated•6 years ago
|
Depends on: semmle-analysis
Reporter | ||
Updated•6 years ago
|
Blocks: semmle-analysis
No longer depends on: semmle-analysis
Reporter | ||
Comment 1•6 years ago
|
||
PS I wonder if this is related to bug 1471312 - given that its the same function.
Reporter | ||
Comment 2•6 years ago
|
||
(I'm not linking, since that bug is public)
Assignee | ||
Comment 3•6 years ago
|
||
This is not related to that bug, that's a boring null-ptr-deref if you call this method before something has been initialized.
Updated•6 years ago
|
Comment 4•6 years ago
|
||
(In reply to Paul Theriault [:pauljt] from comment #0)
> My more general question is: "is it safe to pass untrusted length values to
> NS_NewByteInputStream. If the answer is no, then there are half a dozen
> other places that we need to add checks (Recv*DataAvailable mostly). And I
> assume the fix here is just to check that the count < data.length()
From what I can tell this is not safe, and we really should add these checks.
Flags: needinfo?(valentin.gosu)
Comment 5•6 years ago
|
||
As far as ExternalHelperAppParent::RecvOnDataAvailable is concerned, we don't need to send offset and count args at all. Those can be easily kept track of (offset) and calculated from the string (count).
- offset is the sum of all |count|s before:
OnDataAvailable(offset = 0, count = 100)
OnDataAvailable(offset = 100, count = 50)
OnDataAvailable(offset = 150, count = ...)
etc...
- count is how many bytes *may* be available in the input stream, the consumer obligation is to attempt to read this amount of data from the input stream (that's the contract)
so: offset has to be re-calculated in ExternalHelperAppParent, count is data.Length()
and, the offset and toRead we are sending at [1] are both actually sent wrong, when count > kCopyChunkSize - offset is broken, when count < input.Available() - the toRead is smaller than the actual data length we send. we can fix that too with omitting the args completely.
[1] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/uriloader/exthandler/ExternalHelperAppChild.cpp#57
Flags: needinfo?(honzab.moz)
Comment 6•6 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #5)
> the toRead is smaller
sorry, larger!
then, would be good to assert an assert at [2] that stringStream.available() == 0
[2] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/uriloader/exthandler/ExternalHelperAppParent.cpp#169
Updated•6 years ago
|
Priority: -- → P3
Whiteboard: [necko-triaged]
Reporter | ||
Comment 7•6 years ago
|
||
Note that there around 5 or so places where use this pattern. I just relised that we already have one of these already on file in 1415508. Need-info'ing myself to file the other instance (or documenting them in this bug, if it looks like the fix proposed in comment 6 will apply to them as well. )
Flags: needinfo?(ptheriault)
Reporter | ||
Comment 8•6 years ago
|
||
Alex is fixing this bug as part of the fix in 1415508.
Depends on: CVE-2019-9802
Flags: needinfo?(ptheriault)
Assignee | ||
Comment 9•6 years ago
|
||
Bug 1415508 landed, also fixing this.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Assignee: nobody → agaynor
Group: network-core-security → core-security-release
status-firefox65:
--- → wontfix
status-firefox66:
--- → affected
status-firefox67:
--- → fixed
status-firefox-esr60:
--- → wontfix
Target Milestone: --- → mozilla67
Comment 10•6 years ago
|
||
Just noticed this is related to bug 1415508 - that makes me lean even more towards trying beta uplift.
Flags: needinfo?(agaynor)
Assignee | ||
Comment 11•6 years ago
|
||
Yeah, I did a beta uplift request on the basis of the fact that it was fixing a whole family of issues.
Flags: needinfo?(agaynor)
Updated•6 years ago
|
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-cristsmash-triage]
Updated•6 years ago
|
Whiteboard: [necko-triaged][post-cristsmash-triage] → [necko-triaged][post-cristsmash-triage][adv-main66+]
Updated•6 years ago
|
Alias: CVE-2019-9799
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•