Closed Bug 1328121 (CVE-2017-5416) Opened 3 years ago Closed 3 years ago
nightly crash when using ebay/paypal [@ ns
Document Open Info::On Start Request ]
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
I don't see HttpChannel to follow COM rules here when doing OnStartRequest call
(In reply to Olli Pettay [:smaug] from comment #1) > Do you have links to also other crash reports? Here they are: https://crash-stats.mozilla.com/report/index/be06cf6d-89d0-49d6-9b8f-384182170102 https://crash-stats.mozilla.com/report/index/4764c0da-bbe6-4f71-a208-958d02170102 https://crash-stats.mozilla.com/report/index/64cf9507-3818-4d6a-b35c-b3bd62170102 https://crash-stats.mozilla.com/report/index/f8b34630-29d7-4e74-9242-61dad2170102
this is a null deref - dan can you clear the security flag?
(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 ??
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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+
(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!
sorry - I thought I labeled this as sec-moderate. apparently that didn't happen. Its a zero-page deref in practice.
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.
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.
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+
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.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
You need to log in before you can comment on or make changes to this bug.