Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1219493004:
Prevent size_t underflow in H264 SPS parsing. (Closed)
|
Created:
5 years, 5 months ago by pbos-webrtc Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman, noahric-google Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPrevent size_t underflow in H264 SPS parsing.
BUG=webrtc:4771
R=stefan@webrtc.org
Committed: https://crrev.com/4daa90eed7591f37d7d157f9ec5000d83272a604
Cr-Commit-Position: refs/heads/master@{#9523}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 16 (4 generated)
PTAL
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1219493004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stefan@webrtc.org
lgtm +cc noahric, you may want to know about this bug as it might be worth solving in other places if you have been reusing this code. https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc (right): https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc:39: if (i + 3 < byte_length_ && sps_[i] == 0 && sps_[i + 1] == 0 && Interesting bug. :)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1219493004/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4daa90eed7591f37d7d157f9ec5000d83272a604 Cr-Commit-Position: refs/heads/master@{#9523}
Message was sent while issue was closed.
noahric@chromium.org changed reviewers: + noahric@chromium.org
Message was sent while issue was closed.
I think you traded an unlikely bug for an unlikely bug that causes an infinite loop :) Please take a look at my comment. https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc (right): https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc:39: if (i + 3 < byte_length_ && sps_[i] == 0 && sps_[i + 1] == 0 && On 2015/07/01 09:59:13, stefan-webrtc (holmer) wrote: > Interesting bug. :) 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)
Message was sent while issue was closed.
On 2015/07/01 16:34:39, noahric wrote: > I think you traded an unlikely bug for an unlikely bug that causes an infinite > loop :) Please take a look at my comment. > > https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... > File webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc (right): > > https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... > webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc:39: if (i + 3 < byte_length_ > && sps_[i] == 0 && sps_[i + 1] == 0 && > On 2015/07/01 09:59:13, stefan-webrtc (holmer) wrote: > > Interesting bug. :) > > 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) I believe that assuming that byte_length_ is at least 4 less than SIZE_MAX is a reasonable implicit assumption, in this case no loops can occur.
Message was sent while issue was closed.
https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc (right): https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc:39: if (i + 3 < byte_length_ && sps_[i] == 0 && sps_[i + 1] == 0 && On 2015/07/01 16:34:38, noahric wrote: > On 2015/07/01 09:59:13, stefan-webrtc (holmer) wrote: > > Interesting bug. :) > > 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) 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.
Message was sent while issue was closed.
https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc (right): https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc:39: if (i + 3 < byte_length_ && sps_[i] == 0 && sps_[i + 1] == 0 && 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.
Message was sent while issue was closed.
https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc (right): https://codereview.webrtc.org/1219493004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/h264_sps_parser.cc:39: if (i + 3 < byte_length_ && sps_[i] == 0 && sps_[i + 1] == 0 && On 2015/07/01 18:49:24, noahric wrote: > 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". Slightly less readable in my opinion, but I won't fight about this one if you want to push it in. > 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. I have no idea what this is or even means. It should only be affected if it's near SIZE_MAX, correct? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
