WebRTC format string vulnerability / crash [@CSFLogError]

RESOLVED FIXED in Firefox 20

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: posidron, Assigned: snandaku)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla20
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 unaffected, firefox19 disabled, firefox20 fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [qa-][adv-main20-])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

7 years ago
Posted file callstack
MediaConduitErrorCode
WebrtcAudioConduit::ReceivedRTPPacket(const void *data, int len)
{
[...]
media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:486
      CSFLogError(logTag, "%s RTP Processing Error %d ", error);
[...]

The trigger is to set the length 'len' of an RTP packet lower than the actual length of 'data'. 

Tested with m-c changeset: 114069:30fb98343919
This is a format string mismatch. I have seen at least one other in this code.

Suhas, please audit this entire directory for such.
Assigning (for now at least) to Suhas; let me know if you're unavailable to knock this one off quickly.
Assignee: nobody → snandaku
I retract that.  If it's not a problem, I'd like to have Jan-Ivar learn the patch/checkin/etc ropes on this bug.  Yes, I know it's a security bug, but as he doesn't even have try access yet I'll be vetting all the Try runs and doing the checkin.
Assignee: snandaku → wingboy
After further thought and discussion, I retract that retraction - let's let Suhas deal with this one, and I'll find an even simpler first-bug for Jan-Ivar.
Assignee: wingboy → snandaku
Assignee

Comment 5

7 years ago
I will audit the code ..
Assignee

Comment 6

7 years ago
Assignee

Comment 7

7 years ago
Comment on attachment 684918 [details] [diff] [review]
Fixed Log format string mismatches

Fixed format mismatch while logging and few minor nits to make logging consistent.
Attachment #684918 - Flags: review?(rjesup)
Attachment #684918 - Flags: review?(ekr)
Attachment #684918 - Flags: review?(rjesup) → review+
This bug makes a case for using the stream versions of logging in the C++ code.  We had to #if DEBUG the stream versions during the merge since streams are not desired in the release code.   If that changes, we should consider going back to them.
Flags: in-testsuite-
Assignee

Comment 9

7 years ago
If we think we are good here with respect to the changes .. should we go ahead and commit
Comment on attachment 684918 [details] [diff] [review]
Fixed Log format string mismatches

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

lgtm

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +482,5 @@
>    {
>      if(mPtrVoENetwork->ReceivedRTPPacket(mChannel,data,len) == -1)
>      {
>        int error = mPtrVoEBase->LastError();
> +      CSFLogError(logTag, "%s RTP Processing Error %d ", __FUNCTION__, error);

Why a space between %d and "

@@ +508,5 @@
>    {
>      if(mPtrVoENetwork->ReceivedRTCPPacket(mChannel, data, len) == -1)
>      {
>        int error = mPtrVoEBase->LastError();
> +      CSFLogError(logTag, "%s RTCP Processing Error %d ", __FUNCTION__, error);

Same here
Attachment #684918 - Flags: review?(ekr) → review+
Assignee

Comment 11

7 years ago
Assignee

Updated

7 years ago
Attachment #684918 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #685185 - Flags: review?(ethanhugg)
Assignee

Comment 12

7 years ago
uploaded a fresh patch with above changes for Ethan or someone to commit it since we have r+ already
Assignee

Updated

7 years ago
Attachment #685185 - Flags: checkin?(ethanhugg)
Comment on attachment 685185 [details] [diff] [review]
Fixed Log format string mismatches


https://hg.mozilla.org/integration/mozilla-inbound/rev/79f4a4713ce5
Attachment #685185 - Flags: review?(ethanhugg)
Attachment #685185 - Flags: review+
Attachment #685185 - Flags: checkin?(ethanhugg)
Attachment #685185 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/79f4a4713ce5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.