[FFmpeg-devel,1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input

Submitted by Michael Niedermayer on Aug. 12, 2019, 7:17 p.m.

Details

Message ID 20190812191708.22608-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 12, 2019, 7:17 p.m.
Fixes: Timeout (12sec -> 32ms)
Fixes: 16078/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296

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

Comments

Tomas Härdin Aug. 14, 2019, 10:32 a.m.
mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> Fixes: Timeout (12sec -> 32ms)
> Fixes: 16078/clusterfuzz-testcase-minimized-
> ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/cinepak.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
> index aeb15de0ed..62eb794332 100644
> --- a/libavcodec/cinepak.c
> +++ b/libavcodec/cinepak.c
> @@ -356,6 +356,9 @@ static int cinepak_predecode_check
> (CinepakContext *s)
>      if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12)
>          return AVERROR_INVALIDDATA;
>  
> +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> +        return AVERROR_INVALIDDATA;

This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
could merge it with the check above into something like:

if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
    (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
0)) {
    return AVERROR_INVALIDDATA;
}

The check further down could also check each strip's size, not just the
first one.

Finally, I don't think we should accept files with num_strips >
MAX_STRIPS in cinepak_decode(). We should ask for samples of them
instead.

/Tomas
Tomas Härdin Aug. 15, 2019, 2:43 p.m.
ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > Fixes: Timeout (12sec -> 32ms)
> > Fixes: 16078/clusterfuzz-testcase-minimized-
> > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > 
> > Found-by: continuous fuzzing process 
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/cinepak.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
> > index aeb15de0ed..62eb794332 100644
> > --- a/libavcodec/cinepak.c
> > +++ b/libavcodec/cinepak.c
> > @@ -356,6 +356,9 @@ static int cinepak_predecode_check
> > (CinepakContext *s)
> >      if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12)
> >          return AVERROR_INVALIDDATA;
> >  
> > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > +        return AVERROR_INVALIDDATA;
> 
> This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> could merge it with the check above into something like:
> 
> if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
>     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> 0)) {
>     return AVERROR_INVALIDDATA;
> }
> 
> The check further down could also check each strip's size, not just the
> first one.

Actually, thinking a bit more about this, I suspect it might be legal
to have strips that don't cover the entire frame. It would certainly be
good to support that, for optimizing skip frames. Not sure what old
decoders make of that however. A skip could potentially be encoded in
22 + (width/4 + 7)/8 bytes while still being compatible.

I'd replace s->avctx->height in the expression with the height of the
first strip, to not constrain things too much.

/Tomas
Michael Niedermayer Aug. 15, 2019, 10:50 p.m.
On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > Fixes: Timeout (12sec -> 32ms)
> > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > 
> > > Found-by: continuous fuzzing process 
> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/cinepak.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
> > > index aeb15de0ed..62eb794332 100644
> > > --- a/libavcodec/cinepak.c
> > > +++ b/libavcodec/cinepak.c
> > > @@ -356,6 +356,9 @@ static int cinepak_predecode_check
> > > (CinepakContext *s)
> > >      if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12)
> > >          return AVERROR_INVALIDDATA;
> > >  
> > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > +        return AVERROR_INVALIDDATA;
> > 
> > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > could merge it with the check above into something like:
> > 
> > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > 0)) {
> >     return AVERROR_INVALIDDATA;
> > }
> > 
> > The check further down could also check each strip's size, not just the
> > first one.
> 
> Actually, thinking a bit more about this, I suspect it might be legal
> to have strips that don't cover the entire frame. It would certainly be
> good to support that, for optimizing skip frames. Not sure what old
> decoders make of that however. A skip could potentially be encoded in
> 22 + (width/4 + 7)/8 bytes while still being compatible.

i was asking myself the same questions before writing the patch, and
in the "spec" there is
"Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
"Each" meaning no holes to me. If thats actually true for all real files
that i do not know.


> 
> I'd replace s->avctx->height in the expression with the height of the
> first strip, to not constrain things too much.

ill post a patch doing that 

thx


