Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1182793002:
Prevent heap overflows for incorrect FEC packet lengths. (Closed)
|
Created:
5 years, 6 months ago by pbos-webrtc Modified:
4 years, 11 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPrevent heap overflows for incorrect FEC packet lengths.
Bugs found by manual inspection of code, not by fuzzing or packet
replays. At least one of them confirmed by local fuzzing.
BUG=chromium:496094, webrtc:4771
R=stefan@webrtc.org
Committed: https://crrev.com/2bad88d164754f1f0694e9fea1051e71b3cb5347
Cr-Commit-Position: refs/heads/master@{#9542}
Patch Set 1 #
Total comments: 2
Patch Set 2 : inject-garbage-lengths unittests #Patch Set 3 : fix leak #Patch Set 4 : rebase #
Total comments: 9
Patch Set 5 : feedback #Patch Set 6 : rebase + cast #
Messages
Total messages: 30 (8 generated)
PTAL and tell me what tests you want for this (and if they're easy to put in there).
On 2015/06/12 at 14:26:34, pbos wrote: > PTAL and tell me what tests you want for this (and if they're easy to put in there). I would put some tests in fec_receiver_unittests.cc. Appropriate tests would probably be something that triggers these code paths? :)
inject-garbage-lengths unittests
PTAL, I couldn't figure out how to easily inject exact offsets, but this at least adds tests that caught excessive incorrect recovered lengths as well as an asan crash.
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/1182793002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/7996)
fix leak
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/1182793002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1182793002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/1182793002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:639: // This is the first packet which we try to recover with. Do we verify that the length of the fec packet is long enough to contain the ulp header etc? Or is that already done earlier?
rebase
PTAL https://codereview.webrtc.org/1182793002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/1182793002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:639: // This is the first packet which we try to recover with. On 2015/06/25 15:23:50, stefan-webrtc (holmer) wrote: > Do we verify that the length of the fec packet is long enough to contain the ulp > header etc? Or is that already done earlier? Accounted for in FecReceiverImpl::AddReceivedRedPacket.
https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:147: BuildAndAddRedMediaPacket(*it); This doesn't really do any recovery, right? Can you move line 149 up here? https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:169: InjectGarbagePacketLength(10); Can you comment on why those offsets are interesting? https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:648: ByteReader<uint16_t>::ReadBigEndian(&fec_packet->pkt->data[10]); Assuming we have checked that the data is long enough somewhere already. https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:652: sizeof(fec_packet->pkt->data) - kFecHeaderSize - ulp_header_size)) { Maybe make consts of these and name them: kMaxRecoveredLength kMaxProtectedLength ?
PTAL https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:147: BuildAndAddRedMediaPacket(*it); On 2015/07/02 09:31:25, stefan-webrtc (holmer) wrote: > This doesn't really do any recovery, right? Can you move line 149 up here? Don't even think I should be running it (line 149), updated this segment along with comment, PTAL. https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:169: InjectGarbagePacketLength(10); On 2015/07/02 09:31:26, stefan-webrtc (holmer) wrote: > Can you comment on why those offsets are interesting? Done, also updated test names to reflect this. https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:648: ByteReader<uint16_t>::ReadBigEndian(&fec_packet->pkt->data[10]); On 2015/07/02 09:31:26, stefan-webrtc (holmer) wrote: > Assuming we have checked that the data is long enough somewhere already. Added a check for that we have room for the ULP header. https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:652: sizeof(fec_packet->pkt->data) - kFecHeaderSize - ulp_header_size)) { On 2015/07/02 09:31:26, stefan-webrtc (holmer) wrote: > Maybe make consts of these and name them: > kMaxRecoveredLength > kMaxProtectedLength > ? They aren't constants though? ulp_header_size varies on input. And tbh. I'm not sure it makes a good constant to hide the sizeofs because we're using buffer sizes which I think should be visible.
feedback
The CQ bit was checked by stefan@webrtc.org
lgtm https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/1182793002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:652: sizeof(fec_packet->pkt->data) - kFecHeaderSize - ulp_header_size)) { On 2015/07/02 13:54:55, pbos-webrtc wrote: > On 2015/07/02 09:31:26, stefan-webrtc (holmer) wrote: > > Maybe make consts of these and name them: > > kMaxRecoveredLength > > kMaxProtectedLength > > ? > > They aren't constants though? ulp_header_size varies on input. And tbh. I'm not > sure it makes a good constant to hide the sizeofs because we're using buffer > sizes which I think should be visible. Acknowledged.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182793002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn/builds/4486) win on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win/builds/8494) (exceeded global retry quota)
rebase + cast
The CQ bit was checked by pbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1182793002/#ps100001 (title: "rebase + cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182793002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2bad88d164754f1f0694e9fea1051e71b3cb5347 Cr-Commit-Position: refs/heads/master@{#9542} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
