diff mbox series

[FFmpeg-devel,2/4] avformat/concatdec: Check filename length before use

Message ID 20201028225643.30485-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/4] avformat/apngdec: Check for incomplete reads in append_extradata()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Oct. 28, 2020, 10:56 p.m. UTC
Fixes: out array read
Fixes: 26610/clusterfuzz-testcase-minimized-ffmpeg_dem_CONCAT_fuzzer-5631838049271808

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/concatdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marton Balint Oct. 28, 2020, 11:40 p.m. UTC | #1
On Wed, 28 Oct 2020, Michael Niedermayer wrote:

> Fixes: out array read
> Fixes: 26610/clusterfuzz-testcase-minimized-ffmpeg_dem_CONCAT_fuzzer-5631838049271808
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/concatdec.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> index 4b56b61404..0734bc44e1 100644
> --- a/libavformat/concatdec.c
> +++ b/libavformat/concatdec.c
> @@ -123,7 +123,8 @@ static int add_file(AVFormatContext *avf, char *filename, ConcatFile **rfile,
>
>     proto = avio_find_protocol_name(filename);
>     proto_len = proto ? strlen(proto) : 0;
> -    if (proto && !memcmp(filename, proto, proto_len) &&
> +    if (proto && strlen(filename) >= proto_len &&
> +        !memcmp(filename, proto, proto_len) &&

Why not strncmp?

Thanks,
Marton

>         (filename[proto_len] == ':' || filename[proto_len] == ',')) {
>         url = filename;
>         filename = NULL;
> -- 
> 2.17.1
>
> _______________________________________________
> 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".
Nicolas George Oct. 29, 2020, 3:09 p.m. UTC | #2
Michael Niedermayer (12020-10-28):
> Fixes: out array read
> Fixes: 26610/clusterfuzz-testcase-minimized-ffmpeg_dem_CONCAT_fuzzer-5631838049271808
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/concatdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This is not the first time such a fix is proposed. If strcmp() does not
behave intelligently, then we need an extra function for the task, but
duplicating the test all over the place is not a good idea.

Regards,
Andreas Rheinhardt Oct. 29, 2020, 3:22 p.m. UTC | #3
Nicolas George:
> Michael Niedermayer (12020-10-28):
>> Fixes: out array read
>> Fixes: 26610/clusterfuzz-testcase-minimized-ffmpeg_dem_CONCAT_fuzzer-5631838049271808
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavformat/concatdec.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> This is not the first time such a fix is proposed. If strcmp() does not
> behave intelligently, then we need an extra function for the task, but
> duplicating the test all over the place is not a good idea.
> 
We already have av_strstart() which would also allow us to avoid
computing strlen(proto) at all.

- Andreas
Michael Niedermayer Oct. 31, 2020, 11:08 p.m. UTC | #4
On Thu, Oct 29, 2020 at 04:22:23PM +0100, Andreas Rheinhardt wrote:
> Nicolas George:
> > Michael Niedermayer (12020-10-28):
> >> Fixes: out array read
> >> Fixes: 26610/clusterfuzz-testcase-minimized-ffmpeg_dem_CONCAT_fuzzer-5631838049271808
> >>
> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> ---
> >>  libavformat/concatdec.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > This is not the first time such a fix is proposed. If strcmp() does not
> > behave intelligently, then we need an extra function for the task, but
> > duplicating the test all over the place is not a good idea.
> > 
> We already have av_strstart() which would also allow us to avoid
> computing strlen(proto) at all.

patch with av_strstart() posted, av_strstart() was available since
a long time so it should have no negative impact on backporting

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 4b56b61404..0734bc44e1 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -123,7 +123,8 @@  static int add_file(AVFormatContext *avf, char *filename, ConcatFile **rfile,
 
     proto = avio_find_protocol_name(filename);
     proto_len = proto ? strlen(proto) : 0;
-    if (proto && !memcmp(filename, proto, proto_len) &&
+    if (proto && strlen(filename) >= proto_len &&
+        !memcmp(filename, proto, proto_len) &&
         (filename[proto_len] == ':' || filename[proto_len] == ',')) {
         url = filename;
         filename = NULL;