814734 - WebRTC format string vulnerability / crash [@CSFLogError]

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Eric Rescorla (:ekr)]

This is a format string mismatch. I have seen at least one other in this code.

Suhas, please audit this entire directory for such.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Assigning (for now at least) to Suhas; let me know if you're unavailable to knock this one off quickly.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 5

[Suhas]

I will audit the code ..

Comment 6

[Suhas]

Attachment: Fixed Log format string mismatches

Comment 7

[Suhas]

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.

Comment 8

[Ethan Hugg [:ehugg]]

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.

Comment 9

[Suhas]

If we think we are good here with respect to the changes .. should we go ahead and commit

Comment 10

[Eric Rescorla (:ekr)]

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

Comment 11

[Suhas]

Attachment: Fixed Log format string mismatches

Comment 12

[Suhas]

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

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 685185 [details] [diff] [review]
Fixed Log format string mismatches


https://hg.mozilla.org/integration/mozilla-inbound/rev/79f4a4713ce5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79f4a4713ce5