diff mbox

[FFmpeg-devel,v2,1/3] lavc/hevc_mp4toannexb: Fix integer overflow

Message ID 20191204000411.29765-1-andriy.gelman@gmail.com
State Superseded
Headers show

Commit Message

Andriy Gelman Dec. 4, 2019, 12:04 a.m. UTC 
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(-)

Comments

Andreas Rheinhardt Dec. 4, 2019, 12:26 a.m. UTC | #1 
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;
>          }
>
>
Andriy Gelman Dec. 4, 2019, 2:42 a.m. UTC | #2 
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,
Andriy Gelman Dec. 4, 2019, 2:22 p.m. UTC | #3 
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 mbox

Patch

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;
         }