| Message ID | 20210123221056.3366-6-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avutil/common: Add FFABSU() for a signed -> unsigned ABS | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Quoting Michael Niedermayer (2021-01-23 23:10:56) > Fixes: Timeout > Fixes: 29656/clusterfuzz-testcase-minimized-ffmpeg_dem_FLV_fuzzer-5840098987999232 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/flvdec.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > index 4bc5e15dd2..02797e1fba 100644 > --- a/libavformat/flvdec.c > +++ b/libavformat/flvdec.c > @@ -909,6 +909,11 @@ static int flv_data_packet(AVFormatContext *s, AVPacket *pkt, > > while (array || (ret = amf_get_string(pb, buf, sizeof(buf))) > 0) { > AMFDataType type = avio_r8(pb); > + if (avio_feof(pb)) { > + ret = AVERROR_INVALIDDATA; > + goto skip; > + } > + > if (type == AMF_DATA_TYPE_STRING && (array || !strcmp(buf, "text"))) { > length = avio_rb16(pb); > ret = av_get_packet(pb, pkt, length); > -- > 2.17.1 IMO it would be cleaner to make amf_get_string() check the return value of avio_read() and return an error on short reads.
On Sun, Jan 24, 2021 at 02:17:05PM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2021-01-23 23:10:56) > > Fixes: Timeout > > Fixes: 29656/clusterfuzz-testcase-minimized-ffmpeg_dem_FLV_fuzzer-5840098987999232 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/flvdec.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > > index 4bc5e15dd2..02797e1fba 100644 > > --- a/libavformat/flvdec.c > > +++ b/libavformat/flvdec.c > > @@ -909,6 +909,11 @@ static int flv_data_packet(AVFormatContext *s, AVPacket *pkt, > > > > while (array || (ret = amf_get_string(pb, buf, sizeof(buf))) > 0) { > > AMFDataType type = avio_r8(pb); > > + if (avio_feof(pb)) { > > + ret = AVERROR_INVALIDDATA; > > + goto skip; > > + } > > + > > if (type == AMF_DATA_TYPE_STRING && (array || !strcmp(buf, "text"))) { > > length = avio_rb16(pb); > > ret = av_get_packet(pb, pkt, length); > > -- > > 2.17.1 > > IMO it would be cleaner to make amf_get_string() check the return value > of avio_read() and return an error on short reads. will do but that does not fix the issue as array is set and amf_get_string() does not run in that case. thx [...]
Quoting Michael Niedermayer (2021-01-26 17:42:27) > On Sun, Jan 24, 2021 at 02:17:05PM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2021-01-23 23:10:56) > > > Fixes: Timeout > > > Fixes: 29656/clusterfuzz-testcase-minimized-ffmpeg_dem_FLV_fuzzer-5840098987999232 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/flvdec.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > > > index 4bc5e15dd2..02797e1fba 100644 > > > --- a/libavformat/flvdec.c > > > +++ b/libavformat/flvdec.c > > > @@ -909,6 +909,11 @@ static int flv_data_packet(AVFormatContext *s, AVPacket *pkt, > > > > > > while (array || (ret = amf_get_string(pb, buf, sizeof(buf))) > 0) { > > > AMFDataType type = avio_r8(pb); > > > + if (avio_feof(pb)) { > > > + ret = AVERROR_INVALIDDATA; > > > + goto skip; > > > + } > > > + > > > if (type == AMF_DATA_TYPE_STRING && (array || !strcmp(buf, "text"))) { > > > length = avio_rb16(pb); > > > ret = av_get_packet(pb, pkt, length); > > > -- > > > 2.17.1 > > > > IMO it would be cleaner to make amf_get_string() check the return value > > of avio_read() and return an error on short reads. > > will do but that does not fix the issue as array is set and > amf_get_string() does not run in that case. That makes me wonder whether it would not be better to make av_get_packet() return an error on short reads. Is there any use case in lavf where we would want to return a partial packet?
diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c index 4bc5e15dd2..02797e1fba 100644 --- a/libavformat/flvdec.c +++ b/libavformat/flvdec.c @@ -909,6 +909,11 @@ static int flv_data_packet(AVFormatContext *s, AVPacket *pkt, while (array || (ret = amf_get_string(pb, buf, sizeof(buf))) > 0) { AMFDataType type = avio_r8(pb); + if (avio_feof(pb)) { + ret = AVERROR_INVALIDDATA; + goto skip; + } + if (type == AMF_DATA_TYPE_STRING && (array || !strcmp(buf, "text"))) { length = avio_rb16(pb); ret = av_get_packet(pb, pkt, length);
Fixes: Timeout Fixes: 29656/clusterfuzz-testcase-minimized-ffmpeg_dem_FLV_fuzzer-5840098987999232 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/flvdec.c | 5 +++++ 1 file changed, 5 insertions(+)