1219814 - Overflow in RtpHeaderParser::Parse can cause memory-safety bug

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[q1]

RtpHeaderParser::Parse (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_utility.cc) can overflow while parsing the packet extension:

378:  if (X) {
379:    /* RTP header extension, RFC 3550.
370:     0                   1                   2                   3
381:     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
382:    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
383:    |      defined by profile       |           length              |
384:    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
385:    |                        header extension                       |
386:    |                             ....                              |
387:    */
388:    const ptrdiff_t remain = _ptrRTPDataEnd - ptr;
389:    if (remain < 4) {
390:      return false;
391:    }
392:
393:    header.headerLength += 4;
394:
395:    uint16_t definedByProfile = *ptr++ << 8;
396:    definedByProfile += *ptr++;
397:
398:    uint16_t XLen = *ptr++ << 8;
399:    XLen += *ptr++; // in 32 bit words
400:    XLen *= 4; // in octs
401:
402:    if (remain < (4 + XLen)) {
403:      return false;
404:    }
405:    if (definedByProfile == kRtpOneByteHeaderExtensionId) {
406:      const uint8_t* ptrRTPDataExtensionEnd = ptr + XLen;
407:      ParseOneByteExtensionHeader(header,
408:                                  ptrExtensionMap,
409:                                  ptrRTPDataExtensionEnd,
410:                                  ptr);
411:    }
412:    header.headerLength += XLen;
413:  }
414:  return true;
415:}

This can occur if the 2 length bytes fetched in lines 398-99 total >= 0x4000. Choosing exactly 0x4000, line 400 then computes 0. This passes the test on line 402, has no impact on lines 405-11, |leaves header.headerLength| on line 412 unchanged, and causes success status to be returned.

Later, other code (such as srtp_unprotect; see https://bugzilla.mozilla.org/show_bug.cgi?id=1216837 and especially https://bugzilla.mozilla.org/show_bug.cgi?id=1216837#c3) can fetch the same length bytes from the packet and use them without further validation, since RtpHeaderParser::Parse supposedly validated them, causing potential reads and writes beyond the packet bounds.

Comment 1

[q1]

This bug is still present in the current trunk: http://hg.mozilla.org/mozilla-central/file/ac68828c5e3e/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc

Comment 2

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

See bug 1220493, which has a patch to fix that and this together.

Comment 3

[Daniel Veditz [:dveditz]]

bounty decision waiting for an answer to bug 1220493 comment 17

Comment 4

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

Fixed in bug 1220493.  See my comment there.

Comment 5

[Daniel Veditz [:dveditz]]

According to bug 1220493 comment 18 this bug was actually fixed upstream independently which is technically a dupe. The bounty committee has decided to award our smaller "moderate" bounty as a thank-you anyway.