[...]
Tomas Härdin Aug. 16, 2019, 12:57 p.m.
fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > Fixes: Timeout (12sec -> 32ms)
> > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > [...]
> > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > +        return AVERROR_INVALIDDATA;
> > > 
> > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > could merge it with the check above into something like:
> > > 
> > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > 0)) {
> > >     return AVERROR_INVALIDDATA;
> > > }
> > > 
> > > The check further down could also check each strip's size, not just the
> > > first one.
> > 
> > Actually, thinking a bit more about this, I suspect it might be legal
> > to have strips that don't cover the entire frame. It would certainly be
> > good to support that, for optimizing skip frames. Not sure what old
> > decoders make of that however. A skip could potentially be encoded in
> > 22 + (width/4 + 7)/8 bytes while still being compatible.
> 
> i was asking myself the same questions before writing the patch, and
> in the "spec" there is
> "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> "Each" meaning no holes to me. If thats actually true for all real files
> that i do not know.

We should keep in mind the spec is fine with there being zero strips.
It's only if one wants to support certain decoders that there will/must
be one or more strips.

I don't have any way of testing the MacOS decoder, but the old Win3.1
decoder is easy enough to get running. Then I can also check whether
strips narrower than avctx->width are OK.

> > I'd replace s->avctx->height in the expression with the height of the
> > first strip, to not constrain things too much.
> 
> ill post a patch doing that 

I feel like a better fix would be to make the decoder not choke on the
kind of files that trigger this. With this style of check it's still
possible to insert a second strip that triggers the slowness, with a
trivially sized strip before it.

Would you mind sending me the sample?

/Tomas
Tomas Härdin Aug. 16, 2019, 10:26 p.m.
fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin:
> fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > > Fixes: Timeout (12sec -> 32ms)
> > > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > > [...]
> > > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > > +        return AVERROR_INVALIDDATA;
> > > > 
> > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > > could merge it with the check above into something like:
> > > > 
> > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > > 0)) {
> > > >     return AVERROR_INVALIDDATA;
> > > > }
> > > > 
> > > > The check further down could also check each strip's size, not just the
> > > > first one.
> > > 
> > > Actually, thinking a bit more about this, I suspect it might be legal
> > > to have strips that don't cover the entire frame. It would certainly be
> > > good to support that, for optimizing skip frames. Not sure what old
> > > decoders make of that however. A skip could potentially be encoded in
> > > 22 + (width/4 + 7)/8 bytes while still being compatible.
> > 
> > i was asking myself the same questions before writing the patch, and
> > in the "spec" there is
> > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> > "Each" meaning no holes to me. If thats actually true for all real files
> > that i do not know.
> 
> We should keep in mind the spec is fine with there being zero strips.
> It's only if one wants to support certain decoders that there will/must
> be one or more strips.

I've been playing around with the Windows 3.1 cinepak decoder, and it
seems one indeed has to code every MB even if they don't change. I
suspect the reason is because that decoder uses double buffering and
they wanted to avoid doing more memcpy()s than absolutely necessary. If
one tries to play tricks like coding strips that are only 4x4 pixels to
indicate a frame without changes then parts of the video will be
replaced by garbage. So demanding number_of_bits >= number_of_mbs is
certainly in line with that decoder.

I feel I should point out that being conservative here is at odds with
the general "best effort" approach taken in this project. These toy
codecs are useful as illustrative examples of this contradiction. I'm
sure there are many more examples of files that can cause ffmpeg to do
a lot more work than expected, for almost every codec. I know afl-fuzz
is likely to find out that it can make the ZMBV decoder do a lot of
work for a small input file, because I've seen it do that with gzip.

The user base for cinepak is of course miniscule, so I doubt anyone's
going to complain that their broken CVID files don't work any more. I
certainly don't care since cinepakenc only puts out valid files. But
again, for other formats we're just going to have to tell users to put
ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is
vulnerable to DoS-y things.

/Tomas
Michael Niedermayer Aug. 17, 2019, 3:21 p.m.
On Fri, Aug 16, 2019 at 02:57:42PM +0200, Tomas Härdin wrote:
> fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > > Fixes: Timeout (12sec -> 32ms)
> > > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > > [...]
> > > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > > +        return AVERROR_INVALIDDATA;
> > > > 
> > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > > could merge it with the check above into something like:
> > > > 
> > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > > 0)) {
> > > >     return AVERROR_INVALIDDATA;
> > > > }
> > > > 
> > > > The check further down could also check each strip's size, not just the
> > > > first one.
> > > 
> > > Actually, thinking a bit more about this, I suspect it might be legal
> > > to have strips that don't cover the entire frame. It would certainly be
> > > good to support that, for optimizing skip frames. Not sure what old
> > > decoders make of that however. A skip could potentially be encoded in
> > > 22 + (width/4 + 7)/8 bytes while still being compatible.
> > 
> > i was asking myself the same questions before writing the patch, and
> > in the "spec" there is
> > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> > "Each" meaning no holes to me. If thats actually true for all real files
> > that i do not know.
> 
> We should keep in mind the spec is fine with there being zero strips.
> It's only if one wants to support certain decoders that there will/must
> be one or more strips.
> 
> I don't have any way of testing the MacOS decoder, but the old Win3.1
> decoder is easy enough to get running. Then I can also check whether
> strips narrower than avctx->width are OK.
> 
> > > I'd replace s->avctx->height in the expression with the height of the
> > > first strip, to not constrain things too much.
> > 
> > ill post a patch doing that 
> 
> I feel like a better fix would be to make the decoder not choke on the
> kind of files that trigger this. With this style of check it's still
> possible to insert a second strip that triggers the slowness, with a
> trivially sized strip before it.
> 

