Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Carl Eugen Hoyos]

2019-01-18 15:24 GMT+01:00, Derek Buitenhuis :

> To that end, I've opened a bug with oss-fuzz for some guidance:
>
> https://github.com/google/oss-fuzz/issues/2095

You are late to this party...

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Vittorio Giovara]

On Thu, Jan 17, 2019 at 6:34 PM Michael Niedermayer 
wrote:

> On Wed, Jan 16, 2019 at 09:05:18PM -0500, Vittorio Giovara wrote:
> > On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer
> 
> > wrote:
> >
> > > Fixes: Timeout
> > > Fixes:
> > >
> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > >
> > > Before:
> > >
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > > in 15423 ms
> > > After:
> > >
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > > in 190 ms
> > >
> > > Found-by: continuous fuzzing process
> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by
> > > <
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by
> >:
> > > Michael Niedermayer 
> > > ---
> > >  libavcodec/rscc.c | 7 ++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> > > index 7921f149ed..fa066afd7f 100644
> > > --- a/libavcodec/rscc.c
> > > +++ b/libavcodec/rscc.c
> > > @@ -64,6 +64,7 @@ typedef struct RsccContext {
> > >  /* zlib interaction */
> > >  uint8_t *inflated_buf;
> > >  uLongf inflated_size;
> > > +int valid_pixels
> > >  } RsccContext;
> > >
> > >  static av_cold int rscc_init(AVCodecContext *avctx)
> > > @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext
> *avctx,
> > > void *data,
> > >  memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
> > >  }
> > >
> > > -*got_frame = 1;
> > > +// We only return a picture when more than 5% is undameged, this
> > > avoids copying nearly broken frames around
> > > +if (ctx->valid_pixels < ctx->inflated_size)
> > > +ctx->valid_pixels += pixel_size;
> > > +if (ctx->valid_pixels >= ctx->inflated_size/20)
> > >
> >
> > this feels arbitrary and hackish, for very little gain, IMO
>
> Would you be ok with rejecting RSCC files without a keyframe ?
> or more precissely all frames before a keyframe and thus if there is
> no keyframe the whole file
> (that would be a superset of what this patch rejects)
>

If that could be achieved in a clean way without codec-specific additions,
sure why not. It could maybe exploit the AV_FRAME_FLAG_CORRUPT flag and
some other combination of codec flag to expose this feature. However I'm
still adamant at "fixing" issues in the code found by external code tools
in this way: at most it is the fuzzer that should be better instrumented to
accept longer or no timeouts for this codec.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Derek Buitenhuis]

On 17/01/2019 23:33, Michael Niedermayer wrote:
> Would you be ok with rejecting RSCC files without a keyframe ?
> or more precissely all frames before a keyframe and thus if there is
> no keyframe the whole file
> (that would be a superset of what this patch rejects)

This, to me, soundsp preferable, if hidden under ER being disable or something.

> 
> I suggested that arbitrary check above as it would allow decoding more data
> in case of a damaged or lost keyframe

[...]

> The gain these changes have is they increase the cost for an
> attacker in a DOS attack. (they also improve the efficiency of the fuzzer but
> thats secondary)

By this logic, every single optimization is an anti-DoS measure and can
be argued ad nauseam in favor of every such changed, regardless of cost.

> As is you can have a file with huge resolution where each frame only encodes
> 1 pixel in about a dozen bytes per frame. That transforms a very low cost of
> network bandwidth from an attacker to a rather large computational cost on
> anything processing such file.

You don't need to explain what a DoS is to us...

> Requiring more than 1 valid pixels in a billion or Requiring a valid keyframe
> would increase the cost for an attacker.

This is not codec specific, either.

> Also do you see a cleaner way to achive this as the author of this
> decoder ?

People are disagreeing with the premise, not the implementation.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Derek Buitenhuis]

On 18/01/2019 11:46, Carl Eugen Hoyos wrote:
> No, you are completely missing the point.

I am not. I fully understand the argument in favour of these,
I just don't agree.

> Possible security issues in this decoder will only be
> searched (and therefore found) if the decoder doesn't
> timeout quickly on damaged files.

I am aware, and I disagree with the premise of dumping all over
the code and its complexity/readability in order to make a particular
fuzzer happy, so we can be 100% sure it won't miss an issue.

To that end, I've opened a bug with oss-fuzz for some guidance:

