Closed Bug 1505678 (CVE-2019-9799) Opened 2 years ago Closed 2 years ago
Helper App Parent doesn't validate arguments (potential sandbox issue)
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 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 , 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()  https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/uriloader/exthandler/ExternalHelperAppParent.cpp#156  https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/uriloader/exthandler/nsExternalHelperAppService.cpp#1939  https://searchfox.org/mozilla-central/search?q=Recv.*dataAvailable&case=false®exp=true&path=parent
2 years ago
Depends on: semmle-analysis
2 years ago
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.
(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.
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  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.  https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/uriloader/exthandler/ExternalHelperAppChild.cpp#57
(In reply to Honza Bambas (:mayhemer) from comment #5) > the toRead is smaller sorry, larger! then, would be good to assert an assert at  that stringStream.available() == 0  https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/uriloader/exthandler/ExternalHelperAppParent.cpp#169
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. )
Depends on: CVE-2019-9802
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Whiteboard: [necko-triaged] → [necko-triaged][post-cristsmash-triage]
Whiteboard: [necko-triaged][post-cristsmash-triage] → [necko-triaged][post-cristsmash-triage][adv-main66+]
You need to log in before you can comment on or make changes to this bug.