[FFmpeg-devel] [PATCH] nutdec: reject negative sm_size

[Michael Niedermayer]

On Sat, Dec 19, 2015 at 02:25:42PM +0100, Andreas Cadhalpun wrote:
> On 19.12.2015 13:58, Michael Niedermayer wrote:
> > On Sat, Dec 19, 2015 at 12:19:44PM +0100, Andreas Cadhalpun wrote:
> >> On 19.12.2015 01:51, Michael Niedermayer wrote:
> >>> On Fri, Dec 18, 2015 at 05:22:31PM +0100, Andreas Cadhalpun wrote:
> >>>> If it is negative, it makes size larger than the size of the packet
> >>>> buffer, causing invalid writes in avio_read.
> >>>>
> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >>>> ---
> >>>>  libavformat/nutdec.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
> >>>> index 286d1ee..47ae7a7 100644
> >>>> --- a/libavformat/nutdec.c
> >>>> +++ b/libavformat/nutdec.c
> >>>> @@ -1146,6 +1146,11 @@ static int decode_frame(NUTContext *nut, AVPacket *pkt, int frame_code)
> >>>>              goto fail;
> >>>>          }
> >>>>          sm_size = avio_tell(bc) - pkt->pos;
> >>>> +        if (sm_size < 0) {
> >>>
> >>> did sm_size overflow and should be 64bit ?
> >>
> >> No.
> >>
> >>> did the byte position (avio_tell) move backward ?
> >>
> >> Yes.
> >>
> >>> (this should not happen)
> >>
> >> In that case, the check needs to be moved to read_sm_data.
> >> Patch doing that is attached.
> >>
> >> Best regards,
> >> Andreas
> >>
> > 
> >>  nutdec.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 2f0ac251ee05a8a36ffbaec5a9d5346ac0ef4240  0001-nutdec-reject-negative-value_len-in-read_sm_data.patch
> >> From bdca159087d426f5f989656a08464fec967b4bc3 Mon Sep 17 00:00:00 2001
> >> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> Date: Sat, 19 Dec 2015 12:02:56 +0100
> >> Subject: [PATCH] nutdec: reject negative value_len in read_sm_data
> >>
> >> If it is negative, it can cause the byte position to move backwards in
> >> avio_skip, which in turn makes sm_size negative and thus size larger
> >> than the size of the packet buffer, causing invalid writes in avio_read.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavformat/nutdec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
> >> index 286d1ee..b33b3e2 100644
> >> --- a/libavformat/nutdec.c
> >> +++ b/libavformat/nutdec.c
> >> @@ -934,7 +934,7 @@ static int read_sm_data(AVFormatContext *s, AVIOContext *bc, AVPacket *pkt, int
> >>                  return ret;
> >>              }
> >>              value_len = ffio_read_varlen(bc);
> >> -            if (avio_tell(bc) + value_len >= maxpos)
> >> +            if (value_len < 0 || avio_tell(bc) + value_len >= maxpos)
> >>                  return AVERROR_INVALIDDATA;
> > 
> > ok, also while at it please fix avio_tell(bc) + value_len, which i
> > belive can overflow
> 
> OK, updated patch attached.
> 
> > you could alternatively change value_len to uint64_t which might
> > simplify the check
> 
> It wouldn't help, because either avio_tell(bc) + value_len can be too large
> or maxpos - avio_tell(bc) negative, which would be treated as a large
> unsigned value, if value_len is unsigned.
> 
> Best regards,
> Andreas
> 

>  nutdec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> d1813b6394c006a3f235e5e9a5fb8f5172933736  0001-nutdec-reject-negative-value_len-in-read_sm_data.patch
> From 98fc98ce850d4d7fce30ee653dd48c063f0eaae6 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Sat, 19 Dec 2015 12:02:56 +0100
> Subject: [PATCH] nutdec: reject negative value_len in read_sm_data
> 
> If it is negative, it can cause the byte position to move backwards in
> avio_skip, which in turn makes sm_size negative and thus size larger
> than the size of the packet buffer, causing invalid writes in avio_read.
> 
> Also fix potential overflow of avio_tell(bc) + value_len.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
>  libavformat/nutdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

LGTM

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151219/860fd2cb/attachment.sig>