[FFmpeg-devel,03/11] avformat/mxfdec: Check type before setting sub_descriptors_refs

Fixes: memleak
Fixes: 26352/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5201158714687488

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

Michael Niedermayer:
> Fixes: memleak
> Fixes: 26352/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5201158714687488
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index d16a7af0df..0313cf4a28 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1172,6 +1172,8 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
>  
>      switch(tag) {
>      case 0x3F01:
> +        if (descriptor->type != MultipleDescriptor)
> +            return AVERROR_INVALIDDATA;
>          return mxf_read_strong_ref_array(pb, &descriptor->sub_descriptors_refs,
>                                               &descriptor->sub_descriptors_count);
>      case 0x3002: /* ContainerDuration */
> 
I think this memleak has a twin: I don't see anything that makes sure
that a descriptor of type MultipleDescriptor can't take the default case
in that switch, leading to allocations that are never freed. All it
needs is the local_tags list. Presuming this to be true, one would
either add a corresponding check to the default case of the switch or
one would have to modify mxf_free_metadataset() or one would have to use
a dedicated function to read the MultipleDescriptors. The answer of
course depends upon the specification (it seems that the only thing used
from MultipleDescriptors are indeed the sub descriptors, so if the other
case labels are spec-incompliant anyway, then this would be my favourite
solution).

- Andreas

tis 2020-10-20 klockan 22:56 +0200 skrev Michael Niedermayer:
> Fixes: memleak
> Fixes: 26352/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5201158714687488
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index d16a7af0df..0313cf4a28 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1172,6 +1172,8 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
>  
>      switch(tag) {
>      case 0x3F01:
> +        if (descriptor->type != MultipleDescriptor)
> +            return AVERROR_INVALIDDATA;
>          return mxf_read_strong_ref_array(pb, &descriptor->sub_descriptors_refs,
>                                               &descriptor->sub_descriptors_count);
>      case 0x3002: /* ContainerDuration */

mxf_read_strong_ref_array() is called in more places than this. A
better solution would be to make freeing sub_descriptors_refs in
mxf_free_metadataset() not depends on type

/Tomas

Tomas Härdin:
> tis 2020-10-20 klockan 22:56 +0200 skrev Michael Niedermayer:
>> Fixes: memleak
>> Fixes: 26352/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5201158714687488
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavformat/mxfdec.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index d16a7af0df..0313cf4a28 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -1172,6 +1172,8 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
>>  
>>      switch(tag) {
>>      case 0x3F01:
>> +        if (descriptor->type != MultipleDescriptor)
>> +            return AVERROR_INVALIDDATA;
>>          return mxf_read_strong_ref_array(pb, &descriptor->sub_descriptors_refs,
>>                                               &descriptor->sub_descriptors_count);
>>      case 0x3002: /* ContainerDuration */
> 
> mxf_read_strong_ref_array() is called in more places than this. A
> better solution would be to make freeing sub_descriptors_refs in
> mxf_free_metadataset() not depends on type
> 
But this is the only place that sets the sub_descriptors_ref.
Do the other tags that lead to certain properties being read (like
frame_layouts) actually make sense for a MultipleDescriptor? If not, why
do Descriptor and MultipleDescriptor share the same parsing function?

- Andreas

ons 2020-10-21 klockan 16:48 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2020-10-20 klockan 22:56 +0200 skrev Michael Niedermayer:
> > > Fixes: memleak
> > > Fixes: 26352/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5201158714687488
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/mxfdec.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index d16a7af0df..0313cf4a28 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -1172,6 +1172,8 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
> > >  
> > >      switch(tag) {
> > >      case 0x3F01:
> > > +        if (descriptor->type != MultipleDescriptor)
> > > +            return AVERROR_INVALIDDATA;
> > >          return mxf_read_strong_ref_array(pb, &descriptor->sub_descriptors_refs,
> > >                                               &descriptor->sub_descriptors_count);
> > >      case 0x3002: /* ContainerDuration */
> > 
> > mxf_read_strong_ref_array() is called in more places than this. A
> > better solution would be to make freeing sub_descriptors_refs in
> > mxf_free_metadataset() not depends on type
> > 
> But this is the only place that sets the sub_descriptors_ref.
> Do the other tags that lead to certain properties being read (like
> frame_layouts) actually make sense for a MultipleDescriptor? If not, why
> do Descriptor and MultipleDescriptor share the same parsing function?

MultipleDescriptor (Annex D.5) can contain any FileDescriptor field
(Annex D.1), for example Codec (0x3005). At the moment none of those
fields are used, instead source_package->descriptor gets resolved by
mxf_resolve_multidescriptor() and the resolved descriptor's fields are
used instead.

The 0x3F01 case could be broken out into a separate function that calls
mxf_read_generic_descriptor() in the default case.

/Tomas