WebRTC possible data race with strlib_copy vs strlib_empty

RESOLVED FIXED in mozilla19

Status

()

Core
WebRTC: Signaling
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: posidron, Assigned: jesup)

Tracking

(Blocks: 1 bug)

Trunk
mozilla19
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox18 unaffected, firefox19 unaffected)

Details

(Whiteboard: [tsan] [WebRTC] [blocking-webrtc+] [qa-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 676571 [details]
callstack

media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c:104
if ((temp->refcount < 0xffff) && (str != empty_str)) {
        temp->refcount++;
}

media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c:374
        empty_str = strlib_malloc("", LEN_UNKNOWN, __FILE__, __LINE__);
        temp = STR_TO_STRUCT(empty_str);
        temp->refcount = 0xffff;

Marked as s-s because memory functions are involved, please downgrade if necessary.

Tested with m-c changeset: 111684:e19e170d2f6d
(Assignee)

Comment 1

5 years ago
This looks like it's real unless we mandate that strlib_empty() is called during init, which is probably the best way to solve it.

Updated

5 years ago
Whiteboard: [tsan] → [tsan] [WebRTC] [blocking-webrtc+]
sec-moderate on the (possibly erroneous) assumption arranging such a race reliably enough to exploit would be hard.
Keywords: sec-moderate
(Assignee)

Comment 3

5 years ago
Analysis is that strlib_empty() is called in the init of the signaling GSMTask:
GSMTask()->fsm_init->fsmdef_init_dcb->strlib_empty
GSMTask is started from thread_init(), which is called from ccInit() (initializing all of SipCC)

Since this will occur before any other uses of it, the mandate I mention above is already occurring, though not on purpose.

I'll add a patch that explicitly inits it, however there's no security risk with the current code, imo.

As there's no risk, removing sec-moderate (dan, if this is wrong please let me know).
Keywords: sec-moderate
(Assignee)

Comment 4

5 years ago
Created attachment 680412 [details] [diff] [review]
Enforce initializing strlib before using
(Assignee)

Updated

5 years ago
Assignee: nobody → rjesup
(Assignee)

Updated

5 years ago
Attachment #680412 - Flags: review?(ethanhugg)

Comment 5

5 years ago
Comment on attachment 680412 [details] [diff] [review]
Enforce initializing strlib before using

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

This should work.
Attachment #680412 - Flags: review?(ethanhugg) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b123de2b4109

All as unaffected as there's no actual security risk here
status-firefox18: --- → unaffected
status-firefox19: --- → unaffected
Target Milestone: --- → mozilla19
(Reporter)

Updated

5 years ago
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/b123de2b4109
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [tsan] [WebRTC] [blocking-webrtc+] → [tsan] [WebRTC] [blocking-webrtc+] [qa-]
Christoph, have you had any testcase for it?
(Reporter)

Comment 9

5 years ago
Nope sorry, I was using the examples of the landing page and run https://developer.mozilla.org/en-US/docs/Thread_Sanitizer
You need to log in before you can comment on or make changes to this bug.