diff mbox series

[FFmpeg-devel] avcodec/msp2dec: Check available space in RLE decoder

Message ID 20210406215022.13983-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avcodec/msp2dec: Check available space in RLE decoder
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer April 6, 2021, 9:50 p.m. UTC
Fixes: out of array read
Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/msp2dec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Peter Ross April 6, 2021, 10:33 p.m. UTC | #1
On Tue, Apr 06, 2021 at 11:50:22PM +0200, Michael Niedermayer wrote:
> Fixes: out of array read
> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/msp2dec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
> index cc548d218a..87057fb5e2 100644
> --- a/libavcodec/msp2dec.c
> +++ b/libavcodec/msp2dec.c
> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
>  
>          bytestream2_init(&gb, buf, pkt_size);
>          x = 0;
> -        while (bytestream2_get_bytes_left(&gb) && x < width) {
> +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
>              int size = bytestream2_get_byte(&gb);
>              if (size) {
> +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
>                  memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
>                  bytestream2_skip(&gb, size);
>              } else {

please apply

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Andreas Rheinhardt April 6, 2021, 10:42 p.m. UTC | #2
Michael Niedermayer:
> Fixes: out of array read
> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/msp2dec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
> index cc548d218a..87057fb5e2 100644
> --- a/libavcodec/msp2dec.c
> +++ b/libavcodec/msp2dec.c
> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
>  
>          bytestream2_init(&gb, buf, pkt_size);
>          x = 0;
> -        while (bytestream2_get_bytes_left(&gb) && x < width) {
> +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {

This decoder uses the checked bytestream2 API, so != 0 and > 0 should be
equivalent for bytestream2_get_bytes_left(&gb).

>              int size = bytestream2_get_byte(&gb);
>              if (size) {
> +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
>                  memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));

width can include seven bytes of the packet's padding, but it stays
within the padding, so I wonder where the out of array read comes from.
The only fishy thing in this decoder I see is that 2 * avctx->height
might overflow.

>                  bytestream2_skip(&gb, size);
>              } else {
>
Andreas Rheinhardt April 6, 2021, 10:50 p.m. UTC | #3
Andreas Rheinhardt:
> Michael Niedermayer:
>> Fixes: out of array read
>> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/msp2dec.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
>> index cc548d218a..87057fb5e2 100644
>> --- a/libavcodec/msp2dec.c
>> +++ b/libavcodec/msp2dec.c
>> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
>>  
>>          bytestream2_init(&gb, buf, pkt_size);
>>          x = 0;
>> -        while (bytestream2_get_bytes_left(&gb) && x < width) {
>> +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
> 
> This decoder uses the checked bytestream2 API, so != 0 and > 0 should be
> equivalent for bytestream2_get_bytes_left(&gb).
> 
>>              int size = bytestream2_get_byte(&gb);
>>              if (size) {
>> +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
>>                  memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
> 
> width can include seven bytes of the packet's padding, but it stays
> within the padding, so I wonder where the out of array read comes from.
> The only fishy thing in this decoder I see is that 2 * avctx->height
> might overflow.
> 

Correction: width has no relationship with the packet's size at all; the
width - x in the above FFMIN only ensures that it writes at most seven
bytes into the frame's padding (is the frame actually guaranteed to be
padded?).
Btw: I don't see any advantage of writing in the frame's padding.

- Andreas
Michael Niedermayer April 7, 2021, 2:59 p.m. UTC | #4
On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: out of array read
> > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/msp2dec.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
> > index cc548d218a..87057fb5e2 100644
> > --- a/libavcodec/msp2dec.c
> > +++ b/libavcodec/msp2dec.c
> > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
> >  
> >          bytestream2_init(&gb, buf, pkt_size);
> >          x = 0;
> > -        while (bytestream2_get_bytes_left(&gb) && x < width) {
> > +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
> 
> This decoder uses the checked bytestream2 API, so != 0 and > 0 should be
> equivalent for bytestream2_get_bytes_left(&gb).

I changed it to "> 0" because it felt clearer&more robust
i will drop that as its not needed for the bugfix


> 
> >              int size = bytestream2_get_byte(&gb);
> >              if (size) {
> > +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
> >                  memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
> 
> width can include seven bytes of the packet's padding, but it stays
> within the padding, so I wonder where the out of array read comes from.
> The only fishy thing in this decoder I see is that 2 * avctx->height
> might overflow.

size is a value read from the bytestream, theres no guarntee that
the amount that byte calls for is not beyond the end of the bytestream
and its not checked by memcpy

bytestream2_get_buffer() would check it and it can maybe be used instead

thx

[...]
Michael Niedermayer April 8, 2021, 3:35 p.m. UTC | #5
On Wed, Apr 07, 2021 at 04:59:09PM +0200, Michael Niedermayer wrote:
> On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote:
> > Michael Niedermayer:
> > > Fixes: out of array read
> > > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/msp2dec.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
> > > index cc548d218a..87057fb5e2 100644
> > > --- a/libavcodec/msp2dec.c
> > > +++ b/libavcodec/msp2dec.c
> > > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
> > >  
> > >          bytestream2_init(&gb, buf, pkt_size);
> > >          x = 0;
> > > -        while (bytestream2_get_bytes_left(&gb) && x < width) {
> > > +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
> > 
> > This decoder uses the checked bytestream2 API, so != 0 and > 0 should be
> > equivalent for bytestream2_get_bytes_left(&gb).
> 
> I changed it to "> 0" because it felt clearer&more robust
> i will drop that as its not needed for the bugfix
> 
> 
> > 
> > >              int size = bytestream2_get_byte(&gb);
> > >              if (size) {
> > > +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
> > >                  memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
> > 
> > width can include seven bytes of the packet's padding, but it stays
> > within the padding, so I wonder where the out of array read comes from.
> > The only fishy thing in this decoder I see is that 2 * avctx->height
> > might overflow.
> 
> size is a value read from the bytestream, theres no guarntee that
> the amount that byte calls for is not beyond the end of the bytestream
> and its not checked by memcpy

will apply and backport


[...]
James Almer April 10, 2021, 1:59 a.m. UTC | #6
On 4/7/2021 11:59 AM, Michael Niedermayer wrote:
> On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Fixes: out of array read
>>> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>   libavcodec/msp2dec.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
>>> index cc548d218a..87057fb5e2 100644
>>> --- a/libavcodec/msp2dec.c
>>> +++ b/libavcodec/msp2dec.c
>>> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
>>>   
>>>           bytestream2_init(&gb, buf, pkt_size);
>>>           x = 0;
>>> -        while (bytestream2_get_bytes_left(&gb) && x < width) {
>>> +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
>>
>> This decoder uses the checked bytestream2 API, so != 0 and > 0 should be
>> equivalent for bytestream2_get_bytes_left(&gb).
> 
> I changed it to "> 0" because it felt clearer&more robust
> i will drop that as its not needed for the bugfix
> 
> 
>>
>>>               int size = bytestream2_get_byte(&gb);
>>>               if (size) {
>>> +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
>>>                   memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
>>
>> width can include seven bytes of the packet's padding, but it stays
>> within the padding, so I wonder where the out of array read comes from.
>> The only fishy thing in this decoder I see is that 2 * avctx->height
>> might overflow.
> 
> size is a value read from the bytestream, theres no guarntee that
> the amount that byte calls for is not beyond the end of the bytestream
> and its not checked by memcpy
> 
> bytestream2_get_buffer() would check it and it can maybe be used instead

If you knew about it, why didn't you use it? This code here basically 
duplicates the three lines in bytestream2_get_buffer().

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer April 10, 2021, 9:21 p.m. UTC | #7
On Fri, Apr 09, 2021 at 10:59:44PM -0300, James Almer wrote:
> On 4/7/2021 11:59 AM, Michael Niedermayer wrote:
> > On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote:
> > > Michael Niedermayer:
> > > > Fixes: out of array read
> > > > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >   libavcodec/msp2dec.c | 3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
> > > > index cc548d218a..87057fb5e2 100644
> > > > --- a/libavcodec/msp2dec.c
> > > > +++ b/libavcodec/msp2dec.c
> > > > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx,
> > > >           bytestream2_init(&gb, buf, pkt_size);
> > > >           x = 0;
> > > > -        while (bytestream2_get_bytes_left(&gb) && x < width) {
> > > > +        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
> > > 
> > > This decoder uses the checked bytestream2 API, so != 0 and > 0 should be
> > > equivalent for bytestream2_get_bytes_left(&gb).
> > 
> > I changed it to "> 0" because it felt clearer&more robust
> > i will drop that as its not needed for the bugfix
> > 
> > 
> > > 
> > > >               int size = bytestream2_get_byte(&gb);
> > > >               if (size) {
> > > > +                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
> > > >                   memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
> > > 
> > > width can include seven bytes of the packet's padding, but it stays
> > > within the padding, so I wonder where the out of array read comes from.
> > > The only fishy thing in this decoder I see is that 2 * avctx->height
> > > might overflow.
> > 
> > size is a value read from the bytestream, theres no guarntee that
> > the amount that byte calls for is not beyond the end of the bytestream
> > and its not checked by memcpy
> > 
> > bytestream2_get_buffer() would check it and it can maybe be used instead
> 
> If you knew about it, why didn't you use it? This code here basically
> duplicates the three lines in bytestream2_get_buffer().

why i didnt use it, well ultimatly probable because i had a bugfix that
i tested and that solved the issue and that was easy to backport. And
noone asked me to include the simplification.
Of course no question when looking at it now, its prettier to replace
these 3 lines by bytestream2_get_buffer().
Thats what review is for, to spot these things. Which it now did.
Feel free to replace it or ill do it tomorrow if i dont forget

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c
index cc548d218a..87057fb5e2 100644
--- a/libavcodec/msp2dec.c
+++ b/libavcodec/msp2dec.c
@@ -68,9 +68,10 @@  static int msp2_decode_frame(AVCodecContext *avctx,
 
         bytestream2_init(&gb, buf, pkt_size);
         x = 0;
-        while (bytestream2_get_bytes_left(&gb) && x < width) {
+        while (bytestream2_get_bytes_left(&gb) > 0 && x < width) {
             int size = bytestream2_get_byte(&gb);
             if (size) {
+                size = FFMIN(size, bytestream2_get_bytes_left(&gb));
                 memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x));
                 bytestream2_skip(&gb, size);
             } else {