| Message ID | 20180513230756.15878-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | a9dacdeea6168787a142209bd19fdd74aefc9dd6 |
| Headers | show |
On Mon, May 14, 2018 at 1:07 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: Timeout > Fixes: 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > This does not seem like an issue to fix, but a fundamental change in the behavior of the codec. It currently properly honors all frame types, including "empty" frames, which results in proper constant frame rate output. After the change, it just flat out ignores those and discards them without any further consideration. Does not sound like a fuzzing or security related change to me, and possibly also quite the wrong thing to do. - Hendrik
On Mon, May 14, 2018 at 01:31:42AM +0200, Hendrik Leppkes wrote: > On Mon, May 14, 2018 at 1:07 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > Fixes: Timeout > > Fixes: 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > This does not seem like an issue to fix, but a fundamental change in > the behavior of the codec. It currently properly honors all frame > types, including "empty" frames, which results in proper constant > frame rate output. > After the change, it just flat out ignores those and discards them > without any further consideration. > > Does not sound like a fuzzing or security related change to me, and > possibly also quite the wrong thing to do. having a small code that outputs empty frames is a denial of service issue (which is a security issue). The attacker can with almost no bandwidth requirement cause the decode side to spend almost arbitrary amounts of CPU doing nothing usefull. This problem is reduced when the copy case requires bandwidth proportional to the resolution. As in a code per macroblock to skip it. not one bit to make the player copy 1gb of data said differently this increases the cost to an attacker per CPU cycle she causes on the player. In this particular case it also looks like that this is not just a skip code but the decoder actually outputs empty frames on errors. besides security its also more efficient not to send cloned frames though the whole chain (which forces every component to do duplicated work on each). And we avoid this in other decoders as well. [...]
2018-05-14 1:07 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc>: > Fixes: Timeout > Fixes: > 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032 Looking at the fate change, my suspicion is that the change is a (very) good idea but the commit message could explain better that duplicating frames is generally not what libavcodec does, it should be left to the calling application. Carl Eugen
On Tue, May 15, 2018 at 11:02 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, May 14, 2018 at 01:31:42AM +0200, Hendrik Leppkes wrote: >> On Mon, May 14, 2018 at 1:07 AM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >> > Fixes: Timeout >> > Fixes: 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032 >> > >> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > >> >> This does not seem like an issue to fix, but a fundamental change in >> the behavior of the codec. It currently properly honors all frame >> types, including "empty" frames, which results in proper constant >> frame rate output. >> After the change, it just flat out ignores those and discards them >> without any further consideration. >> >> Does not sound like a fuzzing or security related change to me, and >> possibly also quite the wrong thing to do. > > having a small code that outputs empty frames is a denial of service > issue (which is a security issue). The attacker can with almost no bandwidth > requirement cause the decode side to spend almost arbitrary amounts of > CPU doing nothing usefull. > > This problem is reduced when the copy case requires bandwidth proportional > to the resolution. As in a code per macroblock to skip it. > not one bit to make the player copy 1gb of data > > said differently this increases the cost to an attacker per CPU cycle > she causes on the player. > > In this particular case it also looks like that this is not > just a skip code but the decoder actually outputs empty frames on errors. > > besides security its also more efficient not to send cloned frames > though the whole chain (which forces every component to do duplicated work > on each). And we avoid this in other decoders as well. > It makes no real difference if its less efficient or whatever - if a codec specification asks for this behavior, then our decoders should act accordingly. Generating vfr decoder output from a cfr input stream is not in any way a good idea. - Hendrik
2018-05-16 0:29 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>: > It makes no real difference if its less efficient or whatever - > if a codec specification asks for this behavior, then our > decoders should act accordingly. I wonder where this suddenly comes from? (I was away from my mail client when a similar argument was used a few weeks ago and I forgot later.) Luckily, many of our codecs do smarter things than the specifications asks for... Do we have a specification for qtrle? Carl Eugen
On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote: > 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>: > > > It makes no real difference if its less efficient or whatever - > > if a codec specification asks for this behavior, then our > > decoders should act accordingly. > > I wonder where this suddenly comes from? > (I was away from my mail client when a similar argument > was used a few weeks ago and I forgot later.) > Luckily, many of our codecs do smarter things > than the specifications asks for... > > Do we have a specification for qtrle? Iam only aware of the one "we", that is more correctly mike melanson wrote: https://multimedia.cx/qtrle.txt I think this invalidates the argument, so if i hear no objections then ill apply this patch in a few days thanks [...]
On Thu, May 17, 2018 at 11:49 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote: >> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>: >> >> > It makes no real difference if its less efficient or whatever - >> > if a codec specification asks for this behavior, then our >> > decoders should act accordingly. >> >> I wonder where this suddenly comes from? >> (I was away from my mail client when a similar argument >> was used a few weeks ago and I forgot later.) >> Luckily, many of our codecs do smarter things >> than the specifications asks for... >> >> Do we have a specification for qtrle? > > Iam only aware of the one "we", that is more correctly mike melanson > wrote: https://multimedia.cx/qtrle.txt > > I think this invalidates the argument, so if i hear no objections then > ill apply this patch in a few days > How does that invalidate the argument that you take a compressed CRF stream and suddenly decide to make VFR out of it? Because it does not. - Hendrik
2018-05-17 12:22 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>: > On Thu, May 17, 2018 at 11:49 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: >> On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote: >>> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>: >>> >>> > It makes no real difference if its less efficient or whatever - >>> > if a codec specification asks for this behavior, then our >>> > decoders should act accordingly. >>> >>> I wonder where this suddenly comes from? >>> (I was away from my mail client when a similar argument >>> was used a few weeks ago and I forgot later.) >>> Luckily, many of our codecs do smarter things >>> than the specifications asks for... >>> >>> Do we have a specification for qtrle? >> >> Iam only aware of the one "we", that is more correctly mike >> melanson wrote: https://multimedia.cx/qtrle.txt >> >> I think this invalidates the argument, so if i hear no objections >> then ill apply this patch in a few days > > How does that invalidate the argument that you take a compressed > CRF stream and suddenly decide to make VFR out of it? Apart from the fact that your argument was that there is a specification that we have to follow (an argument that should be used very carefully imo), the argument that we should do something less efficient instead of improving the efficiency does not seem very strong to me. Carl Eugen
On Thu, May 17, 2018 at 12:22:01PM +0200, Hendrik Leppkes wrote: > On Thu, May 17, 2018 at 11:49 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Wed, May 16, 2018 at 12:53:46AM +0200, Carl Eugen Hoyos wrote: > >> 2018-05-16 0:29 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>: > >> > >> > It makes no real difference if its less efficient or whatever - > >> > if a codec specification asks for this behavior, then our > >> > decoders should act accordingly. > >> > >> I wonder where this suddenly comes from? > >> (I was away from my mail client when a similar argument > >> was used a few weeks ago and I forgot later.) > >> Luckily, many of our codecs do smarter things > >> than the specifications asks for... > >> > >> Do we have a specification for qtrle? > > > > Iam only aware of the one "we", that is more correctly mike melanson > > wrote: https://multimedia.cx/qtrle.txt > > > > I think this invalidates the argument, so if i hear no objections then > > ill apply this patch in a few days > > > > How does that invalidate the argument that you take a compressed CRF > stream and suddenly decide to make VFR out of it? > Because it does not. We have always droped cloned fields and frames even where the specification unquestionably wants something different, when this is inefficient For example mpeg2 has its field and frame duplication information, we update timestamps according to this information but never duplicate fields/frames. If we imagine an alterantive reality where we did clone then the decoder would be 20% slower in cases where it differs maybe, and we would output many progressive videos as interlaced which could not be displayed nicely that way on the majority of display devices. So what we do for mpeg2 makes sense. And this too changes constant field rate mpeg2 into vfr now qtrle, is a niche codec, why should it be handled in a way that is 1. inefficient & slower 2. easier to mount a DOS attack against 3. inconsistent with the other decoders we have ? Maybe i just dont understand your concern Is it that theres some need in some application for something like regular heartbeat frames ? If so this is a issue for most VFR and especially subtitles, changes to qtrle wont really help or hurt this. It would require injecting regular frames or a flag that for every decoder clones frames if nothing occured beyond a threshold [...]
On Tue, May 15, 2018 at 11:48:46PM +0200, Carl Eugen Hoyos wrote: > 2018-05-14 1:07 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc>: > > Fixes: Timeout > > Fixes: > > 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032 > > Looking at the fate change, my suspicion is that the change > is a (very) good idea but the commit message could explain > better that duplicating frames is generally not what > libavcodec does, it should be left to the calling application. will repost with a more verbose commit message [...]
diff --git a/libavcodec/qtrle.c b/libavcodec/qtrle.c index 1b0d2016b5..670690d0a4 100644 --- a/libavcodec/qtrle.c +++ b/libavcodec/qtrle.c @@ -433,12 +433,10 @@ static int qtrle_decode_frame(AVCodecContext *avctx, int ret; bytestream2_init(&s->g, avpkt->data, avpkt->size); - if ((ret = ff_reget_buffer(avctx, s->frame)) < 0) - return ret; /* check if this frame is even supposed to change */ if (avpkt->size < 8) - goto done; + return avpkt->size; /* start after the chunk size */ bytestream2_seek(&s->g, 4, SEEK_SET); @@ -449,17 +447,20 @@ static int qtrle_decode_frame(AVCodecContext *avctx, /* if a header is present, fetch additional decoding parameters */ if (header & 0x0008) { if (avpkt->size < 14) - goto done; + return avpkt->size; start_line = bytestream2_get_be16(&s->g); bytestream2_skip(&s->g, 2); height = bytestream2_get_be16(&s->g); bytestream2_skip(&s->g, 2); if (height > s->avctx->height - start_line) - goto done; + return avpkt->size; } else { start_line = 0; height = s->avctx->height; } + if ((ret = ff_reget_buffer(avctx, s->frame)) < 0) + return ret; + row_ptr = s->frame->linesize[0] * start_line; switch (avctx->bits_per_coded_sample) { @@ -520,7 +521,6 @@ static int qtrle_decode_frame(AVCodecContext *avctx, memcpy(s->frame->data[1], s->pal, AVPALETTE_SIZE); } -done: if ((ret = av_frame_ref(data, s->frame)) < 0) return ret; *got_frame = 1; diff --git a/tests/ref/fate/qtrle-8bit b/tests/ref/fate/qtrle-8bit index 8da113d83e..27bb8aad71 100644 --- a/tests/ref/fate/qtrle-8bit +++ b/tests/ref/fate/qtrle-8bit @@ -4,169 +4,60 @@ #dimensions 0: 640x480 #sar 0: 0/1 0, 0, 0, 1, 921600, 0x1492e3ed -0, 1, 1, 1, 921600, 0x1492e3ed -0, 2, 2, 1, 921600, 0x1492e3ed 0, 3, 3, 1, 921600, 0x23ef4fc7 -0, 4, 4, 1, 921600, 0x23ef4fc7 0, 5, 5, 1, 921600, 0xe406d4be -0, 6, 6, 1, 921600, 0xe406d4be -0, 7, 7, 1, 921600, 0xe406d4be 0, 8, 8, 1, 921600, 0x62b8b5a1 -0, 9, 9, 1, 921600, 0x62b8b5a1 0, 10, 10, 1, 921600, 0x7d8ba674 -0, 11, 11, 1, 921600, 0x7d8ba674 -0, 12, 12, 1, 921600, 0x7d8ba674 0, 13, 13, 1, 921600, 0xfe666be7 -0, 14, 14, 1, 921600, 0xfe666be7 0, 15, 15, 1, 921600, 0x721baec0 -0, 16, 16, 1, 921600, 0x721baec0 -0, 17, 17, 1, 921600, 0x721baec0 0, 18, 18, 1, 921600, 0xc237180a -0, 19, 19, 1, 921600, 0xc237180a 0, 20, 20, 1, 921600, 0xf03a7482 -0, 21, 21, 1, 921600, 0xf03a7482 -0, 22, 22, 1, 921600, 0xf03a7482 0, 23, 23, 1, 921600, 0x5612a391 -0, 24, 24, 1, 921600, 0x5612a391 0, 25, 25, 1, 921600, 0x9dbcc46a -0, 26, 26, 1, 921600, 0x9dbcc46a -0, 27, 27, 1, 921600, 0x9dbcc46a 0, 28, 28, 1, 921600, 0xa128a5d5 -0, 29, 29, 1, 921600, 0xa128a5d5 0, 30, 30, 1, 921600, 0x63e0025c -0, 31, 31, 1, 921600, 0x63e0025c -0, 32, 32, 1, 921600, 0x63e0025c 0, 33, 33, 1, 921600, 0x262359ed -0, 34, 34, 1, 921600, 0x262359ed 0, 35, 35, 1, 921600, 0x343688e8 -0, 36, 36, 1, 921600, 0x343688e8 -0, 37, 37, 1, 921600, 0x343688e8 -0, 38, 38, 1, 921600, 0x343688e8 -0, 39, 39, 1, 921600, 0x343688e8 -0, 40, 40, 1, 921600, 0x343688e8 -0, 41, 41, 1, 921600, 0x343688e8 -0, 42, 42, 1, 921600, 0x343688e8 -0, 43, 43, 1, 921600, 0x343688e8 -0, 44, 44, 1, 921600, 0x343688e8 0, 45, 45, 1, 921600, 0xe4b29d57 -0, 46, 46, 1, 921600, 0xe4b29d57 -0, 47, 47, 1, 921600, 0xe4b29d57 0, 48, 48, 1, 921600, 0x198e8a4a -0, 49, 49, 1, 921600, 0x198e8a4a 0, 50, 50, 1, 921600, 0x0cad8dc9 -0, 51, 51, 1, 921600, 0x0cad8dc9 -0, 52, 52, 1, 921600, 0x0cad8dc9 0, 53, 53, 1, 921600, 0x1f74cf3d -0, 54, 54, 1, 921600, 0x1f74cf3d 0, 55, 55, 1, 921600, 0xec5b5449 -0, 56, 56, 1, 921600, 0xec5b5449 -0, 57, 57, 1, 921600, 0xec5b5449 0, 58, 58, 1, 921600, 0x39829711 -0, 59, 59, 1, 921600, 0x39829711 0, 60, 60, 1, 921600, 0x6de5b9c6 -0, 61, 61, 1, 921600, 0x6de5b9c6 -0, 62, 62, 1, 921600, 0x6de5b9c6 0, 63, 63, 1, 921600, 0x47b0e9d4 -0, 64, 64, 1, 921600, 0x47b0e9d4 0, 65, 65, 1, 921600, 0x756452b8 -0, 66, 66, 1, 921600, 0x756452b8 -0, 67, 67, 1, 921600, 0x756452b8 0, 68, 68, 1, 921600, 0x6fce3478 -0, 69, 69, 1, 921600, 0x6fce3478 0, 70, 70, 1, 921600, 0x372397cd -0, 71, 71, 1, 921600, 0x372397cd -0, 72, 72, 1, 921600, 0x372397cd 0, 73, 73, 1, 921600, 0xe3999ba1 -0, 74, 74, 1, 921600, 0xe3999ba1 0, 75, 75, 1, 921600, 0x6ba26b43 -0, 76, 76, 1, 921600, 0x6ba26b43 -0, 77, 77, 1, 921600, 0x6ba26b43 0, 78, 78, 1, 921600, 0x4e9ee49e -0, 79, 79, 1, 921600, 0x4e9ee49e 0, 80, 80, 1, 921600, 0xdb5fd6e7 -0, 81, 81, 1, 921600, 0xdb5fd6e7 -0, 82, 82, 1, 921600, 0xdb5fd6e7 0, 83, 83, 1, 921600, 0x8f2254a5 -0, 84, 84, 1, 921600, 0x8f2254a5 -0, 85, 85, 1, 921600, 0x8f2254a5 -0, 86, 86, 1, 921600, 0x8f2254a5 -0, 87, 87, 1, 921600, 0x8f2254a5 -0, 88, 88, 1, 921600, 0x8f2254a5 -0, 89, 89, 1, 921600, 0x8f2254a5 -0, 90, 90, 1, 921600, 0x8f2254a5 -0, 91, 91, 1, 921600, 0x8f2254a5 -0, 92, 92, 1, 921600, 0x8f2254a5 0, 93, 93, 1, 921600, 0x57e95c32 -0, 94, 94, 1, 921600, 0x57e95c32 0, 95, 95, 1, 921600, 0x41627a9b -0, 96, 96, 1, 921600, 0x41627a9b -0, 97, 97, 1, 921600, 0x41627a9b 0, 98, 98, 1, 921600, 0x7412dcee -0, 99, 99, 1, 921600, 0x7412dcee 0, 100, 100, 1, 921600, 0xaebe10ed -0, 101, 101, 1, 921600, 0xaebe10ed -0, 102, 102, 1, 921600, 0xaebe10ed 0, 103, 103, 1, 921600, 0x411a91f6 -0, 104, 104, 1, 921600, 0x411a91f6 0, 105, 105, 1, 921600, 0xb059df3f -0, 106, 106, 1, 921600, 0xb059df3f -0, 107, 107, 1, 921600, 0xb059df3f 0, 108, 108, 1, 921600, 0x4d6f5a77 -0, 109, 109, 1, 921600, 0x4d6f5a77 0, 110, 110, 1, 921600, 0xbbf06df4 -0, 111, 111, 1, 921600, 0xbbf06df4 -0, 112, 112, 1, 921600, 0xbbf06df4 0, 113, 113, 1, 921600, 0xe27f7bf6 -0, 114, 114, 1, 921600, 0xe27f7bf6 0, 115, 115, 1, 921600, 0xd7e8360e -0, 116, 116, 1, 921600, 0xd7e8360e -0, 117, 117, 1, 921600, 0xd7e8360e 0, 118, 118, 1, 921600, 0x1dd4c344 -0, 119, 119, 1, 921600, 0x1dd4c344 0, 120, 120, 1, 921600, 0x7995a7ce -0, 121, 121, 1, 921600, 0x7995a7ce -0, 122, 122, 1, 921600, 0x7995a7ce 0, 123, 123, 1, 921600, 0x2ef3c566 -0, 124, 124, 1, 921600, 0x2ef3c566 0, 125, 125, 1, 921600, 0xf296736e -0, 126, 126, 1, 921600, 0xf296736e -0, 127, 127, 1, 921600, 0xf296736e -0, 128, 128, 1, 921600, 0xf296736e -0, 129, 129, 1, 921600, 0xf296736e -0, 130, 130, 1, 921600, 0xf296736e -0, 131, 131, 1, 921600, 0xf296736e -0, 132, 132, 1, 921600, 0xf296736e -0, 133, 133, 1, 921600, 0xf296736e -0, 134, 134, 1, 921600, 0xf296736e 0, 135, 135, 1, 921600, 0x1a488311 -0, 136, 136, 1, 921600, 0x1a488311 -0, 137, 137, 1, 921600, 0x1a488311 0, 138, 138, 1, 921600, 0x9e28011b -0, 139, 139, 1, 921600, 0x9e28011b 0, 140, 140, 1, 921600, 0x84d1ea80 -0, 141, 141, 1, 921600, 0x84d1ea80 -0, 142, 142, 1, 921600, 0x84d1ea80 0, 143, 143, 1, 921600, 0x9ed41052 -0, 144, 144, 1, 921600, 0x9ed41052 0, 145, 145, 1, 921600, 0xd4db7206 -0, 146, 146, 1, 921600, 0xd4db7206 -0, 147, 147, 1, 921600, 0xd4db7206 0, 148, 148, 1, 921600, 0x55f695a9 -0, 149, 149, 1, 921600, 0x55f695a9 0, 150, 150, 1, 921600, 0x9d8c667f -0, 151, 151, 1, 921600, 0x9d8c667f -0, 152, 152, 1, 921600, 0x9d8c667f 0, 153, 153, 1, 921600, 0x9b6037ec -0, 154, 154, 1, 921600, 0x9b6037ec 0, 155, 155, 1, 921600, 0x57c5e835 -0, 156, 156, 1, 921600, 0x57c5e835 -0, 157, 157, 1, 921600, 0x57c5e835 0, 158, 158, 1, 921600, 0x476dad89 -0, 159, 159, 1, 921600, 0x476dad89 0, 160, 160, 1, 921600, 0xcfd6ad2b -0, 161, 161, 1, 921600, 0xcfd6ad2b -0, 162, 162, 1, 921600, 0xcfd6ad2b 0, 163, 163, 1, 921600, 0x3b372379 -0, 164, 164, 1, 921600, 0x3b372379 0, 165, 165, 1, 921600, 0x36f245f5 -0, 166, 166, 1, 921600, 0x36f245f5
Fixes: Timeout Fixes: 6383/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_QTRLE_fuzzer-6199846902956032 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/qtrle.c | 12 ++--- tests/ref/fate/qtrle-8bit | 109 -------------------------------------- 2 files changed, 6 insertions(+), 115 deletions(-)