1234578 - UAF in sigslot code (MutexAutoLock::MutexAutoLock)

Status: RESOLVED INCOMPLETE

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

Description

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

This bug was filed from the Socorro interface and is 
report bp-4cae62bb-dbad-4a1c-b4bb-64ada2151215.
=============================================================

UAF (0xffffffffe5e5e5e9)

Comment 1

[Byron Campen [:bwc]]

This looks like a dispatch landing on a PeerConnectionMedia that has been freed.

Comment 2

[Byron Campen [:bwc]]

The signature matches SignalUpdateDefaultCandidate.

Comment 3

[Byron Campen [:bwc]]

I'm not having any luck finding a way for SignalUpdateDefaultCandidate to fire after the PeerConnectionMedia has been destroyed.

Comment 4

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

FYI, this was in 42 in case things have changed

Comment 5

[Maire Reavy [:mreavy]]

I've asked Byron to take a look at the Fx42 code to see if there's a way this could have happened in Fx42 that can't happen now.  (We've landed a lot of code changes since 42.)

Comment 6

[Byron Campen [:bwc]]

I've looked at 42, doesn't seem to be different enough to allow this to happen either.

Comment 7

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

Once we know what versions this affects , can you request tracking? Thanks.

Comment 8

[Andrew McCreight [:mccr8]]

I see a bunch of crashes on 43, and one on 44, in the last 28 days, but not more recent than that. Maybe this can be closed as incomplete?

Comment 9

[Maire Reavy [:mreavy]]

I'd like to wait and reassess when Fx44 has more usage.  BTW, I don't this crash is actually being hit that frequently.  You have to look very closely at the stack to know if it's this bug.

Comment 10

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

"More Reports" for this bug catches a tun of bugs in different areas; you have to check the stack to know which one is which.  I think I've seen 1 hit for this particular bug total, in 42.  I'll check the recent stacks again for more hits from this, but it's time-consuming.

We desperately need search terms for "on the crashing thread's stack" in crashstats

Comment 11

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

In the last month:
43.0.1: https://crash-stats.mozilla.com/report/index/a8c5433b-3790-4bf9-a6c5-963492160126
43.0b2: https://crash-stats.mozilla.com/report/index/4cfb1a41-51e2-4b60-ba80-efe0e2160120
It's so low frequency we're not likely to see hits outside of release (though we got one beta)

Comment 12

[Byron Campen [:bwc]]

Attachment: Assert if PCM is destroyed improperly

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8714364 [details] [diff] [review]
Assert if PCM is destroyed improperly

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

This might catch this bug when it happens, but it is hard to be sure.

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

Comment 14

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

Once it's stable on Nightly, likely we want to uplift this to Aurora and Beta.  If this does catch it, it converts it into a safe crash.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8714364 [details] [diff] [review]
Assert if PCM is destroyed improperly

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

   Not very easily at all; we aren't even sure this will catch this bug.

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

   No.

Which older supported branches are affected by this flaw?

   It looks like 42-44 are definitely affected, maybe more.   

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

   Unknown.

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

   No, but this should be low-risk.

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

   Very unlikely, but it might cause things to crash more reliably when things go horribly wrong.

Comment 16

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

Comment on attachment 8714364 [details] [diff] [review]
Assert if PCM is destroyed improperly

sec-approval+ because...why not?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a580fa4ebbcc

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

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

Tracking since this is sec-critical and will likely need uplift. 

Randall can you check to see if ESR38 is affected?

Comment 20

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

There's no easy way to tell barring a hit; this is a defense-in-depth patch.  Likely any bug goes back to 38, but there's no easy way to tell without a different interface to crash-stats to look at a much wider timeframe - and then looking at the stacks of hundreds of crashes to see if any of them are from sigslot.

I'd suggest taking this safely-assert patch for 38

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Byron, could you fill the uplift request to aurora, beta & esr38? Thanks

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8714364 [details] [diff] [review]
Assert if PCM is destroyed improperly

[Approval Request Comment]
User impact if declined: 

   This is mostly a diagnostic patch, but if it does catch this problem, it converts a potential unsafe UAF crash into a safe crash.

Fix Landed on Version:

   47

Risk to taking this patch (and alternatives if risky): 

   Very low risk, although it might cause more frequent (safe) crashes than are happening now.

String or UUID changes made by this patch: 

   None.

