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

[Michael Niedermayer]

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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210410/1e9bd937/attachment.sig>