925896 - UAF in signaling_unittests

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: asan-crash-stack.txt

Comment 1

[Eric Rescorla (:ekr)]

http://buildbot.rtfm.com:8010/builders/ubuntu12_04_64_asan/builds/12/steps/WebRTC%3A%20Signaling/logs/stdio

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Signaling - Addref when adding sessiondata_t to hash

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 816792 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash


I believe we need to addref when putting this call infos into the hashtable.  Refcounting is done already in ccapi_call.c but there may not have been obvious to do it here because callinfo_ref_t, struct callinfo_ref_t_ *, and session_data_t * actually point to the same structure.  For example - CCAPI_Call_releaseCallInfo in ccapi_call.c

I have been unable to duplicate the original race condition so perhaps we could put this patch on your builders and run it a billion times.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 816792 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash

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

Retargeting review to ABR. I will rerun this on my builders next week.

Comment 5

[Adam Roach [:abr]]

Comment on attachment 816792 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash

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

I haven't dug into the stack traces aggressively enough to have an opinion one way or another about whether this is a watertight fix for the problem that turned up. The premise of the patch seems sound -- that we need to hold a refcount when we're in the hash table -- but I have some concerns about hashtable entry immortality and/or a different source of UAFs.

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c
@@ +1586,5 @@
>          if ( sess_data_p != NULL ){
> +            sess_data_p->ref_count--;
> +            if (!sess_data_p->ref_count) {
> +                cleanSessionData(sess_data_p);
> +                if ( 0 >  delhash(sessUpd->sessionID) ) {

This hash table removal is now conditional on the refcount reaching zero, even though we've already decremented the refcount that ostensibly reflects the hashtable holding a reference to the sess_data.

One of two things can then happen; either:

 - We never come back through this code with the 
   same sessionID, which means that the entry
   will live in the hashtable indefinitely; or

 - We *do* come back through this code with the
   same sessionID, which matches a hash entry that
   we didn't hold a refcount for, which in turn
   means that it may well have been deallocated
   already.

In other words, I suspect the logic here needs to be:

delhash(sessUpd->sessionID);
sess_data_p->ref_count--;
if (!sess_data_p->refcount) {
  cleanSessionData(sess_data_p);
  cpr_free(sess_data_p);
}

Alternately, to consolidate reference count handling and make the logic easier to read:

delhash(sessUpd->sessionID);
CCAPI_Call_releaseCallInfo(sess_data_p);

@@ +1648,5 @@
>                  if ( sess_data_p != NULL ){
> +                    sess_data_p->ref_count--;
> +                    if (!sess_data_p->ref_count) {
> +                        cleanSessionData(sess_data_p);
> +                        if ( 0 >  delhash(sessUpd->sessionID) ) {

Same issue as above.

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 816792 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash


Gah. You're right, that's not what I wanted.  Let me fix and re-test.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Signaling - Addref when adding sessiondata_t to hash

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 820587 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash


Updated to use CCAPI_Call_retainCallInfo() and CCAPI_Call_releaseCallInfo().

Comment 9

[Adam Roach [:abr]]

Comment on attachment 820587 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash

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

Looks good to me.

Comment 10

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30c52c82e37b

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/30c52c82e37b

Comment 12

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

This is a sec-critical. Shouldn't we be taking this in ESR24 since it is affected?

Comment 13

[Adam Roach [:abr]]

(In reply to Al Billings [:abillings] from comment #12)
> This is a sec-critical. Shouldn't we be taking this in ESR24 since it is
> affected?

Sorry, tracking ESR builds wasn't even on my radar. The short answer is "yes," and this patch should be pretty easy to backport. I'll make sure it applies on the esr24 tree and ask for approval (either on the attached patch or on a modified version that applies to mozilla-esr24).

There's probably a broader question around whether the primitive form of WebRTC that was available in 24 is worth keeping enabled for the ESR release, but that's independent of the final disposition of this bug.

Comment 14

[Adam Roach [:abr]]

Comment on attachment 820587 [details] [diff] [review]
Signaling - Addref when adding sessiondata_t to hash

Conveniently, this patch applies cleanly to the ESR24 tree, so I'm going to just request approval on this version.

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

Bug is sec:crit.

> User impact if declined: 

Exposure to UAF condition, which may be exploitable by a clever adversary.

> Fix Landed on Version:

27

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

Very low -- has been on train since FFx27 with no ill effects.

> String or UUID changes made by this patch: 

None.

Comment 15

[Daniel Veditz [:dveditz]]

If ESR24 is affected then we probably should fix this on b2g26/1.2, it supports WebRTC in that version iirc.

Comment 16

[Adam Roach [:abr]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/ac729af904c9

Comment 17

[Adam Roach [:abr]]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> If ESR24 is affected then we probably should fix this on b2g26/1.2, it
> supports WebRTC in that version iirc.

1.2 doesn't have WebRTC support -- that was added in 1.3.