1280443 - Crash in nr_socket_sendto

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Byron Campen [:bwc]]

From crash-stats:

https://crash-stats.mozilla.com/signature/?date=%3E01%2F01%2F2016&signature=nr_socket_sendto&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports

As of 48, we are seeing crashes in nr_socket_sendto, some of which look like UAF. Here's the mtransport stuff that landed in 48:

https://bugzilla.mozilla.org/buglist.cgi?list_id=13071777&resolution=FIXED&query_format=advanced&component=WebRTC%3A%20Networking&target_milestone=mozilla48

It might have something to do with ICE restart, but it is hard to say yet. My preliminary analysis of the source did not turn anything up. It may be some bug elsewhere has crept in, but I looked over the timer stuff and I didn't see anything scary.

Comment 1

[Byron Campen [:bwc]]

Ok, after looking at the timer stuff some more, there's a bunch of racy stuff going on, particularly in nsTimerImpl::Release. I'm going to put together a diagnostic patch for this bug to see if the problem is timers failing to cancel properly.

Comment 2

[Byron Campen [:bwc]]

Attachment: Make a timer assert more aggressive

Comment 3

[Byron Campen [:bwc]]

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

Comment 4

[Nils Ohlmeier [:drno]]

Hm crash-stats shows a single crash in 39 and then lots of crashes from 48 on. Looks like something has changed in the timing in 48.
Looks like only ICE restart really changed anything significantly in 48. The only idea which comes to mind would be double checking if we properly set, reset and clear all timers in case of rollbacks and if ICE restarts fail.

Comment 5

[Byron Campen [:bwc]]

Yeah, I've looked it over, and ICE restart made very few modifications to the nICEr code, which is where all the timers are handled. It could be that we're running into a similar problem as found in bug 1279146, where we have some dangling pointers to nICEr stuff that we try to use, but so far I have not discovered a code-path that gets from there to this crash. It may be that bug 1279146 is corrupting the heap in a way that causes this bug, but that's a long shot.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8763626 [details] [diff] [review]
Make a timer assert more aggressive

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Very hard I think.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   Not really.

Which older supported branches are affected by this flaw?

   Unknown; this patch should detect whether canceling an nsTimerImpl is not working in rare cases. If this ends up being the case, it is probably a long-standing bug.

How likely is this patch to cause regressions; how much testing does it need?

   At most it will cause some null pointer crashes in rare cases.

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8763626 [details] [diff] [review]
Make a timer assert more aggressive

Review of attachment 8763626 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8763626 [details] [diff] [review]
Make a timer assert more aggressive

sec-approval=dveditz

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8763626 [details] [diff] [review]
Make a timer assert more aggressive

Approval Request Comment
[Feature/regressing bug #]:

   If the crash is being caused by flaws in the timer implementation, this problem has likely existed since webrtc landed.

[User impact if declined]:

   This could convert a rare UAF crash into a slightly less rare null pointer crash.

[Describe test coverage new/current, TreeHerder]:

   Given how rare this crash is, it would probably be hard to get coverage.

[Risks and why]: 

   It is possible that we will get null pointer crashes more often, instead of the pretty rare UAF crashes we're seeing.

[String/UUID change made/needed]:

   None.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/063a3fe1aad1

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/063a3fe1aad1

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Byron, it seems that esr45 is affected, right?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8763626 [details] [diff] [review]
Make a timer assert more aggressive

Taking it as it fixes a sec-high.
Should be in 48 beta 4.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd8d4ad885aa

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c921c304fad

Comment 16

[Byron Campen [:bwc]]

I have not seen an esr 45 crash, but it is possible it is affected. Also, I should have marked this leave-open, since the patch might not actually fix the root cause. We have to wait and see.

Comment 17

[Andrew McCreight [:mccr8]]

I maybe see one crash since this landed on 48, and none on 49, for what that is worth.

Comment 18

[Byron Campen [:bwc]]

Attachment: Copy this the hard way

.

Comment 20

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a265da05d2e

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

I've run the patch from Try for 40min on a machine where it insta-crashed, no problems.  Also on Linux for me it would i-loop due to the same bug, also running for 40 min now.

Source of the bug was bug 1258753

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Sec-critical since the dup shows you can insta-crash it with a call through a UAF vtbl

Comment 23

[Nils Ohlmeier [:drno]]

Comment on attachment 8779833 [details] [diff] [review]
Copy this the hard way

Review of attachment 8779833 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 24

[Byron Campen [:bwc]]

Comment on attachment 8779833 [details] [diff] [review]
Copy this the hard way

Approval Request Comment
[Feature/regressing bug #]:

   Bug 1258753

[User impact if declined]:

   We're using a function pointer in freed memory using a fairly simple test-case, which means an arbitrary code execution exploit could be mounted.

[Describe test coverage new/current, TreeHerder]:

   Getting the timing down has been hard; we've only been able to repro consistently on windows 10.

[Risks and why]: 

   Pretty low. It is a pretty simple change.

[String/UUID change made/needed]:

   None.


[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Based on the patch, not so easily, because it is hard to work out where the sec problem comes in.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   Not really. It's pretty subtle.

Which older supported branches are affected by this flaw?

   48 and up.

If not all supported branches, which bug introduced the flaw?

   Bug 1258753

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

   I bet this will apply cleanly, but if it does not it will be easy to make backports.

How likely is this patch to cause regressions; how much testing does it need?

   Pretty unlikely.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

I'm confused by the re-opening of this bug. We fixed it in 48 and noted it in the roll up advisory. Did the fix not work correctly?

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

We landed a patch which was described as "This could convert a rare UAF crash into a slightly less rare null pointer crash."   Also, byron meant this to be leave-open I think (thus his reopening it right after it was closed: "Also, I should have marked this leave-open, since the patch might not actually fix the root cause. We have to wait and see.")

It did quiet down the crashes quite a bit, but not quite 100%.

Now that we have a reproducible testcase, we were able to track the root cause down.  (Ironically the crash didn't exactly lead us to the source of the problem, it was the iloop on Linux that pointed directly at the cause.)

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

I'm going also second byron's evaluation: "We're using a function pointer in freed memory using a fairly simple test-case, which means an arbitrary code execution exploit could be mounted."  Since it's now known to be easy to force the UAF, and it's a function-pointer UAF, I expect a practical attack could be constructed.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

I can take it in the 48.0.1 release but I need a sec approval today.

Comment 29

[Maire Reavy [:mreavy]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> I can take it in the 48.0.1 release but I need a sec approval today.

Al -- Just want to make sure this is at the top of your queue for this morning.  Thanks.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8779833 [details] [diff] [review]
Copy this the hard way

sec-approval for this then!

Comment 31

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb4a4b7bd0497ba8a4868ccbcd04036e63145a26
Bug 1280443: Copy this the hard way. r=drno

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e188ed421a0
https://hg.mozilla.org/releases/mozilla-beta/rev/df5b12ee1761
https://hg.mozilla.org/releases/mozilla-release/rev/9bf4d2171f27

a=sylvestre over IRC

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8779833 [details] [diff] [review]
Copy this the hard way

Let's take it in the 48.0.1 dot release

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 35

[Gerry Chang [:gchang]]

Added in the 48.0.1 release notes: "Fix a crash in WebRTC".