diff mbox

[FFmpeg-devel,1/2] lavc/hevc_mp4toannexb: Fix interger overflow

Message ID 20191203213356.17842-1-andriy.gelman@gmail.com
State New
Headers show

Commit Message

Andriy Gelman Dec. 3, 2019, 9:33 p.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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andreas Rheinhardt Dec. 3, 2019, 10:30 p.m. UTC | #1
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
Andriy Gelman Dec. 4, 2019, 12:09 a.m. UTC | #2
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
James Almer Dec. 4, 2019, 12:28 a.m. UTC | #3
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 mbox

Patch

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