> Would you mind sending me the sample?

will send it privatly

[...]
Michael Niedermayer Aug. 17, 2019, 3:33 p.m.
On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote:
> fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin:
> > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > > > Fixes: Timeout (12sec -> 32ms)
> > > > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > > > [...]
> > > > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > > > +        return AVERROR_INVALIDDATA;
> > > > > 
> > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > > > could merge it with the check above into something like:
> > > > > 
> > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > > > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > > > 0)) {
> > > > >     return AVERROR_INVALIDDATA;
> > > > > }
> > > > > 
> > > > > The check further down could also check each strip's size, not just the
> > > > > first one.
> > > > 
> > > > Actually, thinking a bit more about this, I suspect it might be legal
> > > > to have strips that don't cover the entire frame. It would certainly be
> > > > good to support that, for optimizing skip frames. Not sure what old
> > > > decoders make of that however. A skip could potentially be encoded in
> > > > 22 + (width/4 + 7)/8 bytes while still being compatible.
> > > 
> > > i was asking myself the same questions before writing the patch, and
> > > in the "spec" there is
> > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> > > "Each" meaning no holes to me. If thats actually true for all real files
> > > that i do not know.
> > 
> > We should keep in mind the spec is fine with there being zero strips.
> > It's only if one wants to support certain decoders that there will/must
> > be one or more strips.
> 
> I've been playing around with the Windows 3.1 cinepak decoder, and it
> seems one indeed has to code every MB even if they don't change. I
> suspect the reason is because that decoder uses double buffering and
> they wanted to avoid doing more memcpy()s than absolutely necessary. If
> one tries to play tricks like coding strips that are only 4x4 pixels to
> indicate a frame without changes then parts of the video will be
> replaced by garbage. So demanding number_of_bits >= number_of_mbs is
> certainly in line with that decoder.
> 
> I feel I should point out that being conservative here is at odds with
> the general "best effort" approach taken in this project. These toy
> codecs are useful as illustrative examples of this contradiction. I'm
> sure there are many more examples of files that can cause ffmpeg to do
> a lot more work than expected, for almost every codec. I know afl-fuzz
> is likely to find out that it can make the ZMBV decoder do a lot of
> work for a small input file, because I've seen it do that with gzip.
> 
> The user base for cinepak is of course miniscule, so I doubt anyone's
> going to complain that their broken CVID files don't work any more. I
> certainly don't care since cinepakenc only puts out valid files. 

> But
> again, for other formats we're just going to have to tell users to put
> ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is
> vulnerable to DoS-y things.

yes

the question ATM is just what to do here about this codec ?
apply the patch ?
change it ?
check the first slice as in:
@@ -359,7 +359,16 @@ static int cinepak_predecode_check (CinepakContext *s)
     if (num_strips) {
         const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes;
         int strip_size = AV_RB24 (data + 1);
-        if (strip_size < 12 || strip_size > encoded_buf_size)
+
+        if (!(s->strips[0].y1 = AV_RB16 (&data[4])))
+            s->strips[0].y2 = (s->strips[0].y1 = 0) + AV_RB16 (&data[8]);
+        else
+            s->strips[0].y2 = AV_RB16 (&data[8]);
+
+        if (strip_size < 12 || strip_size > encoded_buf_size ||
+            s->strips[0].y2 <= s->strips[0].y1 ||
+            ((s->avctx->width * (int64_t)(s->strips[0].y2 - s->strips[0].y1)) / 16 + 7)/8  > s->size
+        )
             return AVERROR_INVALIDDATA;
     }

