| Message ID | 20220318174621.26974-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/vp9_superframe_split_bsf: Check in size | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Michael Niedermayer: > Fixes: Out of array read > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/vp9_superframe_split_bsf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > index ed0444561a..481484a4f0 100644 > --- a/libavcodec/vp9_superframe_split_bsf.c > +++ b/libavcodec/vp9_superframe_split_bsf.c > @@ -51,7 +51,7 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > return ret; > in = s->buffer_pkt; > > - marker = in->data[in->size - 1]; > + marker = in->size ? in->data[in->size - 1] : 0; > if ((marker & 0xe0) == 0xc0) { > int length_size = 1 + ((marker >> 3) & 0x3); > int nb_frames = 1 + (marker & 0x7); There is a second place in this BSF where data might be read in the absence of data, namely if one of the frames in a superframe have size of zero (its attempted to read its profile; no actual read takes place due to the checks of the get_bits API, but it is nevertheless invalid data). See https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-7-andreas.rheinhardt@gmail.com/; also see https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-11-andreas.rheinhardt@gmail.com/ and https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-1-andreas.rheinhardt@gmail.com/ - Andreas
On Fri, Mar 18, 2022 at 06:56:19PM +0100, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: Out of array read > > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/vp9_superframe_split_bsf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > > index ed0444561a..481484a4f0 100644 > > --- a/libavcodec/vp9_superframe_split_bsf.c > > +++ b/libavcodec/vp9_superframe_split_bsf.c > > @@ -51,7 +51,7 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > > return ret; > > in = s->buffer_pkt; > > > > - marker = in->data[in->size - 1]; > > + marker = in->size ? in->data[in->size - 1] : 0; > > if ((marker & 0xe0) == 0xc0) { > > int length_size = 1 + ((marker >> 3) & 0x3); > > int nb_frames = 1 + (marker & 0x7); > > There is a second place in this BSF where data might be read in the > absence of data, namely if one of the frames in a superframe have size > of zero (its attempted to read its profile; no actual read takes place > due to the checks of the get_bits API, but it is nevertheless invalid > data). See > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-7-andreas.rheinhardt@gmail.com/; > also see > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-11-andreas.rheinhardt@gmail.com/ > and > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-1-andreas.rheinhardt@gmail.com/ Why did you not apply your bugfixes ? I think you should apply them thx [...]
On Sat, Mar 19, 2022 at 06:38:08PM +0100, Michael Niedermayer wrote: > On Fri, Mar 18, 2022 at 06:56:19PM +0100, Andreas Rheinhardt wrote: > > Michael Niedermayer: > > > Fixes: Out of array read > > > Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/vp9_superframe_split_bsf.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > > > index ed0444561a..481484a4f0 100644 > > > --- a/libavcodec/vp9_superframe_split_bsf.c > > > +++ b/libavcodec/vp9_superframe_split_bsf.c > > > @@ -51,7 +51,7 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > > > return ret; > > > in = s->buffer_pkt; > > > > > > - marker = in->data[in->size - 1]; > > > + marker = in->size ? in->data[in->size - 1] : 0; > > > if ((marker & 0xe0) == 0xc0) { > > > int length_size = 1 + ((marker >> 3) & 0x3); > > > int nb_frames = 1 + (marker & 0x7); > > > > There is a second place in this BSF where data might be read in the > > absence of data, namely if one of the frames in a superframe have size > > of zero (its attempted to read its profile; no actual read takes place > > due to the checks of the get_bits API, but it is nevertheless invalid > > data). See > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-7-andreas.rheinhardt@gmail.com/; The get bits API checks for NULL data, if data is not NULL it must be padded even when size is 0. Nothing against the 2nd check, but thats a seperate issue > > also see > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-11-andreas.rheinhardt@gmail.com/ please apply your bugfixes! especially if its about out or array accesses > > and > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-1-andreas.rheinhardt@gmail.com/ thats now found by the fuzzer too, in 45722 if you dont apply your fix i will post a fix thx [...]
Michael Niedermayer: > On Sat, Mar 19, 2022 at 06:38:08PM +0100, Michael Niedermayer wrote: >> On Fri, Mar 18, 2022 at 06:56:19PM +0100, Andreas Rheinhardt wrote: >>> Michael Niedermayer: >>>> Fixes: Out of array read >>>> Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 >>>> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> --- >>>> libavcodec/vp9_superframe_split_bsf.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c >>>> index ed0444561a..481484a4f0 100644 >>>> --- a/libavcodec/vp9_superframe_split_bsf.c >>>> +++ b/libavcodec/vp9_superframe_split_bsf.c >>>> @@ -51,7 +51,7 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) >>>> return ret; >>>> in = s->buffer_pkt; >>>> >>>> - marker = in->data[in->size - 1]; >>>> + marker = in->size ? in->data[in->size - 1] : 0; >>>> if ((marker & 0xe0) == 0xc0) { >>>> int length_size = 1 + ((marker >> 3) & 0x3); >>>> int nb_frames = 1 + (marker & 0x7); >>> >>> There is a second place in this BSF where data might be read in the >>> absence of data, namely if one of the frames in a superframe have size >>> of zero (its attempted to read its profile; no actual read takes place >>> due to the checks of the get_bits API, but it is nevertheless invalid >>> data). See >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-7-andreas.rheinhardt@gmail.com/; > > The get bits API checks for NULL data, if data is not NULL it must be padded > even when size is 0. > Nothing against the 2nd check, but thats a seperate issue I know that there is no invalid read (and said as much) > > >>> also see >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-11-andreas.rheinhardt@gmail.com/ > > please apply your bugfixes! especially if its about out or array accesses > Will do. > >>> and >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-1-andreas.rheinhardt@gmail.com/ > > thats now found by the fuzzer too, in 45722 > if you dont apply your fix i will post a fix > > thx > > [...] > >
On Mon, Mar 21, 2022 at 09:59:00PM +0100, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Sat, Mar 19, 2022 at 06:38:08PM +0100, Michael Niedermayer wrote: > >> On Fri, Mar 18, 2022 at 06:56:19PM +0100, Andreas Rheinhardt wrote: > >>> Michael Niedermayer: > >>>> Fixes: Out of array read > >>>> Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 > >>>> > >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>> --- > >>>> libavcodec/vp9_superframe_split_bsf.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c > >>>> index ed0444561a..481484a4f0 100644 > >>>> --- a/libavcodec/vp9_superframe_split_bsf.c > >>>> +++ b/libavcodec/vp9_superframe_split_bsf.c > >>>> @@ -51,7 +51,7 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) > >>>> return ret; > >>>> in = s->buffer_pkt; > >>>> > >>>> - marker = in->data[in->size - 1]; > >>>> + marker = in->size ? in->data[in->size - 1] : 0; > >>>> if ((marker & 0xe0) == 0xc0) { > >>>> int length_size = 1 + ((marker >> 3) & 0x3); > >>>> int nb_frames = 1 + (marker & 0x7); > >>> > >>> There is a second place in this BSF where data might be read in the > >>> absence of data, namely if one of the frames in a superframe have size > >>> of zero (its attempted to read its profile; no actual read takes place > >>> due to the checks of the get_bits API, but it is nevertheless invalid > >>> data). See > >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-7-andreas.rheinhardt@gmail.com/; > > > > The get bits API checks for NULL data, if data is not NULL it must be padded > > even when size is 0. > > Nothing against the 2nd check, but thats a seperate issue > > I know that there is no invalid read (and said as much) I read what you wrote to quick, i missed this > > > > > > >>> also see > >>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200530160541.29517-11-andreas.rheinhardt@gmail.com/ > > > > please apply your bugfixes! especially if its about out or array accesses > > > > Will do. thx [...]
diff --git a/libavcodec/vp9_superframe_split_bsf.c b/libavcodec/vp9_superframe_split_bsf.c index ed0444561a..481484a4f0 100644 --- a/libavcodec/vp9_superframe_split_bsf.c +++ b/libavcodec/vp9_superframe_split_bsf.c @@ -51,7 +51,7 @@ static int vp9_superframe_split_filter(AVBSFContext *ctx, AVPacket *out) return ret; in = s->buffer_pkt; - marker = in->data[in->size - 1]; + marker = in->size ? in->data[in->size - 1] : 0; if ((marker & 0xe0) == 0xc0) { int length_size = 1 + ((marker >> 3) & 0x3); int nb_frames = 1 + (marker & 0x7);
Fixes: Out of array read Fixes: 45137/clusterfuzz-testcase-minimized-ffmpeg_BSF_VP9_SUPERFRAME_SPLIT_fuzzer-4984270639202304 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vp9_superframe_split_bsf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)