| Message ID | 20191203213356.17842-1-andriy.gelman@gmail.com |
|---|---|
| State | Withdrawn |
| Headers | show |
On Tue, Dec 3, 2019 at 10:41 PM Andriy Gelman <andriy.gelman@gmail.com> wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > Check packet grow size against INT_MAX instead of SIZE_MAX. > > Found with libFuzzer: > 4294967044 cannot be represented as int. > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavcodec/hevc_mp4toannexb_bsf.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c > b/libavcodec/hevc_mp4toannexb_bsf.c > index 09bce5b34c2..36fd6f0b15c 100644 > --- a/libavcodec/hevc_mp4toannexb_bsf.c > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > @@ -152,8 +152,8 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, > AVPacket *out) > extra_size = add_extradata * ctx->par_out->extradata_size; > got_irap |= is_irap; > > - if (SIZE_MAX - nalu_size < 4 || > - SIZE_MAX - 4 - nalu_size < extra_size) { > + if (INT_MAX < 4 + nalu_size || > + INT_MAX - 4 < extra_size + nalu_size) { > ret = AVERROR_INVALIDDATA; > goto fail; > } > -- > > 1. Typo in "Interger". 2. You did not really fix the overflow problem: If nalu_size is (say) UINT32_MAX, 4 + nalu_size is 3 (no undefined behaviour involved here) and if extra_size is > 0 and <= INT_MAX - 4, then we don't go to fail here. Your check merely ensures that (after possible (defined) overflow) 4 + nalu_size + extra_size is >= 0 and <= INT_MAX, so that the automatic conversion to int (the type of the parameter of av_grow_packet()) is lossless. But in a scenario outlined here, the memcpy operations that follow will surely segfault. 3. There is another problem here: There is no check that the NAL unit is actually contained in the packet. A packet of size 10 bytes could contain a NAL unit with a size field indicating a size of (say) 1 GB and it would be allocated; it would not segfault, though, because the safe version of the bytestream2 API is used. (I think I already told you this, didn't I?) - Andreas
On Tue, 03. Dec 23:30, Andreas Rheinhardt wrote: > On Tue, Dec 3, 2019 at 10:41 PM Andriy Gelman <andriy.gelman@gmail.com> > wrote: > > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > Check packet grow size against INT_MAX instead of SIZE_MAX. > > > > Found with libFuzzer: > > 4294967044 cannot be represented as int. > > > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > > --- > > libavcodec/hevc_mp4toannexb_bsf.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c > > b/libavcodec/hevc_mp4toannexb_bsf.c > > index 09bce5b34c2..36fd6f0b15c 100644 > > --- a/libavcodec/hevc_mp4toannexb_bsf.c > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > > @@ -152,8 +152,8 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, > > AVPacket *out) > > extra_size = add_extradata * ctx->par_out->extradata_size; > > got_irap |= is_irap; > > > > - if (SIZE_MAX - nalu_size < 4 || > > - SIZE_MAX - 4 - nalu_size < extra_size) { > > + if (INT_MAX < 4 + nalu_size || > > + INT_MAX - 4 < extra_size + nalu_size) { > > ret = AVERROR_INVALIDDATA; > > goto fail; > > } > > -- > > > > 1. Typo in "Interger". > 2. You did not really fix the overflow problem: If nalu_size is (say) > UINT32_MAX, 4 + nalu_size is 3 (no undefined behaviour involved here) and > if extra_size is > 0 and <= INT_MAX - 4, then we don't go to fail here. > Your check merely ensures that (after possible (defined) overflow) 4 + > nalu_size + extra_size is >= 0 and <= INT_MAX, so that the automatic > conversion to int (the type of the parameter of av_grow_packet()) is > lossless. But in a scenario outlined here, the memcpy operations that > follow will surely segfault. thanks, I've resent. > 3. There is another problem here: There is no check that the NAL unit is > actually contained in the packet. A packet of size 10 bytes could contain a > NAL unit with a size field indicating a size of (say) 1 GB and it would be > allocated; it would not segfault, though, because the safe version of the > bytestream2 API is used. (I think I already told you this, didn't I?) Yes true, but I didn't make this problem. You are welcome to send the patch. Thanks for looking over, Andriy
On 12/3/2019 9:09 PM, Andriy Gelman wrote: > On Tue, 03. Dec 23:30, Andreas Rheinhardt wrote: >> On Tue, Dec 3, 2019 at 10:41 PM Andriy Gelman <andriy.gelman@gmail.com> >> wrote: >> >>> From: Andriy Gelman <andriy.gelman@gmail.com> >>> >>> Check packet grow size against INT_MAX instead of SIZE_MAX. >>> >>> Found with libFuzzer: >>> 4294967044 cannot be represented as int. >>> >>> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> >>> --- >>> libavcodec/hevc_mp4toannexb_bsf.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c >>> b/libavcodec/hevc_mp4toannexb_bsf.c >>> index 09bce5b34c2..36fd6f0b15c 100644 >>> --- a/libavcodec/hevc_mp4toannexb_bsf.c >>> +++ b/libavcodec/hevc_mp4toannexb_bsf.c >>> @@ -152,8 +152,8 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, >>> AVPacket *out) >>> extra_size = add_extradata * ctx->par_out->extradata_size; >>> got_irap |= is_irap; >>> >>> - if (SIZE_MAX - nalu_size < 4 || >>> - SIZE_MAX - 4 - nalu_size < extra_size) { >>> + if (INT_MAX < 4 + nalu_size || >>> + INT_MAX - 4 < extra_size + nalu_size) { >>> ret = AVERROR_INVALIDDATA; >>> goto fail; >>> } >>> -- >>> >>> 1. Typo in "Interger". > >> 2. You did not really fix the overflow problem: If nalu_size is (say) >> UINT32_MAX, 4 + nalu_size is 3 (no undefined behaviour involved here) and >> if extra_size is > 0 and <= INT_MAX - 4, then we don't go to fail here. >> Your check merely ensures that (after possible (defined) overflow) 4 + >> nalu_size + extra_size is >= 0 and <= INT_MAX, so that the automatic >> conversion to int (the type of the parameter of av_grow_packet()) is >> lossless. But in a scenario outlined here, the memcpy operations that >> follow will surely segfault. > > thanks, I've resent. > >> 3. There is another problem here: There is no check that the NAL unit is >> actually contained in the packet. A packet of size 10 bytes could contain a >> NAL unit with a size field indicating a size of (say) 1 GB and it would be >> allocated; it would not segfault, though, because the safe version of the >> bytestream2 API is used. (I think I already told you this, didn't I?) > > Yes true, but I didn't make this problem. You are welcome to send the patch. > > Thanks for looking over, > Andriy For whatever reason, the new patchset you sent has ffmpeg-devel@ffmpeg.org in the in-reply-to field, which makes clients like mine attach it to some unrelated thread.
diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c index 09bce5b34c2..36fd6f0b15c 100644 --- a/libavcodec/hevc_mp4toannexb_bsf.c +++ b/libavcodec/hevc_mp4toannexb_bsf.c @@ -152,8 +152,8 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) extra_size = add_extradata * ctx->par_out->extradata_size; got_irap |= is_irap; - if (SIZE_MAX - nalu_size < 4 || - SIZE_MAX - 4 - nalu_size < extra_size) { + if (INT_MAX < 4 + nalu_size || + INT_MAX - 4 < extra_size + nalu_size) { ret = AVERROR_INVALIDDATA; goto fail; }