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