[FFmpeg-devel] [PATCH v1] libavcodec/flac_parser: Validate subframe zero bit and type

[Mattias Wadman]

On Wed, Oct 20, 2021 at 11:07 PM Michael Niedermayer <michael at 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