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

Thu Oct 21 14:00:20 EEST 2021

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 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?

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211021/79a2bb00/attachment.sig>