[Feature/regressing bug #]:

   Unknown.

[Describe test coverage new/current, TreeHerder]:

   We have no test case that is able to reproduce this problem, and are unsure how this problem could happen.

[Risks and why]: 

   This patch adds a release assert for a case that should never happen, but it may happen more often than we think. There's no way to know for sure until we see some data.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8714364 [details] [diff] [review]
Assert if PCM is destroyed improperly

Improve the situation, taking it.
Should be in 45 beta 5.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2520e1d75c80
https://hg.mozilla.org/releases/mozilla-beta/rev/308dbe3ce541
https://hg.mozilla.org/releases/mozilla-esr38/rev/9719b71d72dd

Comment 25

[Byron Campen [:bwc]]

Not seeing this crash on 45 still. I do see another crash on 44 that might be related:

https://crash-stats.mozilla.com/report/index/e31270dc-61d1-491f-b0b7-9e4f12160229

This one definitely looks like it is caused by using the PeerConnectionMedia after it has started tearing down, possibly for the same reason. It also looks like this might be caused if the following code doesn't actually do what it is supposed to:

http://mxr.mozilla.org/mozilla-release/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#794

I don't see how that could cause this bug though.

Comment 26

[Byron Campen [:bwc]]

I'm also not seeing the release assertion we added in crash-stats.

Comment 27

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

How long do we want to keep this open?

Comment 28

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

Only crash I see in the last month was a single 44b1 crash.   (19 crashes, but all are other sources than this except one).  44b1 wouldn't have byron's mitigation (MOZ_RELEASE_ASSERT); a search for PeerConnectionMedia in crash-stats doesn't find anything pointing to the release assert.

I think we need more time to know, given low frequency of hits.  We could close this and needinfo ourselves to come back and check it.  What we landed is mitigation, though, not really a core fix.

Comment 29

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

I can go either way. If you'd prefer to leave it open, that's fine.

Comment 30

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

One crash in the last week, in 46, from the 20160324011246 build. 
https://crash-stats.mozilla.com/report/index/aff2428b-e751-4f23-a119-5b91d2160402

Comment 31

[Byron Campen [:bwc]]

I have a hypothesis as to what is going on. Another patch coming soon.

Comment 32

[Byron Campen [:bwc]]

Attachment: Add an assertion

MozReview-Commit-ID: 5Mq8CBmSDsu

Comment 33

[Byron Campen [:bwc]]

Comment on attachment 8744906 [details] [diff] [review]
Add an assertion

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

Adding an assertion to check another possible cause.

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

Comment 34

[Nils Ohlmeier [:drno]]

Comment on attachment 8744906 [details] [diff] [review]
Add an assertion

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

LGTM

Comment 35

[Byron Campen [:bwc]]

Comment on attachment 8744906 [details] [diff] [review]
Add an assertion

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

   Not easily at all.

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 is just an attempt to diagnose.

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

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

   No, but they should be easy if we need them.

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

   Extremely unlikely. At most, it will convert an unsafe crash/undefined behavior into a safe crash.

Comment 36

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

I'm confused on why you're asking for sec-approval on a bug we shipped a fix for a month and a half ago. 

Is this a new few for the core issue and supersedes the previous fix?

Comment 37

[Byron Campen [:bwc]]

Both of these patches have been investigatory, not fixes.

Comment 38

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

(In reply to Byron Campen [:bwc] (PTO Apr 15) from comment #37)
> Both of these patches have been investigatory, not fixes.

I was going by comment 28:

> What we landed is mitigation, though, not really a core fix.

Comment 39

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

Tracking for 49. We may want to mark this as still affected for older versions if uplifting this latest patch is a good idea. Or, it's usually more clear to open a new bug.

Comment 40

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f241151c616953e1dbbb29e4757dda10a56a3dd2
Bug 1234578: Add an assertion. r=drno, a=abillings

Comment 41

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

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

Comment 42

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

Oops, didn't catch that leave-open.

Comment 43

[Andrew McCreight [:mccr8]]

I don't see any crashes containing PeerConnectionMedia on Nightly, for what it is worth.

Comment 44

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

What's left to do here. It is 1 1/2 months later and this bug comes up in all security triage meetings as an open issue.

Comment 45

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

No crashes in the last month again.  I'm going to close as incomplete pending new data

Comment 46

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.