1298125 - Missing bounds check in RtpHeaderParser::ParseOneByteExtensionHeader() causes reads beyond buffer end

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[q1]

RtpHeaderParser::ParseOneByteExtensionHeader() (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_utility.cc) reads data from a packet extension without first checking whether the packet is long enough to contain the data.

This omission can cause the function to read data beyond the packet's end.

I am unsure whether this bug can be manifested.

This bug still exists in trunk.

Details:

370: void RtpHeaderParser::ParseOneByteExtensionHeader(
371:     RTPHeader& header,
372:     const RtpHeaderExtensionMap* ptrExtensionMap,
373:     const uint8_t* ptrRTPDataExtensionEnd,
374:     const uint8_t* ptr) const {
375:   if (!ptrExtensionMap) {
376:     return;
377:   }
378: 

Line 379 validates the reads on line 388-89...

379:   while (ptrRTPDataExtensionEnd - ptr > 0) {
380:     //  0
381:     //  0 1 2 3 4 5 6 7
382:     // +-+-+-+-+-+-+-+-+
383:     // |  ID   |  len  |
384:     // +-+-+-+-+-+-+-+-+
385: 
386:     // Note that 'len' is the header extension element length, which is the
387:     // number of bytes - 1.
388:     const uint8_t id = (*ptr & 0xf0) >> 4;
389:     const uint8_t len = (*ptr & 0x0f);
390:     ptr++;
391: ...
398:     RTPExtensionType type;
399:     if (ptrExtensionMap->GetType(id, &type) != 0) {
400:       // If we encounter an unknown extension, just skip over it.
401:       LOG(LS_WARNING) << "Failed to find extension id: "
402:                       << static_cast<int>(id);
403:     } else {
404:       switch (type) {

...but none of the cases check |len| (read from the packet on line 389) against |ptrRTPDataExtensionEnd - ptr| to see whether the packet extension contains enough data to allow |len| bytes to be read. So, for example, lines 417-18, below, read 3 bytes from the location at |ptr|, even though |ptrRTPDataExtensionEnd - ptr| would == 0 if, on entry, |ptrRTPDataExtensionEnd - ptr| had been 1:

405:         case kRtpExtensionTransmissionTimeOffset: {
406:           if (len != 2) {
407:             LOG(LS_WARNING) << "Incorrect transmission time offset len: "
408:                             << len;
409:             return;
410:           }
411:           //  0                   1                   2                   3
412:           //  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
413:           // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
414:           // |  ID   | len=2 |              transmission offset              |
415:           // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
416: 
417:           header.extension.transmissionTimeOffset =
418:               ByteReader<int32_t, 3>::ReadBigEndian(ptr);
419:           header.extension.hasTransmissionTimeOffset = true;
420:           break;
421:         }

All of the remaining cases do something similar, including the last, which reads a string:

493:         case kRtpExtensionRID: {
494:           //   0                   1                   2
495:           //   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3
496:           //  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
497:           //  |  ID   | L=?   |UTF-8 RID value......          |...
498:           //  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
499: 
500:           // TODO(jesup) - avoid allocating on each packet - high watermark the RID buffer?
501:           char* ptrRID = new char[len+1];
502:           memcpy(ptrRID, ptr, len);
503:           ptrRID[len] = '\0';
504:           header.extension.rid = ptrRID;
505:           header.extension.hasRID = true;
506:           break;
507:         }
...
512:       }
513:     }
514:     ptr += (len + 1);
515:     uint8_t num_bytes = ParsePaddingBytes(ptrRTPDataExtensionEnd, ptr);
516:     ptr += num_bytes;
517:   }
518: }

Comment 1

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

Attachment: check rtp header extension lengths

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8787772 [details] [diff] [review]
check rtp header extension lengths

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