https://github.com/google/oss-fuzz/issues/2095

> I assume this is the result of a (simple) cost-benefit-
> analysis by the people running the fuzzing systems.

Yes, the cost of them running the tests, not dev/complexity costs
for downstream.

> Nobody asks you to fix the issues, blocking them is an
> interesting concept security-wise.

It makes plenty of code horrible and unnecessarily complex, so you
cannot simply argue "well you're not the one fixing them so bugger 
off".

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Carl Eugen Hoyos]

2019-01-17 22:58 GMT+01:00, Derek Buitenhuis :
> On 17/01/2019 03:06, Carl Eugen Hoyos wrote:
>> You mean searching for security issues makes no sense?
>
> This isn't a security and it isn't a fix. It's a completely
> arbitrary statistic to make an arbitrary program happy.

No, you are completely missing the point.

Possible security issues in this decoder will only be
searched (and therefore found) if the decoder doesn't
timeout quickly on damaged files.
I assume this is the result of a (simple) cost-benefit-
analysis by the people running the fuzzing systems.

Nobody asks you to fix the issues, blocking them is an
interesting concept security-wise.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Michael Niedermayer]

On Wed, Jan 16, 2019 at 09:05:18PM -0500, Vittorio Giovara wrote:
> On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer 
> wrote:
> 
> > Fixes: Timeout
> > Fixes:
> > 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> >
> > Before:
> > clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > in 15423 ms
> > After:
> > clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > in 190 ms
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by
> > :
> > Michael Niedermayer 
> > ---
> >  libavcodec/rscc.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> > index 7921f149ed..fa066afd7f 100644
> > --- a/libavcodec/rscc.c
> > +++ b/libavcodec/rscc.c
> > @@ -64,6 +64,7 @@ typedef struct RsccContext {
> >  /* zlib interaction */
> >  uint8_t *inflated_buf;
> >  uLongf inflated_size;
> > +int valid_pixels
> >  } RsccContext;
> >
> >  static av_cold int rscc_init(AVCodecContext *avctx)
> > @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx,
> > void *data,
> >  memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
> >  }
> >
> > -*got_frame = 1;
> > +// We only return a picture when more than 5% is undameged, this
> > avoids copying nearly broken frames around
> > +if (ctx->valid_pixels < ctx->inflated_size)
> > +ctx->valid_pixels += pixel_size;
> > +if (ctx->valid_pixels >= ctx->inflated_size/20)
> >
> 
> this feels arbitrary and hackish, for very little gain, IMO

Would you be ok with rejecting RSCC files without a keyframe ?
or more precissely all frames before a keyframe and thus if there is
no keyframe the whole file
(that would be a superset of what this patch rejects)

I suggested that arbitrary check above as it would allow decoding more data
in case of a damaged or lost keyframe

The gain these changes have is they increase the cost for an
attacker in a DOS attack. (they also improve the efficiency of the fuzzer but
thats secondary)
As is you can have a file with huge resolution where each frame only encodes
1 pixel in about a dozen bytes per frame. That transforms a very low cost of
network bandwidth from an attacker to a rather large computational cost on
anything processing such file.
Requiring more than 1 valid pixels in a billion or Requiring a valid keyframe
would increase the cost for an attacker.

Also do you see a cleaner way to achive this as the author of this
decoder ?

Thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"I am not trying to be anyone's saviour, I'm trying to think about the
 future and not be sad" - Elon Musk



signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Derek Buitenhuis]

On 17/01/2019 03:06, Carl Eugen Hoyos wrote:
> You mean searching for security issues makes no sense?

This isn't a security and it isn't a fix. It's a completely
arbitrary statistic to make an arbitrary program happy.

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Paul B Mahol]

On 1/17/19, Michael Niedermayer  wrote:
> Fixes: Timeout
> Fixes:
> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>
> Before:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 15423 ms
> After:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 190 ms
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/rscc.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> index 7921f149ed..fa066afd7f 100644
> --- a/libavcodec/rscc.c
> +++ b/libavcodec/rscc.c
> @@ -64,6 +64,7 @@ typedef struct RsccContext {
>  /* zlib interaction */
>  uint8_t *inflated_buf;
>  uLongf inflated_size;
> +int valid_pixels
>  } RsccContext;
>
>  static av_cold int rscc_init(AVCodecContext *avctx)
> @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx,
> void *data,
>  memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
>  }
>
> -*got_frame = 1;
> +// We only return a picture when more than 5% is undameged, this avoids
> copying nearly broken frames around
> +if (ctx->valid_pixels < ctx->inflated_size)
> +ctx->valid_pixels += pixel_size;
> +if (ctx->valid_pixels >= ctx->inflated_size/20)
> +*got_frame = 1;
>
>  ret = avpkt->size;
>  end:
> --
> 2.20.1
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

