1048261 - ASAN failure in the runnable for SetDtlsConnected - doesn't hold a reference to pcimpl

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

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

With billm's GC/CC patches (https://tbpl.mozilla.org/?tree=Try&rev=ebce8337a8ed), there was an ASAN failure (https://tbpl.mozilla.org/php/getParsedLog.php?id=45173855&tree=Try) which pointed to the fact that the dispatch of the runnable for SetDtlsConnected doesn't hold a reference to the PCImpl (destination object), and in this case it's getting deleted before the runnable executes.

We should take and verify this fix, then uplift as far as possible.  No indication that it's exploitable (and without this GC/CC change, ASAN has never triggered on this), but we should assume it's theoretically possible.

Comment 1

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

Attachment: Fix SetDtlsConnected()

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Per Randell, the debug crashes are probably the same issue:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45153067&tree=Try

Comment 3

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

Regression from bug 942367

Comment 4

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

Comment on attachment 8467056 [details] [diff] [review]
Fix SetDtlsConnected()

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard.  You'd need to force the GC/CC into freeing it and get something else allocated there.  With current gC/CC I suspect somewhere between very hard and impossible.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  They do point to a missing refcount, but nothing more beyond that.

Which older supported branches are affected by this flaw? this was caused by a patch that landed in Fx32, only 32, 33, 34, and b2g 2.0

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

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

How likely is this patch to cause regressions; how much testing does it need? Virtually no chance for regression.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8467056 [details] [diff] [review]
Fix SetDtlsConnected()

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

