diff mbox series

[FFmpeg-devel] avcodec/flac_parser: Consider AV_INPUT_BUFFER_PADDING_SIZE

Message ID 20211021120001.14095-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avcodec/flac_parser: Consider AV_INPUT_BUFFER_PADDING_SIZE
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

Michael Niedermayer Oct. 21, 2021, noon UTC
Fixes: out if array read
Fixes: 40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
CC: Mattias Wadman <mattias.wadman@gmail.com>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/flac_parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Oct. 21, 2021, 12:11 p.m. UTC | #1
On Thu, Oct 21, 2021 at 02:00:01PM +0200, Michael Niedermayer wrote:
> Fixes: out if array read
> Fixes: 40109/clusterfuzz-testcase-minimized-ffmpeg_dem_FLAC_fuzzer-4805686811295744
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> CC: Mattias Wadman <mattias.wadman@gmail.com>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/flac_parser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Note, iam not sure this is enough, but it fixes this specific sample

a more complete audit/review of the flac parser is maybe not a bad idea
if someone has time

thx

[...]
Paul B Mahol Oct. 21, 2021, 8:17 p.m. UTC | #2
LGTM for now
Michael Niedermayer Oct. 21, 2021, 8:35 p.m. UTC | #3
On Thu, Oct 21, 2021 at 10:17:25PM +0200, Paul B Mahol wrote:
> LGTM for now

will apply the improved variant below

diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
index 2c550507fc8..3b27b152fc5 100644
--- a/libavcodec/flac_parser.c
+++ b/libavcodec/flac_parser.c
@@ -55,6 +55,7 @@
 
 /** largest possible size of flac header */
 #define MAX_FRAME_HEADER_SIZE 16