LGTM

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
@@ +387,5 @@
>      // number of bytes - 1.
>      const uint8_t id = (*ptr & 0xf0) >> 4;
>      const uint8_t len = (*ptr & 0x0f);
>      ptr++;
> +    if (ptrRTPDataExtensionEnd - (ptr + len+1) < 0) {

This works fine (at least in gcc) but I would feel more comfortable if you rewrote it to avoid the subtraction and use '>='. I'm not sure offhand if this sort of pointer arithmetic is guaranteed to be treated as signed by the standard or if this is implementation specific.

Comment 3

[q1]

(In reply to Dan Minor [:dminor] from comment #2)
> Comment on attachment 8787772 [details] [diff] [review]
> check rtp header extension lengths
> 
> Review of attachment 8787772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
> @@ +387,5 @@
> >      // number of bytes - 1.
> >      const uint8_t id = (*ptr & 0xf0) >> 4;
> >      const uint8_t len = (*ptr & 0x0f);
> >      ptr++;
> > +    if (ptrRTPDataExtensionEnd - (ptr + len+1) < 0) {
> 
> This works fine (at least in gcc) but I would feel more comfortable if you
> rewrote it to avoid the subtraction and use '>='. I'm not sure offhand if
> this sort of pointer arithmetic is guaranteed to be treated as signed by the
> standard or if this is implementation specific.

Such pointer math technically is defined only if both elements are within the array, and there is no under-/overflow. See C++1 s.5.7(6).

Comment 4

[q1]

Ack! "if both elements are within the array" should read "if both elements are within the array or one element past its end".

Comment 5

[q1]

Ack ack! "C++1" should read "C++11". I need more coffee.

Comment 6

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

(In reply to q1 from comment #3)
> (In reply to Dan Minor [:dminor] from comment #2)
> > Comment on attachment 8787772 [details] [diff] [review]
> > check rtp header extension lengths
> > 
> > Review of attachment 8787772 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > LGTM
> > 
> > ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
> > @@ +387,5 @@
> > >      // number of bytes - 1.
> > >      const uint8_t id = (*ptr & 0xf0) >> 4;
> > >      const uint8_t len = (*ptr & 0x0f);
> > >      ptr++;
> > > +    if (ptrRTPDataExtensionEnd - (ptr + len+1) < 0) {
> > 
> > This works fine (at least in gcc) but I would feel more comfortable if you
> > rewrote it to avoid the subtraction and use '>='. I'm not sure offhand if
> > this sort of pointer arithmetic is guaranteed to be treated as signed by the
> > standard or if this is implementation specific.
> 
> Such pointer math technically is defined only if both elements are within
> the array, and there is no under-/overflow. See C++1 s.5.7(6).

Well, this is as-defined as the line above "while (ptrRTPDataExtensionEnd - ptr > 0) {" -- if that works, then this does.  And as both are parameters to the function, and presumably the calling code must pass both as ptrs into the same array, this should be ok, even if it's not great code.  ("while (ptr < ptrRTPDataExtensionEnd) {" would be smarter.)  I could redo this comparison to be similar.

Comment 7

[q1]

(In reply to Randell Jesup [:jesup] from comment #6)
> (In reply to q1 from comment #3)
> > (In reply to Dan Minor [:dminor] from comment #2)
> > > Comment on attachment 8787772 [details] [diff] [review]
> > > check rtp header extension lengths
> > > 
> > > Review of attachment 8787772 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > LGTM
> > > 
> > > ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
> > > @@ +387,5 @@
> > > >      // number of bytes - 1.
> > > >      const uint8_t id = (*ptr & 0xf0) >> 4;
> > > >      const uint8_t len = (*ptr & 0x0f);
> > > >      ptr++;
> > > > +    if (ptrRTPDataExtensionEnd - (ptr + len+1) < 0) {
> > > 
> > > This works fine (at least in gcc) but I would feel more comfortable if you
> > > rewrote it to avoid the subtraction and use '>='. I'm not sure offhand if
> > > this sort of pointer arithmetic is guaranteed to be treated as signed by the
> > > standard or if this is implementation specific.
> > 
> > Such pointer math technically is defined only if both elements are within
> > the array, and there is no under-/overflow. See C++1 s.5.7(6).
> 
> Well, this is as-defined as the line above "while (ptrRTPDataExtensionEnd -
> ptr > 0) {" -- if that works, then this does.  And as both are parameters to
> the function, and presumably the calling code must pass both as ptrs into
> the same array, this should be ok, even if it's not great code.  ("while
> (ptr < ptrRTPDataExtensionEnd) {" would be smarter.)  I could redo this
> comparison to be similar.

I will guess that this kind of pointer math works on all practical architectures, despite it being technically undefined. But one case that will fail is if one of the pointer values wraps around the address space. Technically that could occur in the expression |ptr + len+1|, though because |len| <= 0x0f, it is very unlikely.

I think an expression of the form

   if (pArrayEnd - pArrayElement < numArrayElementsNeeded) {
      return failure();
   }

is Standard-compliant and also easy to understand.

Comment 8

[Dan Minor [:dminor]]

(In reply to Randell Jesup [:jesup] from comment #6)
> (In reply to q1 from comment #3)
> > (In reply to Dan Minor [:dminor] from comment #2)
> > > Comment on attachment 8787772 [details] [diff] [review]
> > > check rtp header extension lengths
> > > 
> > > Review of attachment 8787772 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > LGTM
> > > 
> > > ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
> > > @@ +387,5 @@
> > > >      // number of bytes - 1.
> > > >      const uint8_t id = (*ptr & 0xf0) >> 4;
> > > >      const uint8_t len = (*ptr & 0x0f);
> > > >      ptr++;
> > > > +    if (ptrRTPDataExtensionEnd - (ptr + len+1) < 0) {
> > > 
> > > This works fine (at least in gcc) but I would feel more comfortable if you
> > > rewrote it to avoid the subtraction and use '>='. I'm not sure offhand if
> > > this sort of pointer arithmetic is guaranteed to be treated as signed by the
> > > standard or if this is implementation specific.
> > 
> > Such pointer math technically is defined only if both elements are within
> > the array, and there is no under-/overflow. See C++1 s.5.7(6).
> 
> Well, this is as-defined as the line above "while (ptrRTPDataExtensionEnd -
> ptr > 0) {" -- if that works, then this does.  And as both are parameters to
> the function, and presumably the calling code must pass both as ptrs into
> the same array, this should be ok, even if it's not great code.  ("while
> (ptr < ptrRTPDataExtensionEnd) {" would be smarter.)  I could redo this
> comparison to be similar.

Sorry, I misread the while loop on my first pass and thought that ptr was only incremented by one in the body. That's not the case, so this must be fine as written.

Comment 9

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

NI to myself to land this

Comment 10

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

The webrtc.org 57 update includes a version of this fix.