Prevent size_t underflow in H264 SPS parsing.

webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 18 matching lines @@
   // General note: this is based off the 02/2014 version of the H.264 standard.
   // You can find it on this page:
   // http://www.itu.int/rec/T-REC-H.264
 
   const char* sps_bytes = reinterpret_cast<const char*>(sps_);
   // First, parse out rbsp, which is basically the source buffer minus emulation
   // bytes (the last byte of a 0x00 0x00 0x03 sequence). RBSP is defined in
   // section 7.3.1 of the H.264 standard.
   rtc::ByteBuffer rbsp_buffer;
   for (size_t i = 0; i < byte_length_;) {
-    if (i < byte_length_ - 3 &&
-        sps_[i] == 0 && sps_[i + 1] == 0 && sps_[i + 2] == 3) {
+    if (i + 3 < byte_length_ && sps_[i] == 0 && sps_[i + 1] == 0 &&

[stefan-webrtc, 2015/07/01 09:59:13]

Interesting bug. :)

[noahric, 2015/07/01 16:34:38]

I think this introduced a separate bug, though you'll never hit it (or the one
it fixed) in practice. You should have just added a byte_length_ >= 3 check at
the front.

The new bug is that if byte_length_ is really near size_t max, then i + 3 will
wrap at various points, so you could end up with an infinite loop:
sps_[0] == 3
...
sps_[byte_length_ - 1] == 0
sps_[byte_length_] == 0
and i == byte_length_ - 1 and byte_length_ == size_t max, then:

i + 3 == 1, which is < byte_length_
sps_[i] == 0, sps[i + 1] == 0, sps[i + 2] == 3
So it'd write 0, 0, then do i+=3, which makes i == 1 again.

(I'm still on pain meds, so that may be not quite right, but please take another
look)

[pbos-webrtc, 2015/07/01 18:12:29]

So just to clarify: The bug I fixed gets hit from malicious/corrupt packets. The
overflow bug you're mentioned gets hit if there's a packet near SIZE_MAX which I
think can never happen in practice for a bunch of reasons; not sure if any
system will ever let you allocate SIZE_MAX bytes due to malloc overhead etc. in
which case there's no block that can be fed here. On any systems we support
SIZE_MAX is going to be 4GB or above, and there's no way of putting that on the
wire afaict. A jumbogram will theoretically do it, but if it's encapsulated by
RTP, then RTP's header is > 3, so this theoretical overflow won't ever happen in
RTP.

If you can fix this theoretical overflow without making the code confusing I'm
all for it, but I think even checking i against SIZE_MAX makes this confusing.
If there's another overall h264 packetization max size that we can check in the
h264 depacketizer that sounds like a better way to do it.

[noahric, 2015/07/01 18:49:24]

I think the most readable fix is:

if (byte_length_ - i >= 3 ...

Meant to be read as: "If there are at least 3 bytes left in the sequence".

Since i is always < byte_length_, that shouldn't underflow, and it'll prevent
overflow since byte_length_ can't be greater than SIZE_MAX.

That also fixes an existing bug where a 0 0 3 at the end of the rbsp would get
skipped. That's not a legal sps anyways, so it'd probably fail for other reasons
and give the correct result, but seems sensible to fix it here.

[pbos-webrtc, 2015/07/02 07:37:50]

Slightly less readable in my opinion, but I won't fight about this one if you
want to push it in.
I have no idea what this is or even means. It should only be affected if it's
near SIZE_MAX, correct?

+        sps_[i + 2] == 3) {
       // Two rbsp bytes + the emulation byte.
       rbsp_buffer.WriteBytes(sps_bytes + i, 2);
       i += 3;
     } else {
       // Single rbsp byte.
       rbsp_buffer.WriteBytes(sps_bytes + i, 1);
       i++;
     }
   }
 
@@ skipping 168 matching lines @@
   // Subtract the crop for each dimension.
   width -= (frame_crop_left_offset + frame_crop_right_offset);
   height -= (frame_crop_top_offset + frame_crop_bottom_offset);
 
   width_ = width;
   height_ = height;
   return true;
 }
 
 }  // namespace webrtc