just raise the "threshold" in tools/target_dec_fuzzer.c for cinepak?
something else ?

This is not really a technical question, its a question what is preferred.

thx

[...]
Tomas Härdin Aug. 18, 2019, 12:35 a.m.
lör 2019-08-17 klockan 17:33 +0200 skrev Michael Niedermayer:
> On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote:
> > fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin:
> > > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> > > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > > > > Fixes: Timeout (12sec -> 32ms)
> > > > > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > > > > [...]
> > > > > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > > > > +        return AVERROR_INVALIDDATA;
> > > > > > 
> > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > > > > could merge it with the check above into something like:
> > > > > > 
> > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > > > > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > > > > 0)) {
> > > > > >     return AVERROR_INVALIDDATA;
> > > > > > }
> > > > > > 
> > > > > > The check further down could also check each strip's size, not just the
> > > > > > first one.
> > > > > 
> > > > > Actually, thinking a bit more about this, I suspect it might be legal
> > > > > to have strips that don't cover the entire frame. It would certainly be
> > > > > good to support that, for optimizing skip frames. Not sure what old
> > > > > decoders make of that however. A skip could potentially be encoded in
> > > > > 22 + (width/4 + 7)/8 bytes while still being compatible.
> > > > 
> > > > i was asking myself the same questions before writing the patch, and
> > > > in the "spec" there is
> > > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> > > > "Each" meaning no holes to me. If thats actually true for all real files
> > > > that i do not know.
> > > 
> > > We should keep in mind the spec is fine with there being zero strips.
> > > It's only if one wants to support certain decoders that there will/must
> > > be one or more strips.
> > 
> > I've been playing around with the Windows 3.1 cinepak decoder, and it
> > seems one indeed has to code every MB even if they don't change. I
> > suspect the reason is because that decoder uses double buffering and
> > they wanted to avoid doing more memcpy()s than absolutely necessary. If
> > one tries to play tricks like coding strips that are only 4x4 pixels to
> > indicate a frame without changes then parts of the video will be
> > replaced by garbage. So demanding number_of_bits >= number_of_mbs is
> > certainly in line with that decoder.
> > 
> > I feel I should point out that being conservative here is at odds with
> > the general "best effort" approach taken in this project. These toy
> > codecs are useful as illustrative examples of this contradiction. I'm
> > sure there are many more examples of files that can cause ffmpeg to do
> > a lot more work than expected, for almost every codec. I know afl-fuzz
> > is likely to find out that it can make the ZMBV decoder do a lot of
> > work for a small input file, because I've seen it do that with gzip.
> > 
> > The user base for cinepak is of course miniscule, so I doubt anyone's
> > going to complain that their broken CVID files don't work any more. I
> > certainly don't care since cinepakenc only puts out valid files. 
> > But
> > again, for other formats we're just going to have to tell users to put
> > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is
> > vulnerable to DoS-y things.
> 
> yes
> 
> the question ATM is just what to do here about this codec ?
> apply the patch ?
> change it ?

Well for a start, the file is 65535 x 209 pixels, 3166 frames. I
wouldn't call decoding that @ 263 fps particularly slow

Second, it's not the decoder which is slow. If I comment out the
"*got_frame = 1;" then the test also runs fast. I'm not sure what
happens elsewhere with the decoded buffer, but I suspect there's a
bunch of useless malloc()/memset()ing going on. Maybe the decoder is
using ff_reget_buffer() or av_frame_ref() incorrectly, I'm not sure.

As I said on IRC, this class of problems will exist for every codec.
Cinepak is easy to decode, even at these resolutions. Just imagine what
will happens when someone feeds in a 65535x209 av1 stream..

