[FFmpeg-devel,2/2] avcodec/alac: Check decorr_shift to avoid invalid shift

Fixes: shift exponent 128 is too large for 32-bit type 'int'
Fixes: 23860/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5751138914402304

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

Hi Michael!

On 2020-07-24 00:11 +0200, Michael Niedermayer wrote:
> Fixes: shift exponent 128 is too large for 32-bit type 'int'
> Fixes: 23860/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5751138914402304
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/alac.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> index bf05406230..9040673528 100644
> --- a/libavcodec/alac.c
> +++ b/libavcodec/alac.c
> @@ -302,6 +302,9 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
>          decorr_shift       = get_bits(&alac->gb, 8);
>          decorr_left_weight = get_bits(&alac->gb, 8);
>
> +        if (channels == 2 && decorr_left_weight && decorr_shift > 31)
> +            return AVERROR_INVALIDDATA;

IMHO this check looks a bit specific in this patch. If I read the
calling function and the conditions that guard the called function
where the problem was detected it makes sense.

A little elaboration in the commit message body would have made this
change easier to agree on without checking the surroundings:

   Later the decorrelate_stereo call is guarded by channels == 2
   and non-zero decorr_left_weight. Make sure decorr_shift is in
   the expected shift range for that case.

I started thinking about the robustness of this fix because of
the channels == 2. Thinking if channel will end up somehow not
being 2, would that circumvent the check? As I found out no,
since decorrelate_stereo would not be called.

Leaving the patch as is, seems fine to me. decode_element will
only ever be called with values 1 or 2 and the function is so
deep down in the decoder that it seems not justified to make it
more flexible.


> +
>          for (ch = 0; ch < channels; ch++) {
>              prediction_type[ch]   = get_bits(&alac->gb, 4);
>              lpc_quant[ch]         = get_bits(&alac->gb, 4);
> --


  Alexander

On Fri, Jul 24, 2020 at 08:31:04PM +0200, Alexander Strasser wrote:
> Hi Michael!
> 
> On 2020-07-24 00:11 +0200, Michael Niedermayer wrote:
> > Fixes: shift exponent 128 is too large for 32-bit type 'int'
> > Fixes: 23860/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5751138914402304
> >
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/alac.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> > index bf05406230..9040673528 100644
> > --- a/libavcodec/alac.c
> > +++ b/libavcodec/alac.c
> > @@ -302,6 +302,9 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
> >          decorr_shift       = get_bits(&alac->gb, 8);
> >          decorr_left_weight = get_bits(&alac->gb, 8);
> >
> > +        if (channels == 2 && decorr_left_weight && decorr_shift > 31)
> > +            return AVERROR_INVALIDDATA;
> 
> IMHO this check looks a bit specific in this patch. If I read the
> calling function and the conditions that guard the called function
> where the problem was detected it makes sense.
> 
> A little elaboration in the commit message body would have made this
> change easier to agree on without checking the surroundings:
> 

>    Later the decorrelate_stereo call is guarded by channels == 2
>    and non-zero decorr_left_weight. Make sure decorr_shift is in
>    the expected shift range for that case.

will add this


> 
> I started thinking about the robustness of this fix because of
> the channels == 2. Thinking if channel will end up somehow not
> being 2, would that circumvent the check? As I found out no,
> since decorrelate_stereo would not be called.
> 
> Leaving the patch as is, seems fine to me. decode_element will
> only ever be called with values 1 or 2 and the function is so
> deep down in the decoder that it seems not justified to make it
> more flexible.

will apply

thanks

[...]