Message ID | 20191204000411.29765-1-andriy.gelman@gmail.com |
---|---|
State | Superseded |
Headers | show |
On Wed, Dec 4, 2019 at 1:04 AM 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 | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c > b/libavcodec/hevc_mp4toannexb_bsf.c > index 09bce5b34c2..bf4c25b3b7b 100644 > --- a/libavcodec/hevc_mp4toannexb_bsf.c > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > @@ -66,7 +66,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx) > for (j = 0; j < cnt; j++) { > int nalu_len = bytestream2_get_be16(&gb); > > - if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - > new_extradata_size) { > + if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > INT_MAX - > new_extradata_size) { > There is actually FF_MAX_EXTRADATA_SIZE constant to limit the size of extradata (it is chosen so small that one can parse the extradata via a GetBitContext). (But honestly, it is not used consistently.) > ret = AVERROR_INVALIDDATA; > goto fail; > } > @@ -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 + (uint64_t)nalu_size || > The cast is unnecessary: INT_MAX - 4 < nalu_size does the same. And INT_MAX - 4 can even be computed at compile-time. > + INT_MAX - 4 < extra_size + (uint64_t)nalu_size) { > If you check that the extradata is not too big (namely <= INT_MAX - 4), then you can simplify this to INT_MAX - 4 - extra_size < nalu_size. The left side is positive, so it does not wrap around when converting it to uint32_t. This makes the first check superfluous. But it only checks for overflow, not for the scenario of a NAL unit extending beyond the input packet. ret = AVERROR_INVALIDDATA; > goto fail; > } > >
On Wed, 04. Dec 01:26, Andreas Rheinhardt wrote: > On Wed, Dec 4, 2019 at 1:04 AM 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 | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c > > b/libavcodec/hevc_mp4toannexb_bsf.c > > index 09bce5b34c2..bf4c25b3b7b 100644 > > --- a/libavcodec/hevc_mp4toannexb_bsf.c > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > > @@ -66,7 +66,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx) > > for (j = 0; j < cnt; j++) { > > int nalu_len = bytestream2_get_be16(&gb); > > > > - if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - > > new_extradata_size) { > > + if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > INT_MAX - > > new_extradata_size) { > > > > There is actually FF_MAX_EXTRADATA_SIZE constant to limit the size of > extradata (it is chosen so small that one can parse the extradata via a > GetBitContext). (But honestly, it is not used consistently.) If we decide to use it should be a separate commit IMO. > > > > ret = AVERROR_INVALIDDATA; > > goto fail; > > } > > @@ -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 + (uint64_t)nalu_size || > > > > The cast is unnecessary: INT_MAX - 4 < nalu_size does the same. And INT_MAX > - 4 can even be computed at compile-time. > > > > + INT_MAX - 4 < extra_size + (uint64_t)nalu_size) { > > > > If you check that the extradata is not too big (namely <= INT_MAX - 4), > then you can simplify this to INT_MAX - 4 - extra_size < nalu_size. The > left side is positive, so it does not wrap around when converting it to > uint32_t. This makes the first check superfluous. Yes, that's a good idea. And I think that extra_size <= INT_MAX - 4 always holds because we pad extradata with AV_INPUT_PADDING_SIZE (and the checks are done in the initialization). But I'll double check. Thanks,
On Tue, 03. Dec 21:42, Andriy Gelman wrote: > On Wed, 04. Dec 01:26, Andreas Rheinhardt wrote: > > On Wed, Dec 4, 2019 at 1:04 AM 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 | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c > > > b/libavcodec/hevc_mp4toannexb_bsf.c > > > index 09bce5b34c2..bf4c25b3b7b 100644 > > > --- a/libavcodec/hevc_mp4toannexb_bsf.c > > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > > > @@ -66,7 +66,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx) > > > for (j = 0; j < cnt; j++) { > > > int nalu_len = bytestream2_get_be16(&gb); > > > > > > - if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - > > > new_extradata_size) { > > > + if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > INT_MAX - > > > new_extradata_size) { > > > > > > > > There is actually FF_MAX_EXTRADATA_SIZE constant to limit the size of > > extradata (it is chosen so small that one can parse the extradata via a > > GetBitContext). (But honestly, it is not used consistently.) > > If we decide to use it should be a separate commit IMO. > > > > > > > > ret = AVERROR_INVALIDDATA; > > > goto fail; > > > } > > > @@ -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 + (uint64_t)nalu_size || > > > > > > > The cast is unnecessary: INT_MAX - 4 < nalu_size does the same. And INT_MAX > > - 4 can even be computed at compile-time. > > > > > > > > + INT_MAX - 4 < extra_size + (uint64_t)nalu_size) { > > > > > > > If you check that the extradata is not too big (namely <= INT_MAX - 4), > > then you can simplify this to INT_MAX - 4 - extra_size < nalu_size. The > > left side is positive, so it does not wrap around when converting it to > > uint32_t. This makes the first check superfluous. > > Yes, that's a good idea. And I think that extra_size <= INT_MAX - 4 always holds > because we pad extradata with AV_INPUT_PADDING_SIZE (and the checks are done in > the initialization). But I'll double check. The above check also needs to take into account the current packet size, i.e.: INT_MAX - 4 - extra_size < (uint64_t)nalu_size + out->size I don't think it's possible to get rid of the cast on the rhs then. Thanks,
diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c index 09bce5b34c2..bf4c25b3b7b 100644 --- a/libavcodec/hevc_mp4toannexb_bsf.c +++ b/libavcodec/hevc_mp4toannexb_bsf.c @@ -66,7 +66,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx) for (j = 0; j < cnt; j++) { int nalu_len = bytestream2_get_be16(&gb); - if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) { + if (4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > INT_MAX - new_extradata_size) { ret = AVERROR_INVALIDDATA; goto fail; } @@ -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 + (uint64_t)nalu_size || + INT_MAX - 4 < extra_size + (uint64_t)nalu_size) { ret = AVERROR_INVALIDDATA; goto fail; }