Closed Bug 1505678 (CVE-2019-9799) Opened 2 years ago Closed 2 years ago

ExternalHelperAppParent doesn't validate arguments (potential sandbox issue)

Categories

(Core :: Networking, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: pauljt, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

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&regexp=true&path=parent
PS I wonder if this is related to bug 1471312 - given that its the same function.
(I'm not linking, since that bug is public)
This is not related to that bug, that's a boring null-ptr-deref if you call this method before something has been initialized.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(honzab.moz)
Keywords: sec-high
(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)
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)
(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
Priority: -- → P3
Whiteboard: [necko-triaged]
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)

Alex is fixing this bug as part of the fix in 1415508.

Depends on: CVE-2019-9802
Flags: needinfo?(ptheriault)

Bug 1415508 landed, also fixing this.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Assignee: nobody → agaynor
Group: network-core-security → core-security-release
Target Milestone: --- → mozilla67

Just noticed this is related to bug 1415508 - that makes me lean even more towards trying beta uplift.

Flags: needinfo?(agaynor)

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)
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-cristsmash-triage]
Whiteboard: [necko-triaged][post-cristsmash-triage] → [necko-triaged][post-cristsmash-triage][adv-main66+]
Alias: CVE-2019-9799
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.