| Message ID | 20211013161526.83708-1-mattias.wadman@gmail.com |
|---|---|
| State | Accepted |
| Commit | 49597300e87c5c4b2ca56c5b93930d92f64cdf5b |
| Headers | show |
| Series | [FFmpeg-devel,v1] libavcodec/flac_parser: Validate subframe zero bit and type | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Wed, Oct 13, 2021 at 6:15 PM Mattias Wadman <mattias.wadman@gmail.com> wrote: > Reduces the risk of finding false frames that happens to have valid values > and CRC. > > Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame > https://trac.ffmpeg.org/ticket/9185 > --- > libavcodec/flac_parser.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c > index d3d9c889a1..2c550507fc 100644 > --- a/libavcodec/flac_parser.c > +++ b/libavcodec/flac_parser.c > @@ -96,8 +96,34 @@ static int frame_header_is_valid(AVCodecContext *avctx, > const uint8_t *buf, > FLACFrameInfo *fi) > { > GetBitContext gb; > - init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8); > - return !ff_flac_decode_frame_header(avctx, &gb, fi, 127); > + uint8_t subframe_type; > + > + // header plus one byte from first subframe > + init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8 + 8); > + if (ff_flac_decode_frame_header(avctx, &gb, fi, 127)) { > + return 0; > + } > + // subframe zero bit > + if (get_bits1(&gb) != 0) { > + return 0; > + } > + // subframe type > + // 000000 : SUBFRAME_CONSTANT > + // 000001 : SUBFRAME_VERBATIM > + // 00001x : reserved > + // 0001xx : reserved > + // 001xxx : if(xxx <= 4) SUBFRAME_FIXED, xxx=order ; else reserved > + // 01xxxx : reserved > + // 1xxxxx : SUBFRAME_LPC, xxxxx=order-1 > + subframe_type = get_bits(&gb, 6); > + if (!(subframe_type == 0 || > + subframe_type == 1 || > + ((subframe_type >= 8) && (subframe_type <= 12)) || > + (subframe_type >= 32))) { > + return 0; > + } > + > + return 1; > } > > /** > -- > 2.29.2 > > Ping Maybe I should give some more context. I originally reported https://trac.ffmpeg.org/ticket/9185 after seeing decode errors 1-2 times a day in a system that decodes tens of thousands of FLAC files per day. All of the failing ones I've looked at decodes fine with the FLAC reference decoder and nearly all of them are encoded using "Switch Audio File Converter", but I've managed to make ffmpeg's FLAC encoder produce valid files that it fails on also. With the patch I haven't seen any errors for the last 2 weeks and I've also gone through older files and all have decoded fine. I've looked into solving this without adding more validation in the flac parser but failed. I can't see how to know the frame size without more or less decoding it. -Mattias
LGTM, will apply soon
Thanks for taking your time to review On Mon, Oct 18, 2021 at 5:16 PM Paul B Mahol <onemda@gmail.com> wrote: > LGTM, will apply soon > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Wed, Oct 13, 2021 at 06:15:26PM +0200, Mattias Wadman wrote: > Reduces the risk of finding false frames that happens to have valid values and CRC. > > Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame > https://trac.ffmpeg.org/ticket/9185 > --- > libavcodec/flac_parser.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) After this was applied out of array accesses appeared tools/target_dem_flac_fuzzer: Running 1 inputs 1 time(s) each. Running: libfuzz-repro/40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744 ================================================================= ==30399==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x633000052800 at pc 0x000001734048 bp 0x7ffd3598d090 sp 0x7ffd3598d088 READ of size 4 at 0x633000052800 thread T0 #0 0x1734047 in get_bits libavcodec/get_bits.h:404:5 #1 0x1734047 in frame_header_is_valid libavcodec/flac_parser.c:118 #2 0x173ac77 in find_headers_search_validate libavcodec/flac_parser.c:202:9 #3 0x173a851 in find_headers_search libavcodec/flac_parser.c:248:31 #4 0x172f29b in find_new_headers libavcodec/flac_parser.c:268:18 #5 0x172f29b in flac_parse libavcodec/flac_parser.c:666 #6 0x13fb138 in av_parser_parse2 libavcodec/parser.c:165:13 #7 0x6f1dba in parse_packet libavformat/demux.c:1127:15 #8 0x6befe7 in read_frame_internal libavformat/demux.c:1322:24 #9 0x6bafca in av_read_frame libavformat/demux.c:1426:17 #10 0x4c7832 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:197:15 #11 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 #12 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 #13 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 #14 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 #15 0x7f5d338b8bf6 in __libc_start_main /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 #16 0x41fb79 in _start (tools/target_dem_flac_fuzzer+0x41fb79) 0x633000052800 is located 0 bytes to the right of 106496-byte region [0x633000038800,0x633000052800) allocated by thread T0 here: #0 0x498597 in posix_memalign /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3 #1 0x26112c8 in av_malloc libavutil/mem.c:104:9 #2 0x25a67b3 in av_fifo_alloc_array libavutil/fifo.c:51:20 #3 0x172c9b6 in flac_parse_init libavcodec/flac_parser.c:747:21 #4 0x13f7d67 in av_parser_init libavcodec/parser.c:67:15 #5 0x6ce0b8 in avformat_find_stream_info libavformat/demux.c:2453:27 #6 0x4c77d0 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:192:11 #7 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 #8 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 #9 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 #10 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 #11 0x7f5d338b8bf6 in __libc_start_main /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 SUMMARY: AddressSanitizer: heap-buffer-overflow libavcodec/get_bits.h:404:5 in get_bits Shadow bytes around the buggy address: 0x0c66800024b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c66800024c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c66800024d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c66800024e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c66800024f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c6680002500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c6680002510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c6680002520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c6680002530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c6680002540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c6680002550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==30399==ABORTING [...]
On Wed, Oct 20, 2021 at 11:07 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Oct 13, 2021 at 06:15:26PM +0200, Mattias Wadman wrote: > > Reduces the risk of finding false frames that happens to have valid > values and CRC. > > > > Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame > > https://trac.ffmpeg.org/ticket/9185 > > --- > > libavcodec/flac_parser.c | 30 ++++++++++++++++++++++++++++-- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > After this was applied out of array accesses appeared > > tools/target_dem_flac_fuzzer: Running 1 inputs 1 time(s) each. > Running: > libfuzz-repro/40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744 > ================================================================= > ==30399==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x633000052800 at pc 0x000001734048 bp 0x7ffd3598d090 sp 0x7ffd3598d088 > READ of size 4 at 0x633000052800 thread T0 > #0 0x1734047 in get_bits libavcodec/get_bits.h:404:5 > #1 0x1734047 in frame_header_is_valid libavcodec/flac_parser.c:118 > #2 0x173ac77 in find_headers_search_validate > libavcodec/flac_parser.c:202:9 > #3 0x173a851 in find_headers_search libavcodec/flac_parser.c:248:31 > #4 0x172f29b in find_new_headers libavcodec/flac_parser.c:268:18 > #5 0x172f29b in flac_parse libavcodec/flac_parser.c:666 > #6 0x13fb138 in av_parser_parse2 libavcodec/parser.c:165:13 > #7 0x6f1dba in parse_packet libavformat/demux.c:1127:15 > #8 0x6befe7 in read_frame_internal libavformat/demux.c:1322:24 > #9 0x6bafca in av_read_frame libavformat/demux.c:1426:17 > #10 0x4c7832 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:197:15 > #11 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > #12 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > #13 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > #14 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > #15 0x7f5d338b8bf6 in __libc_start_main > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > #16 0x41fb79 in _start (tools/target_dem_flac_fuzzer+0x41fb79) > > 0x633000052800 is located 0 bytes to the right of 106496-byte region > [0x633000038800,0x633000052800) > allocated by thread T0 here: > #0 0x498597 in posix_memalign > /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3 > #1 0x26112c8 in av_malloc libavutil/mem.c:104:9 > #2 0x25a67b3 in av_fifo_alloc_array libavutil/fifo.c:51:20 > #3 0x172c9b6 in flac_parse_init libavcodec/flac_parser.c:747:21 > #4 0x13f7d67 in av_parser_init libavcodec/parser.c:67:15 > #5 0x6ce0b8 in avformat_find_stream_info libavformat/demux.c:2453:27 > #6 0x4c77d0 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:192:11 > #7 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > #8 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > #9 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > #10 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > #11 0x7f5d338b8bf6 in __libc_start_main > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > > SUMMARY: AddressSanitizer: heap-buffer-overflow > libavcodec/get_bits.h:404:5 in get_bits > Shadow bytes around the buggy address: > 0x0c66800024b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c66800024f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > =>0x0c6680002500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c6680002550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Partially addressable: 01 02 03 04 05 06 07 > Heap left redzone: fa > Freed heap region: fd > Stack left redzone: f1 > Stack mid redzone: f2 > Stack right redzone: f3 > Stack after return: f5 > Stack use after scope: f8 > Global redzone: f9 > Global init order: f6 > Poisoned by user: f7 > Container overflow: fc > Array cookie: ac > Intra object redzone: bb > ASan internal: fe > Left alloca redzone: ca > Right alloca redzone: cb > Shadow gap: cc > ==30399==ABORTING > > I think the issue is that currently some or all callers of frame_header_is_valid only guarantee that there are MAX_FRAME_HEADER_SIZE bytes. Could a solution be to add a buf_len argument that it passes to init_get_bits? and then conditionally do the subframe validation if there are bytes left after ff_flac_decode_frame_header is called? -Mattias
On Thu, Oct 21, 2021 at 10:46:18AM +0200, Mattias Wadman wrote: > On Wed, Oct 20, 2021 at 11:07 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Wed, Oct 13, 2021 at 06:15:26PM +0200, Mattias Wadman wrote: > > > Reduces the risk of finding false frames that happens to have valid > > values and CRC. > > > > > > Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame > > > https://trac.ffmpeg.org/ticket/9185 > > > --- > > > libavcodec/flac_parser.c | 30 ++++++++++++++++++++++++++++-- > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > After this was applied out of array accesses appeared > > > > tools/target_dem_flac_fuzzer: Running 1 inputs 1 time(s) each. > > Running: > > libfuzz-repro/40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744 > > ================================================================= > > ==30399==ERROR: AddressSanitizer: heap-buffer-overflow on address > > 0x633000052800 at pc 0x000001734048 bp 0x7ffd3598d090 sp 0x7ffd3598d088 > > READ of size 4 at 0x633000052800 thread T0 > > #0 0x1734047 in get_bits libavcodec/get_bits.h:404:5 > > #1 0x1734047 in frame_header_is_valid libavcodec/flac_parser.c:118 > > #2 0x173ac77 in find_headers_search_validate > > libavcodec/flac_parser.c:202:9 > > #3 0x173a851 in find_headers_search libavcodec/flac_parser.c:248:31 > > #4 0x172f29b in find_new_headers libavcodec/flac_parser.c:268:18 > > #5 0x172f29b in flac_parse libavcodec/flac_parser.c:666 > > #6 0x13fb138 in av_parser_parse2 libavcodec/parser.c:165:13 > > #7 0x6f1dba in parse_packet libavformat/demux.c:1127:15 > > #8 0x6befe7 in read_frame_internal libavformat/demux.c:1322:24 > > #9 0x6bafca in av_read_frame libavformat/demux.c:1426:17 > > #10 0x4c7832 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:197:15 > > #11 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > > #12 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > > #13 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > > #14 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > > #15 0x7f5d338b8bf6 in __libc_start_main > > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > > #16 0x41fb79 in _start (tools/target_dem_flac_fuzzer+0x41fb79) > > > > 0x633000052800 is located 0 bytes to the right of 106496-byte region > > [0x633000038800,0x633000052800) > > allocated by thread T0 here: > > #0 0x498597 in posix_memalign > > /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3 > > #1 0x26112c8 in av_malloc libavutil/mem.c:104:9 > > #2 0x25a67b3 in av_fifo_alloc_array libavutil/fifo.c:51:20 > > #3 0x172c9b6 in flac_parse_init libavcodec/flac_parser.c:747:21 > > #4 0x13f7d67 in av_parser_init libavcodec/parser.c:67:15 > > #5 0x6ce0b8 in avformat_find_stream_info libavformat/demux.c:2453:27 > > #6 0x4c77d0 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:192:11 > > #7 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > > #8 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > > #9 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > > #10 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > > #11 0x7f5d338b8bf6 in __libc_start_main > > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > > > > SUMMARY: AddressSanitizer: heap-buffer-overflow > > libavcodec/get_bits.h:404:5 in get_bits > > Shadow bytes around the buggy address: > > 0x0c66800024b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > 0x0c66800024c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > 0x0c66800024d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > 0x0c66800024e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > 0x0c66800024f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > =>0x0c6680002500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > 0x0c6680002510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > 0x0c6680002520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > 0x0c6680002530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > 0x0c6680002540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > 0x0c6680002550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > Shadow byte legend (one shadow byte represents 8 application bytes): > > Addressable: 00 > > Partially addressable: 01 02 03 04 05 06 07 > > Heap left redzone: fa > > Freed heap region: fd > > Stack left redzone: f1 > > Stack mid redzone: f2 > > Stack right redzone: f3 > > Stack after return: f5 > > Stack use after scope: f8 > > Global redzone: f9 > > Global init order: f6 > > Poisoned by user: f7 > > Container overflow: fc > > Array cookie: ac > > Intra object redzone: bb > > ASan internal: fe > > Left alloca redzone: ca > > Right alloca redzone: cb > > Shadow gap: cc > > ==30399==ABORTING > > > > > I think the issue is that currently some or all callers of > frame_header_is_valid only guarantee that there are MAX_FRAME_HEADER_SIZE > bytes. Could a solution be to add a buf_len argument that it passes > to init_get_bits? and then conditionally do the subframe validation if > there are bytes left after ff_flac_decode_frame_header is called? I havnt checked how the code previously ensured that there was enough data. But introducing a new way to ensure thatm, feels wrong unless there is something wrong with the existing way or theres no existing way and it worked just by luck. Is that the case ? thx [...]
On Thu, Oct 21, 2021 at 01:00:20PM +0200, Michael Niedermayer wrote: > On Thu, Oct 21, 2021 at 10:46:18AM +0200, Mattias Wadman wrote: > > On Wed, Oct 20, 2021 at 11:07 PM Michael Niedermayer <michael@niedermayer.cc> > > wrote: > > > > > On Wed, Oct 13, 2021 at 06:15:26PM +0200, Mattias Wadman wrote: > > > > Reduces the risk of finding false frames that happens to have valid > > > values and CRC. > > > > > > > > Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame > > > > https://trac.ffmpeg.org/ticket/9185 > > > > --- > > > > libavcodec/flac_parser.c | 30 ++++++++++++++++++++++++++++-- > > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > > > After this was applied out of array accesses appeared > > > > > > tools/target_dem_flac_fuzzer: Running 1 inputs 1 time(s) each. > > > Running: > > > libfuzz-repro/40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744 > > > ================================================================= > > > ==30399==ERROR: AddressSanitizer: heap-buffer-overflow on address > > > 0x633000052800 at pc 0x000001734048 bp 0x7ffd3598d090 sp 0x7ffd3598d088 > > > READ of size 4 at 0x633000052800 thread T0 > > > #0 0x1734047 in get_bits libavcodec/get_bits.h:404:5 > > > #1 0x1734047 in frame_header_is_valid libavcodec/flac_parser.c:118 > > > #2 0x173ac77 in find_headers_search_validate > > > libavcodec/flac_parser.c:202:9 > > > #3 0x173a851 in find_headers_search libavcodec/flac_parser.c:248:31 > > > #4 0x172f29b in find_new_headers libavcodec/flac_parser.c:268:18 > > > #5 0x172f29b in flac_parse libavcodec/flac_parser.c:666 > > > #6 0x13fb138 in av_parser_parse2 libavcodec/parser.c:165:13 > > > #7 0x6f1dba in parse_packet libavformat/demux.c:1127:15 > > > #8 0x6befe7 in read_frame_internal libavformat/demux.c:1322:24 > > > #9 0x6bafca in av_read_frame libavformat/demux.c:1426:17 > > > #10 0x4c7832 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:197:15 > > > #11 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > > > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > > > #12 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > > > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > > > #13 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > > > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > > > #14 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > > > #15 0x7f5d338b8bf6 in __libc_start_main > > > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > > > #16 0x41fb79 in _start (tools/target_dem_flac_fuzzer+0x41fb79) > > > > > > 0x633000052800 is located 0 bytes to the right of 106496-byte region > > > [0x633000038800,0x633000052800) > > > allocated by thread T0 here: > > > #0 0x498597 in posix_memalign > > > /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3 > > > #1 0x26112c8 in av_malloc libavutil/mem.c:104:9 > > > #2 0x25a67b3 in av_fifo_alloc_array libavutil/fifo.c:51:20 > > > #3 0x172c9b6 in flac_parse_init libavcodec/flac_parser.c:747:21 > > > #4 0x13f7d67 in av_parser_init libavcodec/parser.c:67:15 > > > #5 0x6ce0b8 in avformat_find_stream_info libavformat/demux.c:2453:27 > > > #6 0x4c77d0 in LLVMFuzzerTestOneInput tools/target_dem_fuzzer.c:192:11 > > > #7 0x27055bd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, > > > unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 > > > #8 0x26fa192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > > > unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 > > > #9 0x26ff391 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned > > > char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 > > > #10 0x26f9e70 in main Fuzzer/build/../FuzzerMain.cpp:20:10 > > > #11 0x7f5d338b8bf6 in __libc_start_main > > > /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310 > > > > > > SUMMARY: AddressSanitizer: heap-buffer-overflow > > > libavcodec/get_bits.h:404:5 in get_bits > > > Shadow bytes around the buggy address: > > > 0x0c66800024b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > 0x0c66800024c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > 0x0c66800024d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > 0x0c66800024e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > 0x0c66800024f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > =>0x0c6680002500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > > 0x0c6680002510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > > 0x0c6680002520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > > 0x0c6680002530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > > 0x0c6680002540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > > 0x0c6680002550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > > > Shadow byte legend (one shadow byte represents 8 application bytes): > > > Addressable: 00 > > > Partially addressable: 01 02 03 04 05 06 07 > > > Heap left redzone: fa > > > Freed heap region: fd > > > Stack left redzone: f1 > > > Stack mid redzone: f2 > > > Stack right redzone: f3 > > > Stack after return: f5 > > > Stack use after scope: f8 > > > Global redzone: f9 > > > Global init order: f6 > > > Poisoned by user: f7 > > > Container overflow: fc > > > Array cookie: ac > > > Intra object redzone: bb > > > ASan internal: fe > > > Left alloca redzone: ca > > > Right alloca redzone: cb > > > Shadow gap: cc > > > ==30399==ABORTING > > > > > > > > I think the issue is that currently some or all callers of > > frame_header_is_valid only guarantee that there are MAX_FRAME_HEADER_SIZE > > bytes. Could a solution be to add a buf_len argument that it passes > > to init_get_bits? and then conditionally do the subframe validation if > > there are bytes left after ff_flac_decode_frame_header is called? > > I havnt checked how the code previously ensured that there was enough > data. But introducing a new way to ensure thatm, feels wrong unless there > is something wrong with the existing way or theres no existing way and it > worked just by luck. > Is that the case ? Also just passing the buffer size will not solve this the amount allocated needs to consider AV_INPUT_BUFFER_PADDING_SIZE [...]
diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c index d3d9c889a1..2c550507fc 100644 --- a/libavcodec/flac_parser.c +++ b/libavcodec/flac_parser.c @@ -96,8 +96,34 @@ static int frame_header_is_valid(AVCodecContext *avctx, const uint8_t *buf, FLACFrameInfo *fi) { GetBitContext gb; - init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8); - return !ff_flac_decode_frame_header(avctx, &gb, fi, 127); + uint8_t subframe_type; + + // header plus one byte from first subframe + init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8 + 8); + if (ff_flac_decode_frame_header(avctx, &gb, fi, 127)) { + return 0; + } + // subframe zero bit + if (get_bits1(&gb) != 0) { + return 0; + } + // subframe type + // 000000 : SUBFRAME_CONSTANT + // 000001 : SUBFRAME_VERBATIM + // 00001x : reserved + // 0001xx : reserved + // 001xxx : if(xxx <= 4) SUBFRAME_FIXED, xxx=order ; else reserved + // 01xxxx : reserved + // 1xxxxx : SUBFRAME_LPC, xxxxx=order-1 + subframe_type = get_bits(&gb, 6); + if (!(subframe_type == 0 || + subframe_type == 1 || + ((subframe_type >= 8) && (subframe_type <= 12)) || + (subframe_type >= 32))) { + return 0; + } + + return 1; } /**