Looks good with one nit.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +542,5 @@
>  
>    bool privacyRequested = false;
>    // TODO (Bug 952678) set privacy mode, ask the DTLS layer about that
>    GetMainThread()->Dispatch(
> +    WrapRunnable(nsRefPtr<PeerConnectionImpl>(mParent), 

Nit: trailing ws

Comment 6

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

Comment on attachment 8467056 [details] [diff] [review]
Fix SetDtlsConnected()

sec-approval+. Let's get this onto Aurora and Beta as well to avoid shipping it.

Comment 7

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

Comment on attachment 8467056 [details] [diff] [review]
Fix SetDtlsConnected()

Approval Request Comment
See sec-approval request.

Bug was introduced during Fx32

Minimal risk of regression (adds a refcount to a runnable member).

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb6c174ab00
Waiting on aurora/beta approvals to land there

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hi Randell, I pushed my GC patch on top of your fix and now I'm getting a different ASAN error.
https://tbpl.mozilla.org/php/getParsedLog.php?id=45203386&full=1&branch=try#error0
Could you please look and see if it's related?

Comment 10

[Martin Thomson [:mt:]]

That's a problem.  I can see now why the code that I wrote here was wrong and why Randell's fix is not sufficient.

The problem is that the STS thread doesn't maintain a reference to the PC, and it can't with the current structure.  The best thing I can think of right now requires a proxy for taking the STS thread upcall.  That has some tricky lifecycle issues to deal with.

I don't think that we should uplift the current patch until this one is right.  Can this wait until tomorrow?

Comment 11

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1048261-Fixing-dispatch-of-DTLS-connected-event.patch

This applies on top of Randell's patch (attachment 8467056 [details] [diff] [review]) and - I think - addresses the thread dispatch issue at the core of this.  It's much more code, but it seems to at least work.

The problem here is that you have something on main that needs to setup an observer on STS.  That observer runs on STS, but needs to dispatch to main, then destroy itself back on STS.  Oh, and the event that it gets from STS isn't guaranteed, so it needs to observe a second event, which could run while dispatching to main.

This does all that without synchronous dispatches, which are all kinds of bad.

I'm not sure that I got the refcounting around the thread dispatches exactly right.  But any problem there is limited to refcounting of the main and STS threads during dispatch, ...again, I think.  Please double check my logic, it's past my bed time.

https://tbpl.mozilla.org/?tree=Try&rev=887fe2b516d3

Comment 12

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

Comment on attachment 8467561 [details] [diff] [review]
0001-Bug-1048261-Fixing-dispatch-of-DTLS-connected-event.patch

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +536,3 @@
>  void
> +PeerConnectionMedia::AddTransportFlow(
> +  int aIndex, bool aRtcp, const mozilla::RefPtr<TransportFlow> &aFlow)

Leave function style matching rest of file

@@ +551,5 @@
>      NS_DISPATCH_NORMAL);
>  }
>  
>  void
> +PeerConnectionMedia::DtlsObserver::Connect(

in keeping with the "standard" of naming thread-specific functions, Connect_s()

@@ +607,5 @@
> +void
> +PeerConnectionMedia::DtlsObserver::Cleanup_s()
> +{
> +  mDispatching = false;
> +  SelfDestruct();

Comment that if SelfDestruct was blocked by mDispatching earlier, it will occur here.

Comment 13

[Ed Morley [:emorley]]

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

Leaving open for the other patch.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8467561 [details] [diff] [review]
0001-Bug-1048261-Fixing-dispatch-of-DTLS-connected-event.patch

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

Maybe I'm missing something, but why couldn't we just go back to square one and use a PC handle in the dispatch to PeerConnectionImpl::SetDtlsConnected (set up a thunk that attempts to get a strong ref on main, and makes the function call)? Are there other problems we're trying to fix here?

Comment 15

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1048261-Fixing-dispatch-of-DTLS-connected-event.patch

r=jesup already, need to pick Byron's brain about how to make this simpler.

Comment 16

[Martin Thomson [:mt:]]

(In reply to Byron Campen [:bwc] from comment #14)
> Comment on attachment 8467561 [details] [diff] [review]
> 0001-Bug-1048261-Fixing-dispatch-of-DTLS-connected-event.patch
> 
> Review of attachment 8467561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Maybe I'm missing something, but why couldn't we just go back to square one
> and use a PC handle in the dispatch to PeerConnectionImpl::SetDtlsConnected
> (set up a thunk that attempts to get a strong ref on main, and makes the
> function call)? Are there other problems we're trying to fix here?

Sorry about the delay, I typed a response, but didn't send (was on a call).

This is essentially what the patch does.  We can't use either PeerConnectionImpl or PeerConnectionMedia (they share a lifecycle), so we need something that is coupled the lifecycle of TransportLayerDtls.  If you have any ideas on how to make this simpler, I'm very interested.  I just can't think of anything easier.  That is, without creating a dependency between TransportLayerDtls and PeerConnection*, which I think would be a very bad layering violation.

Comment 17

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1048261-Safe-dispatch-from-DTLS-connect-to-PeerC.patch

Based on Byron's feedback, this is a much simpler version of the patch.

https://tbpl.mozilla.org/?tree=Try&rev=b9b2673bac60

Comment 18

[Martin Thomson [:mt:]]

And here, for maximal ASAN crashiness: https://tbpl.mozilla.org/?tree=Try&rev=7893633a011a

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8467973 [details] [diff] [review]
0001-Bug-1048261-Safe-dispatch-from-DTLS-connect-to-PeerC.patch

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

lgtm, if it works

Comment 20

[Eric Rescorla (:ekr)]

Comment on attachment 8467973 [details] [diff] [review]
0001-Bug-1048261-Safe-dispatch-from-DTLS-connect-to-PeerC.patch

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +551,5 @@
>  }
>  
>  void
> +PeerConnectionMedia::DtlsConnected_m(std::string aParentHandle,
> +                                     bool aPrivacyRequested)

Suggest make this const std::string&

Comment 21

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

Comment on attachment 8467973 [details] [diff] [review]
0001-Bug-1048261-Safe-dispatch-from-DTLS-connect-to-PeerC.patch

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

Much better, thanks

Comment 22

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1048261-Safe-dispatch-from-DTLS-connect-to-PeerC.patch

Approval Request Comment
[Feature/regressing bug #]: 942367
[User impact if declined]: none
[Describe test coverage new/current, TBPL]: builds, manual testing, mochitest, asan crashtests x50
[Risks and why]: none identified
[String/UUID change made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty hard.  See comment 4.

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?
See comment 4

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
the same patch should apply cleanly with minor changes

How likely is this patch to cause regressions; how much testing does it need?
the testing thus far seems adequate

Comment 23

[Martin Thomson [:mt:]]

Comment on attachment 8467056 [details] [diff] [review]
Fix SetDtlsConnected()

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

Seems like folly to r- now that I've patched over this.

Comment 24

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

Comment on attachment 8468107 [details] [diff] [review]
0001-Bug-1048261-Safe-dispatch-from-DTLS-connect-to-PeerC.patch

Approvals all around. Let's get it in.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Martin Thomson [:mt] from comment #18)
> And here, for maximal ASAN crashiness:
> https://tbpl.mozilla.org/?tree=Try&rev=7893633a011a

Looks like you're good to go, Bill :)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Bug 1016738 re-landed, so I went ahead and pushed this to inbound to avoid intermittent crashes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6564f687b3ff

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e4aa0b22be6
https://hg.mozilla.org/releases/mozilla-beta/rev/adb28e85421f

Comment 28

[Andrew McCreight [:mccr8]]

Can this be closed now Randell?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment 26 hasn't merged to m-c yet.

Comment 30

[Andrew McCreight [:mccr8]]

Ahh right.  I couldn't figure out why it hadn't been closed yet, but I forgot to check the date.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/adb28e85421f

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/6564f687b3ff

Comment 33

[Kamil Jozwiak [:kjozwiak]]

I quickly talked to Randell via IRC and asked if there's anything that QE can do to reproduce/verify. Randell mentioned that this issue was rarely seen on Try and other than constantly retriggering tbpl, there was never a reliable way to cause the problem. If there's anything else QE can do to test/verify this issue, please let us know and needinfo! Marking this as qe-verify-