Bug 1328121 (CVE-2017-5416)

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

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: syskin2, Assigned: mcmanus)

Tracking

({sec-moderate})

Trunk
mozilla53
sec-moderate
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox50 wontfix, firefox51+ wontfix, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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)
(Assignee)

Comment 5

2 years ago
this is a null deref - dan can you clear the security flag?
Flags: needinfo?(dveditz)
(Assignee)

Comment 6

2 years ago
(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)

Comment 7

2 years ago
Created attachment 8823419 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest
(Assignee)

Updated

2 years ago
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

2 years ago
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+

Updated

2 years ago
Attachment #8823419 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
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!
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Flags: needinfo?(mcmanus)
(Assignee)

Comment 13

2 years ago
sorry - I thought I labeled this as sec-moderate. apparently that didn't happen. Its a zero-page deref in practice.
status-firefox-esr45: --- → affected
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
(Assignee)

Comment 15

2 years ago
its a little unclear exactly how this happens - the fix is speculative. I'd rather assume sec-moderate.
(Assignee)

Comment 16

2 years ago
Created attachment 8824210 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest
(Assignee)

Updated

2 years ago
Attachment #8823419 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Comment on attachment 8824210 [details] [diff] [review]
hold ref on mlistener when calling onstartrequest

fix the static analysis unused complaint
Attachment #8824210 - Flags: review+
(Assignee)

Comment 18

2 years ago
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
Last Resolved: 2 years ago
status-firefox53: affected → fixed
status-firefox-esr45: affected → wontfix
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.
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox53: ? → +
(Assignee)

Comment 21

2 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0146e1addf31
status-firefox51: affected → wontfix
status-firefox52: affected → fixed
Group: network-core-security → core-security-release
(Reporter)

Comment 24

2 years ago
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....
(Assignee)

Comment 25

2 years ago
(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.