/Tomas
Michael Niedermayer Aug. 18, 2019, 10:13 a.m.
On Sun, Aug 18, 2019 at 02:35:45AM +0200, Tomas Härdin wrote:
> lör 2019-08-17 klockan 17:33 +0200 skrev Michael Niedermayer:
> > On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote:
> > > fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin:
> > > > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer:
> > > > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote:
> > > > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin:
> > > > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > > > > > > Fixes: Timeout (12sec -> 32ms)
> > > > > > > > Fixes: 16078/clusterfuzz-testcase-minimized-
> > > > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296
> > > > > > > > [...]
> > > > > > > > +    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
> > > > > > > > +        return AVERROR_INVALIDDATA;
> > > > > > > 
> > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You
> > > > > > > could merge it with the check above into something like:
> > > > > > > 
> > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 +
> > > > > > >     (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 :
> > > > > > > 0)) {
> > > > > > >     return AVERROR_INVALIDDATA;
> > > > > > > }
> > > > > > > 
> > > > > > > The check further down could also check each strip's size, not just the
> > > > > > > first one.
> > > > > > 
> > > > > > Actually, thinking a bit more about this, I suspect it might be legal
> > > > > > to have strips that don't cover the entire frame. It would certainly be
> > > > > > good to support that, for optimizing skip frames. Not sure what old
> > > > > > decoders make of that however. A skip could potentially be encoded in
> > > > > > 22 + (width/4 + 7)/8 bytes while still being compatible.
> > > > > 
> > > > > i was asking myself the same questions before writing the patch, and
> > > > > in the "spec" there is
> > > > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors."
> > > > > "Each" meaning no holes to me. If thats actually true for all real files
> > > > > that i do not know.
> > > > 
> > > > We should keep in mind the spec is fine with there being zero strips.
> > > > It's only if one wants to support certain decoders that there will/must
> > > > be one or more strips.
> > > 
> > > I've been playing around with the Windows 3.1 cinepak decoder, and it
> > > seems one indeed has to code every MB even if they don't change. I
> > > suspect the reason is because that decoder uses double buffering and
> > > they wanted to avoid doing more memcpy()s than absolutely necessary. If
> > > one tries to play tricks like coding strips that are only 4x4 pixels to
> > > indicate a frame without changes then parts of the video will be
> > > replaced by garbage. So demanding number_of_bits >= number_of_mbs is
> > > certainly in line with that decoder.
> > > 
> > > I feel I should point out that being conservative here is at odds with
> > > the general "best effort" approach taken in this project. These toy
> > > codecs are useful as illustrative examples of this contradiction. I'm
> > > sure there are many more examples of files that can cause ffmpeg to do
> > > a lot more work than expected, for almost every codec. I know afl-fuzz
> > > is likely to find out that it can make the ZMBV decoder do a lot of
> > > work for a small input file, because I've seen it do that with gzip.
> > > 
> > > The user base for cinepak is of course miniscule, so I doubt anyone's
> > > going to complain that their broken CVID files don't work any more. I
> > > certainly don't care since cinepakenc only puts out valid files. 
> > > But
> > > again, for other formats we're just going to have to tell users to put
> > > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is
> > > vulnerable to DoS-y things.
> > 
> > yes
> > 
> > the question ATM is just what to do here about this codec ?
> > apply the patch ?
> > change it ?
> 
> Well for a start, the file is 65535 x 209 pixels, 3166 frames. I
> wouldn't call decoding that @ 263 fps particularly slow
> 
> Second, it's not the decoder which is slow. If I comment out the
> "*got_frame = 1;" then the test also runs fast. I'm not sure what
> happens elsewhere with the decoded buffer, but I suspect there's a
> bunch of useless malloc()/memset()ing going on. Maybe the decoder is
> using ff_reget_buffer() or av_frame_ref() incorrectly, I'm not sure.
> 
> As I said on IRC, this class of problems will exist for every codec.
> Cinepak is easy to decode, even at these resolutions. Just imagine what

> will happens when someone feeds in a 65535x209 av1 stream..

i dont know about av1 specifically without checking the specs
but with most of the mainstream codecs. areas not covered by 
slices are forbidden. So the case here of "nothing" wouldnt be allowed.
also many codecs,especially mainstream ones from mpeg/itu have
a maximum compression rate. some 16x16 block or other requiring 
a symbol or 2 to say its not the end of a slice and skiped. And the
arithmetic or range coder or vlc coder having a moderate number of
symbols per bit max.
so feeding really crazy resolutions into a decoder requires some
small but proportional amout of input bytes.
double the width and the minimum input bytes double sort of.

codecs OTOH which allow coding a whole frame in 10bytes input
independant of the resolution behave worse in this way 
as that can produce orders of magnitude more load per bandwidth
the attacker needs.

Thanks

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index aeb15de0ed..62eb794332 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -356,6 +356,9 @@  static int cinepak_predecode_check (CinepakContext *s)
     if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12)
         return AVERROR_INVALIDDATA;
 
+    if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8))
+        return AVERROR_INVALIDDATA;
+
     if (num_strips) {
         const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes;
         int strip_size = AV_RB24 (data + 1);