Closed Bug 849888 Opened 9 years ago Closed 9 years ago

WebRTC heap-buffer-overflow crash with SignalingAgentTest.CreateUntilFailThenWait [@sub_print_msg]

Categories

(Core :: WebRTC: Signaling, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox19 --- disabled
firefox20 - disabled
firefox21 + fixed
firefox22 + fixed
firefox23 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: posidron, Assigned: ehugg)

References

Details

(4 keywords, Whiteboard: [WebRTC],[blocking-webrtc+][qa-][adv-main21-])

Attachments

(2 files)

Attached file callstack
$ ./objdir/media/webrtc/signaling/test/signaling_unittests --gtest_filter=SignalingAgentTest.CreateUntilFailThenWait


Tested with m-i changeset: 124370:b123c8210d41
Assignee: nobody → ethanhugg
Keywords: sec-critical
Whiteboard: sec-critical
Looking at the stack I see that pres_terminate_req_all looks like this:

pres_terminate_req_all (void)
{
    char dummy;

    (void) app_send_message(&dummy, sizeof(dummy), CC_SRC_MISC_APP,
                            SUB_MSG_PRESENCE_TERM_REQ_ALL);
}

Which goes here and allocates a a msgbuf the size of a char using cpr_malloc in gsm_get_buffer via cc_get_msg_buf:

cc_rcs_t
app_send_message (void *msg_data, int msg_len, cc_srcs_t dest_id, int msg_id)
{
    void *pmsg;

    pmsg = (void *) cc_get_msg_buf(msg_len);
...
    return sub_send_msg((cprBuffer_t)pmsg, msg_id, sizeof(*pmsg), dest_id);
}


cc_rcs_t
sub_send_msg (cprBuffer_t buf, uint32_t cmd, uint16_t len, cc_srcs_t dst_id)
{
    cpr_status_e rc;

    CC_DEBUG_MSG sub_print_msg((char *)buf, len);

But this sub_print_msg expects the buf to have at least an int on the front:

sub_print_msg (char *pData, int len)
{
    int ix;
    int msg_id = *((int *)pData);

So, I'm thinking I'll MOZ_ASSERT in sub_send_msg that the buffer size be at least >= sizeof(int) and see what hits that assert.  If this is the only case then I'll change the dummy var in terminate_req_all to an int.  

Going to test today and probably upload patch tomorrow.
Whiteboard: [WebRTC],[blocking-webrtc+]
Attachment #723972 - Flags: review?(rjesup)
Attachment #723972 - Flags: review?(rjesup) → review+
Comment on attachment 723972 [details] [diff] [review]
Enforce minimum buffer size in sub_send_msg

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

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

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

How likely is this patch to cause regressions; how much testing does it need?

*  How easily can the security issue be deduced from the patch?

It's not clear that this read of extra bytes could be exploitable since it gets immediately translated by cc_msg_name() I don't think the next bytes on the stack could be read.  Patch is very clear however.

*  Do comments in the patch, the check-in comment, or tests included
  in the patch paint a bulls-eye on the security problem?

I don't believe so.


*  Which older supported branches are affected by this flaw?

FF 18, but behind pref.


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

FF 18 Aurora


*  Do you have backports for the affected branches? If not, how different,
   hard to create, and risky will they be?

This fix could be upliftable to Aurora.


*  How likely is this patch to cause regressions; how much testing does
   it need?

I believe this is very low risk.  The most likely new problem would be that there is another case of a buffer < sizeof(int) passed in that would cause an assert to fire in debug or a message to be dropped in opt.
Attachment #723972 - Flags: sec-approval?
I also reviewed callers.  The only other non-structure sizes I could find being passed was "sizeof(void *)".  So, as long as sizeof(void *) <= sizeof(int), we're good.  I can't think of any current or planned target that would violate that - and with this patch it should fail cleanly.
When did we change the pref to not disable this by default? In other words, what versions of Firefox are affected out of the box without a pref change?
(In reply to Al Billings [:abillings] from comment #5)
> When did we change the pref to not disable this by default? In other words,
> what versions of Firefox are affected out of the box without a pref change?

FF 21 and FF 22 are preffed on by default. FF 20 is preffed off by default.
WebRTC Preffing on can be followed here - Bug 796463
Attachment #723972 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/838efa504a6b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
QA- and removing testcase keyword - there's no test case here showing a reproduction.
Keywords: testcase
Whiteboard: [WebRTC],[blocking-webrtc+] → [WebRTC],[blocking-webrtc+][qa-]
Explained in IRC - this is a unit test, so not relevant to QA. Adding the keyword back though for that case.
Keywords: testcase
(In reply to Al Billings [:abillings] from comment #5)
> When did we change the pref to not disable this by default? In other words,
> what versions of Firefox are affected out of the box without a pref change?

Not tracking for FF20 atm as WebRTC is preffed off by default, but tracking for FF21 and above.
Whiteboard: [WebRTC],[blocking-webrtc+][qa-] → [WebRTC],[blocking-webrtc+][qa-][webrtc-uplift]
Flags: in-testsuite+
(In reply to Ethan Hugg [:ehugg] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/838efa504a6b

Can you please get a patch ready for aurora here and nominate it with approval request ?
Comment on attachment 723972 [details] [diff] [review]
Enforce minimum buffer size in sub_send_msg

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 792188 import of Signaling code from Alder branch.

User impact if declined:

Probably incorrect log message, possible access violation.
 
Testing completed (on m-c, etc.):

Been on M-C since 3/13
 
Risk to taking this patch (and alternatives if risky): 

Possible new assertion in debug build if we missed any cases of short buffers.

String or UUID changes made by this patch: 
None.

This should apply cleanly no mozilla-aurora as of 3/26.
Attachment #723972 - Flags: approval-mozilla-aurora?
Attachment #723972 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [WebRTC],[blocking-webrtc+][qa-][webrtc-uplift] → [WebRTC],[blocking-webrtc+][qa-]
Whiteboard: [WebRTC],[blocking-webrtc+][qa-] → [WebRTC],[blocking-webrtc+][qa-][adv-main21-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.