[FFmpeg-devel] [PATCH] avfilter/formats: Fix heap-buffer overflow when merging channel layouts

[Nicolas George]

Andreas Rheinhardt (12020-08-13):
> The channel layouts accepted by ff_merge_channel_layouts() are of two
> types: Ordinary channel layouts and generic channel layouts. These are
> layouts that match all layouts with a certain number of channels.
> Therefore parsing these channel layouts is not done in one go; instead
> first the intersection of the ordinary layouts of the first input
> list of channel layouts with the ordinary layouts of the second list is
> determined, then the intersection of the ordinary layouts of the first
> one and the generic layouts of the second one etc. In order to mark the
> ordinary channel layouts that have already been matched as used they are
> zeroed. The inner loop that does this is as follows:
> 
> for (j = 0; j < b->nb_channel_layouts; j++) {
>     if (a->channel_layouts[i] == b->channel_layouts[j]) {
>         ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>         a->channel_layouts[i] = b->channel_layouts[j] = 0;
>     }
> }
> 
> (Here ret->channel_layouts is the array containing the intersection of
> the two input arrays.)
> 
> Yet the problem with this code is that after a match has been found, the
> loop continues the search with the new value a->channel_layouts[i].
> The intention of zeroing these elements was to make sure that elements
> already paired at this stage are ignored later. And while they are indeed
> ignored when pairing ordinary and generic channel layouts later, it has
> the exact opposite effect when pairing ordinary channel layouts.
> 
> To see this consider the channel layouts A B C D E and E D C B A. In the
> first round, A and A will be paired and added to ret->channel_layouts.
> In the second round, the input arrays are 0 B C D E and E D C B 0.
> At first B and B will be matched and zeroed, but after doing so matching
> continues, but this time it will search for 0, which will match with the
> last entry of the second array. ret->channel_layouts now contains A B 0.
> In the third round, C 0 0 will be added to ret->channel_layouts etc.
> This gives a quadratic amount of elements, yet the amount of elements
> allocated for said array is only the sum of the sizes of a and b.
> 
> This issue can e.g. be reproduced by
> ffmpeg -f lavfi -i anullsrc=cl=7.1 \
> -af 'aformat=cl=mono|stereo|2.1|3.0|4.0,aformat=cl=4.0|3.0|2.1|stereo|mono' \
> -f null -

Oh, good catch. It is a bug indeed.

> The fix is easy: break out of the inner loop after having found a match.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> 1. This patch is intended to be applied before
> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267790.html
> 

> 2. After this patch the code will stay within the buffer provided that
> no list contains one and the same generic channel layout multiple times
> (ordinary channel layouts may appear arbitrary often and there may even
> be ordinary channel layouts and generic channel layouts of the same
> channel count in the same list). But is this actually ensured? 
> (Given that the concept of generic channel layouts is not part of the
> public API, but just valid in AVFilterChannelLayouts the question can be
> rephrased as: Is it possible for the (API) user to somehow create
> generic channel layouts?)

I thought it was ensured by the rest of the code, where the lists of
supported formats are very limited. But I forgot what you found: a few
filters accept them from the application or the user. In particular,
bufferink can inject generic layouts.

I will make a patch that checks the validity after calling
query_formats.

> 
>  libavfilter/formats.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 00d050e439..f39396ed81 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -219,6 +219,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>              if (a->channel_layouts[i] == b->channel_layouts[j]) {
>                  ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>                  a->channel_layouts[i] = b->channel_layouts[j] = 0;
> +                break;
>              }
>          }
>      }

I think you can add the break to the other two loops too.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200813/146a1f60/attachment.sig>