Closed
Bug 806830
Opened 12 years ago
Closed 12 years ago
WebRTC possible data race with strlib_copy vs strlib_empty
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
Tracking | Status | |
---|---|---|
firefox18 | --- | unaffected |
firefox19 | --- | unaffected |
People
(Reporter: posidron, Assigned: jesup)
References
Details
(Whiteboard: [tsan] [WebRTC] [blocking-webrtc+] [qa-])
Attachments
(2 files)
2.71 KB,
text/plain
|
Details | |
2.49 KB,
patch
|
ehugg
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
Whiteboard: [tsan] → [tsan] [WebRTC] [blocking-webrtc+]
Comment 2•12 years ago
|
||
sec-moderate on the (possibly erroneous) assumption arranging such a race reliably enough to exploit would be hard.
Keywords: sec-moderate
Assignee | ||
Comment 3•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → rjesup
Assignee | ||
Updated•12 years ago
|
Attachment #680412 -
Flags: review?(ethanhugg)
Comment 5•12 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•12 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•12 years ago
|
Group: core-security
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b123de2b4109
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [tsan] [WebRTC] [blocking-webrtc+] → [tsan] [WebRTC] [blocking-webrtc+] [qa-]
Comment 8•12 years ago
|
||
Christoph, have you had any testcase for it?
Reporter | ||
Comment 9•12 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.
Description
•