+#define MAX_FRAME_VERIFY_SIZE (MAX_FRAME_HEADER_SIZE + 1)
 
 typedef struct FLACHeaderMarker {
     int offset;       /**< byte offset from start of FLACParseContext->buffer */
@@ -99,7 +100,7 @@ static int frame_header_is_valid(AVCodecContext *avctx, const uint8_t *buf,
     uint8_t subframe_type;
 
     // header plus one byte from first subframe
-    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
+    init_get_bits(&gb, buf, MAX_FRAME_VERIFY_SIZE * 8);
     if (ff_flac_decode_frame_header(avctx, &gb, fi, 127)) {
         return 0;
     }
@@ -196,7 +197,7 @@ static int find_headers_search_validate(FLACParseContext *fpc, int offset)
     uint8_t *header_buf;
     int size = 0;
     header_buf = flac_fifo_read_wrap(fpc, offset,
-                                     MAX_FRAME_HEADER_SIZE,
+                                     MAX_FRAME_VERIFY_SIZE + AV_INPUT_BUFFER_PADDING_SIZE,
                                      &fpc->wrap_buf,
                                      &fpc->wrap_buf_allocated_size);
     if (frame_header_is_valid(fpc->avctx, header_buf, &fi)) {


[...]
Mattias Wadman Oct. 21, 2021, 10 p.m. UTC | #4
On Thu, Oct 21, 2021 at 10:35 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Thu, Oct 21, 2021 at 10:17:25PM +0200, Paul B Mahol wrote:
> > LGTM for now
>
> will apply the improved variant below
>
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index 2c550507fc8..3b27b152fc5 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -55,6 +55,7 @@
>
>  /** largest possible size of flac header */
>  #define MAX_FRAME_HEADER_SIZE 16
> +#define MAX_FRAME_VERIFY_SIZE (MAX_FRAME_HEADER_SIZE + 1)
>
>  typedef struct FLACHeaderMarker {
>      int offset;       /**< byte offset from start of
> FLACParseContext->buffer */
> @@ -99,7 +100,7 @@ static int frame_header_is_valid(AVCodecContext *avctx,
> const uint8_t *buf,
>      uint8_t subframe_type;
>
>      // header plus one byte from first subframe
> -    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
> +    init_get_bits(&gb, buf, MAX_FRAME_VERIFY_SIZE * 8);
>      if (ff_flac_decode_frame_header(avctx, &gb, fi, 127)) {
>          return 0;
>      }
> @@ -196,7 +197,7 @@ static int
> find_headers_search_validate(FLACParseContext *fpc, int offset)
>      uint8_t *header_buf;
>      int size = 0;
>      header_buf = flac_fifo_read_wrap(fpc, offset,
> -                                     MAX_FRAME_HEADER_SIZE,
> +                                     MAX_FRAME_VERIFY_SIZE +
> AV_INPUT_BUFFER_PADDING_SIZE,
>                                       &fpc->wrap_buf,
>                                       &fpc->wrap_buf_allocated_size);
>      if (frame_header_is_valid(fpc->avctx, header_buf, &fi)) {
>
>
LGTM

But i'm not sure about the PARSER_FLAG_COMPLETE_FRAMES case, hard to tell
if those code paths will always have
MAX_FRAME_VERIFY_SIZE+AV_INPUT_BUFFER_PADDING_SIZE buf size.

Thanks for helping to fix this.

BTW, yesterday a FLAC file showed up with a "false" frame that even this
patch failed to ignore. Strange enough it is a FLAC file with no encoder
metadata at all and the frame that it failed on is a verbatim frame. It's a
perfectly valid file with correct md5 but the audio is heavily distorted
which explains the verbatim frames. Hopefully they should be very rare.
Michael Niedermayer Oct. 22, 2021, 8:42 p.m. UTC | #5
On Fri, Oct 22, 2021 at 12:00:19AM +0200, Mattias Wadman wrote:
> On Thu, Oct 21, 2021 at 10:35 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Thu, Oct 21, 2021 at 10:17:25PM +0200, Paul B Mahol wrote:
> > > LGTM for now
> >
> > will apply the improved variant below
> >
> > diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> > index 2c550507fc8..3b27b152fc5 100644
> > --- a/libavcodec/flac_parser.c
> > +++ b/libavcodec/flac_parser.c
> > @@ -55,6 +55,7 @@
> >
> >  /** largest possible size of flac header */
> >  #define MAX_FRAME_HEADER_SIZE 16
> > +#define MAX_FRAME_VERIFY_SIZE (MAX_FRAME_HEADER_SIZE + 1)
> >
> >  typedef struct FLACHeaderMarker {
> >      int offset;       /**< byte offset from start of
> > FLACParseContext->buffer */
> > @@ -99,7 +100,7 @@ static int frame_header_is_valid(AVCodecContext *avctx,
> > const uint8_t *buf,
> >      uint8_t subframe_type;
> >
> >      // header plus one byte from first subframe
> > -    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
> > +    init_get_bits(&gb, buf, MAX_FRAME_VERIFY_SIZE * 8);
> >      if (ff_flac_decode_frame_header(avctx, &gb, fi, 127)) {
> >          return 0;
> >      }
> > @@ -196,7 +197,7 @@ static int
> > find_headers_search_validate(FLACParseContext *fpc, int offset)
> >      uint8_t *header_buf;
> >      int size = 0;
> >      header_buf = flac_fifo_read_wrap(fpc, offset,
> > -                                     MAX_FRAME_HEADER_SIZE,
> > +                                     MAX_FRAME_VERIFY_SIZE +
> > AV_INPUT_BUFFER_PADDING_SIZE,
> >                                       &fpc->wrap_buf,
> >                                       &fpc->wrap_buf_allocated_size);
> >      if (frame_header_is_valid(fpc->avctx, header_buf, &fi)) {
> >
> >
> LGTM

will apply


> 
> But i'm not sure about the PARSER_FLAG_COMPLETE_FRAMES case, hard to tell
> if those code paths will always have
> MAX_FRAME_VERIFY_SIZE+AV_INPUT_BUFFER_PADDING_SIZE buf size.

this issue is still open, yes

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
index 2c550507fc8..61232eed829 100644
--- a/libavcodec/flac_parser.c
+++ b/libavcodec/flac_parser.c
@@ -196,7 +196,7 @@  static int find_headers_search_validate(FLACParseContext *fpc, int offset)
     uint8_t *header_buf;
     int size = 0;
     header_buf = flac_fifo_read_wrap(fpc, offset,
-                                     MAX_FRAME_HEADER_SIZE,
+                                     MAX_FRAME_HEADER_SIZE + AV_INPUT_BUFFER_PADDING_SIZE,
                                      &fpc->wrap_buf,
                                      &fpc->wrap_buf_allocated_size);
     if (frame_header_is_valid(fpc->avctx, header_buf, &fi)) {