1328121 - (CVE-2017-5416) nightly crash when using ebay/paypal [@ nsDocumentOpenInfo::OnStartRequest ]

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Radek 'sysKin' Czyz]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Do you have links to also other crash reports?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(initially this looks like an issue in the caller)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I don't see HttpChannel to follow COM rules here when doing OnStartRequest call

Comment 4

[Radek 'sysKin' Czyz]

(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

Comment 5

[Patrick McManus [:mcmanus]]

this is a null deref - dan can you clear the security flag?

Comment 6

[Patrick McManus [:mcmanus]]

(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 ??

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: hold ref on mlistener when calling onstartrequest

Comment 8

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d7e339a3c36

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Patrick McManus [:mcmanus]]

(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.

Comment 11

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5593f1b533da70ca22628d8461ebc75273fbc2e6
Bug 1328121 - hold ref on mlistener when calling onstartrequest r=smaug r=jduell

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 13

[Patrick McManus [:mcmanus]]

sorry - I thought I labeled this as sec-moderate. apparently that didn't happen. Its a zero-page deref in practice.

Comment 14

[Daniel Veditz [:dveditz]]

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

Comment 15

[Patrick McManus [:mcmanus]]

its a little unclear exactly how this happens - the fix is speculative. I'd rather assume sec-moderate.

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: hold ref on mlistener when calling onstartrequest

Comment 17

[Patrick McManus [:mcmanus]]

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

fix the static analysis unused complaint

Comment 18

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd03cacfcdddb92b40e259af83abdaaeaabc00b4
Bug 1328121 - hold ref on mlistener when calling onstartrequest r=smaug r=jduell

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd03cacfcddd

Please request Aurora/Beta approval on this when you feel comfortable doing so.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

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 21

[Patrick McManus [:mcmanus]]

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.

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

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

sec-moderate fix for aurora52

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0146e1addf31

Comment 24

[Radek 'sysKin' Czyz]

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....

Comment 25

[Patrick McManus [:mcmanus]]

(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.