Closed Bug 925896 Opened 6 years ago Closed 6 years ago

UAF in signaling_unittests

Categories

(Core :: WebRTC: Signaling, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 27+ fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 27+ unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed

People

(Reporter: ekr, Assigned: ehugg)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [qa-][adv-main27+][adv-esr24.3+])

Attachments

(2 files, 1 obsolete file)

Attached file asan-crash-stack.txt
No description provided.
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
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.
Attachment #816792 - Flags: review?(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.
Attachment #816792 - Flags: review?(ekr) → review?(adam)
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.
Attachment #816792 - Flags: review?(adam) → review-
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.
Attachment #816792 - Attachment is obsolete: true
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().
Attachment #820587 - Flags: review?(adam)
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.
Attachment #820587 - Flags: review?(adam) → review+
https://hg.mozilla.org/mozilla-central/rev/30c52c82e37b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Whiteboard: [qa-]
This is a sec-critical. Shouldn't we be taking this in ESR24 since it is affected?
Flags: needinfo?(ethanhugg)
Whiteboard: [qa-] → [qa-][adv-main27+]
(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.
Flags: needinfo?(ethanhugg)
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.
Attachment #820587 - Flags: approval-mozilla-esr24?
Attachment #820587 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
If ESR24 is affected then we probably should fix this on b2g26/1.2, it supports WebRTC in that version iirc.
(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.
Whiteboard: [qa-][adv-main27+] → [qa-][adv-main27+][adv-esr24.3+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.