Closed Bug 1328121 (CVE-2017-5416) Opened 3 years ago Closed 3 years ago

nightly crash when using ebay/paypal [@ nsDocumentOpenInfo::OnStartRequest ]

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 + wontfix
firefox52 + fixed
firefox53 + fixed

People

(Reporter: syskin2, Assigned: mcmanus)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 file, 1 obsolete file)

Crash id: https://crash-stats.mozilla.com/report/index/d2771913-daf5-4266-8e79-f62142170102

What I did:
Buy a thing using ebay.com.au
Select PayPal payment method
Enter your Paypal details

It spins for a few seconds, then crashes.
Crash was 100% reproducible and I've made ~6 identical crash reports. I have made the payment since, so now I can no longer try (such as without addons), sorry.
Do you have links to also other crash reports?
(initially this looks like an issue in the caller)
Component: DOM → Networking
Group: core-security
I don't see HttpChannel to follow COM rules here when doing OnStartRequest call
Flags: needinfo?(mcmanus)
this is a null deref - dan can you clear the security flag?
Flags: needinfo?(dveditz)
(In reply to Olli Pettay [:smaug] from comment #3)
> I don't see HttpChannel to follow COM rules here when doing OnStartRequest
> call

are you suggesting that the stream listener is null, or that the references are short, or ??
Flags: needinfo?(mcmanus)
Group: core-security
Flags: needinfo?(dveditz)
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8823419 - Flags: review?(jduell.mcbugs)
Attachment #8823419 - Flags: review?(bugs)
Group: core-security
Comment on attachment 8823419 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest

This is a guess fix, but is Necko in general protected from this kind of issues?
I'm definitely not familiar enough with its callback call handling.
Attachment #8823419 - Flags: review?(bugs) → review+
Attachment #8823419 - Flags: review?(jduell.mcbugs) → review+
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8823419 [details] [diff] [review]
> hold ref on mlistener when calling onstartrequest
> 
> This is a guess fix, but is Necko in general protected from this kind of
> issues?
> I'm definitely not familiar enough with its callback call handling.

I haven't gone looking for this class of bug, though the surface area of where the stream listeners are called is not large. Interesting that this crash is recent - nothing has changed in that call chain that I can think of in a long time.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5593f1b533da70ca22628d8461ebc75273fbc2e6
Bug 1328121 - hold ref on mlistener when calling onstartrequest r=smaug r=jduell
Iris had to back this out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/813099b24bf5

https://treeherder.mozilla.org/logviewer.html#?job_id=66454558&repo=mozilla-inbound&lineNumber=15609

netwerk/protocol/http/nsHttpChannel.cpp:1319:9: error: Unused "kungFuDeathGrip" 'nsCOMPtr<nsIStreamListener>' objects constructed from members are prohibited
        nsCOMPtr<nsIStreamListener> deleteProtector(mListener);
netwerk/protocol/http/nsHttpChannel.cpp:1319:53: note: Please switch all accesses to this member to go through 'deleteProtector', or explicitly pass 'deleteProtector' to `mozilla::Unused`
        nsCOMPtr<nsIStreamListener> deleteProtector(mListener);

Also, Patrick, can you please help assign a security rating here? And what other branches are affected (a cursory glance at crash-stats for this signature suggests at least 49-53)? And possibly a retroactive security approval request depending on the answers to those first 2 questions :). Thanks!
Flags: needinfo?(mcmanus)
sorry - I thought I labeled this as sec-moderate. apparently that didn't happen. Its a zero-page deref in practice.
Flags: needinfo?(mcmanus)
Keywords: sec-moderate
The patch looks like it's fixing a UAF, but the crashes we're seeing are null derefs. If it's always guaranteed to be a null deref we can call this a DOS (not sec-moderate) and unhide it
Group: core-security → network-core-security
its a little unclear exactly how this happens - the fix is speculative. I'd rather assume sec-moderate.
Attachment #8823419 - Attachment is obsolete: true
Comment on attachment 8824210 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest

fix the static analysis unused complaint
Attachment #8824210 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd03cacfcdddb92b40e259af83abdaaeaabc00b4
Bug 1328121 - hold ref on mlistener when calling onstartrequest r=smaug r=jduell
https://hg.mozilla.org/mozilla-central/rev/cd03cacfcddd

Please request Aurora/Beta approval on this when you feel comfortable doing so.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(mcmanus)
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Tracking this for 51 and up. We still have the beta 13 build today, beta 14 later and then the RC build next week, if you want to uplift as far as beta.
Comment on attachment 8824210 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: crash
[Is this code covered by automated tests?]: trigger unknown - inspection fix
[Has the fix been verified in Nightly?]: tested in nightly, but crash too low to verify
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: holds a ref with a smart pointer - very small and safe change
[String changes made/needed]: none

the volume here is too low to verify - and given the lateness of the beta I don't think, on balance, it is right to place it there.
Flags: needinfo?(mcmanus)
Attachment #8824210 - Flags: approval-mozilla-aurora?
Comment on attachment 8824210 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest

sec-moderate fix for aurora52
Attachment #8824210 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: network-core-security → core-security-release
Is there any chance that this fix transformed the crash into a (non-main-thread) freeze?

I had an opportunity to pay with paypal again, with current nightly, and this time instead of crashing it was just spinning forever and ever.
After a few attempts I disabled all my addons and everything went smoothly so it's clearly my addons that are causing it....
(In reply to Radek 'sysKin' Czyz from comment #24)
> Is there any chance that this fix transformed the crash into a
> (non-main-thread) freeze?
> 

I guess its possible, but not because of anything directly in the fix.. you were clearly in some kind of error code path and with the fix you'll avoid a crash and go farther down it.. but ther is nothing actionable. fwiw I would expect any hang to be on the main thread in this case.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Alias: CVE-2017-5416
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.