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 > > <mich...@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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
