| Message ID | 20190117004248.14128-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, 17 Jan 2019 at 00:44 Michael Niedermayer <michael@niedermayer.cc> 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
On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer <michael@niedermayer.cc> 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 <michael@niedermayer.cc> > --- > 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
> Am 17.01.2019 um 03:05 schrieb Vittorio Giovara <vittorio.giovara@gmail.com>: > > On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer <michael@niedermayer.cc> > 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 <michael@niedermayer.cc> >> --- >> 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
On 1/17/19, Michael Niedermayer <michael@niedermayer.cc> 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 <michael@niedermayer.cc> > --- > 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
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
On Wed, Jan 16, 2019 at 09:05:18PM -0500, Vittorio Giovara wrote: > On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer <michael@niedermayer.cc> > 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 <michael@niedermayer.cc> > > --- > > 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 [...]
2019-01-17 22:58 GMT+01:00, Derek Buitenhuis <derek.buitenhuis@gmail.com>: > 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
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
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
On Thu, Jan 17, 2019 at 6:34 PM Michael Niedermayer <michael@niedermayer.cc> 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 > <michael@niedermayer.cc> > > 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 <michael@niedermayer.cc> > > > --- > > > 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.
2019-01-18 15:24 GMT+01:00, Derek Buitenhuis <derek.buitenhuis@gmail.com>: > 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
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:
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 <michael@niedermayer.cc> --- libavcodec/rscc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)