NACK
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Carl Eugen Hoyos]

> Am 17.01.2019 um 03:05 schrieb Vittorio Giovara :
> 
> On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer 
> wrote:
> 
>> Fixes: Timeout
>> Fixes:
>> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>> 
>> Before:
>> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>> in 15423 ms
>> After:
>> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>> in 190 ms
>> 
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by
>> :
>> Michael Niedermayer 
>> ---
>> libavcodec/rscc.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
>> index 7921f149ed..fa066afd7f 100644
>> --- a/libavcodec/rscc.c
>> +++ b/libavcodec/rscc.c
>> @@ -64,6 +64,7 @@ typedef struct RsccContext {
>> /* zlib interaction */
>> uint8_t *inflated_buf;
>> uLongf inflated_size;
>> +int valid_pixels
>> } RsccContext;
>> 
>> static av_cold int rscc_init(AVCodecContext *avctx)
>> @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx,
>> void *data,
>> memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
>> }
>> 
>> -*got_frame = 1;
>> +// We only return a picture when more than 5% is undameged, this
>> avoids copying nearly broken frames around
>> +if (ctx->valid_pixels < ctx->inflated_size)
>> +ctx->valid_pixels += pixel_size;
>> +if (ctx->valid_pixels >= ctx->inflated_size/20)
>> 
> 
> this feels arbitrary and hackish, for very little gain, IMO

You mean searching for security issues makes no sense?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Vittorio Giovara]

On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer 
wrote:

> Fixes: Timeout
> Fixes:
> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>
> Before:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 15423 ms
> After:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 190 ms
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> :
> Michael Niedermayer 
> ---
>  libavcodec/rscc.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> index 7921f149ed..fa066afd7f 100644
> --- a/libavcodec/rscc.c
> +++ b/libavcodec/rscc.c
> @@ -64,6 +64,7 @@ typedef struct RsccContext {
>  /* zlib interaction */
>  uint8_t *inflated_buf;
>  uLongf inflated_size;
> +int valid_pixels
>  } RsccContext;
>
>  static av_cold int rscc_init(AVCodecContext *avctx)
> @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx,
> void *data,
>  memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
>  }
>
> -*got_frame = 1;
> +// We only return a picture when more than 5% is undameged, this
> avoids copying nearly broken frames around
> +if (ctx->valid_pixels < ctx->inflated_size)
> +ctx->valid_pixels += pixel_size;
> +if (ctx->valid_pixels >= ctx->inflated_size/20)
>

this feels arbitrary and hackish, for very little gain, IMO
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Kieran Kunhya]

On Thu, 17 Jan 2019 at 00:44 Michael Niedermayer 
wrote:

> Fixes: Timeout
> Fixes:
> 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
>
> Before:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 15423 ms
> After:
> clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> in 190 ms
>

Oh come on, this is getting a bit ridiculous now.

Kieran
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

[Michael Niedermayer]

Fixes: Timeout
Fixes: 
12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264

Before: 
clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264 
in 15423 ms
After:  
clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264 
in 190 ms

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
 libavcodec/rscc.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
index 7921f149ed..fa066afd7f 100644
--- a/libavcodec/rscc.c
+++ b/libavcodec/rscc.c
@@ -64,6 +64,7 @@ typedef struct RsccContext {
 /* zlib interaction */
 uint8_t *inflated_buf;
 uLongf inflated_size;
+int valid_pixels
 } RsccContext;
 
 static av_cold int rscc_init(AVCodecContext *avctx)
@@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx, void 
*data,
 memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
 }
 
-*got_frame = 1;
+// We only return a picture when more than 5% is undameged, this avoids 
copying nearly broken frames around
+if (ctx->valid_pixels < ctx->inflated_size)
+ctx->valid_pixels += pixel_size;
+if (ctx->valid_pixels >= ctx->inflated_size/20)
+*got_frame = 1;
 
 ret = avpkt->size;
 end:
-- 
2.20.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel