diff mbox series

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

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
Related show

Checks

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

Commit Message

Mattias Wadman Oct. 13, 2021, 4:15 p.m. UTC
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(-)

Comments

Mattias Wadman Oct. 18, 2021, 3:07 p.m. UTC | #1
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
Paul B Mahol Oct. 18, 2021, 3:16 p.m. UTC | #2
LGTM, will apply soon
Mattias Wadman Oct. 18, 2021, 10:30 p.m. UTC | #3
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".
>
Michael Niedermayer Oct. 20, 2021, 9:07 p.m. UTC | #4
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

[...]
Mattias Wadman Oct. 21, 2021, 8:46 a.m. UTC | #5
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
Michael Niedermayer Oct. 21, 2021, 11 a.m. UTC | #6
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

[...]
Michael Niedermayer Oct. 21, 2021, 11:16 a.m. UTC | #7
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 mbox series

Patch

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;
 }
 
 /**