[FFmpeg-devel] [PATCH 2/3] avcodec/wmalosslessdec: Check channel mask against num channels

[Michael Niedermayer]

On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
> 
> 
> On 3/17/2022 9:07 PM, James Almer wrote:
> > 
> > 
> > On 3/17/2022 8:52 PM, James Almer wrote:
> > > On 3/17/2022 8:30 PM, Michael Niedermayer wrote:
> > > > Fixes: Out of array write
> > > > Fixes: 45613/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4539073606320128
> > > > 
> > > > 
> > > > Found-by: continuous fuzzing process
> > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > >   libavcodec/wmalosslessdec.c | 3 +++
> > > >   1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> > > > index cd05b22689..1728920729 100644
> > > > --- a/libavcodec/wmalosslessdec.c
> > > > +++ b/libavcodec/wmalosslessdec.c
> > > > @@ -281,6 +281,9 @@ static av_cold int
> > > > decode_init(AVCodecContext *avctx)
> > > >       av_channel_layout_uninit(&avctx->ch_layout);
> > > >       av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > > > +    if (s->num_channels != avctx->ch_layout.nb_channels)
> > > > +        return AVERROR_PATCHWELCOME; //are there non fuzzed
> > > > files with this or is it an error ?
> > > 
> > > s->num_channels at this point is set to the channels count the user
> > > set before calling avcodec_open2() (Normally from lavf), but it
> > > could be anything.
> > > If channel_mask is taken from extradata, maybe it should be used to
> > > set s->num_channels instead of aborting because the user set value
> > > and extradata disagreed.
> > > 
> > > Also, can you reproduce this crash before 3c933af493?
> > > s->num_channels was being set to the user set channel count too,
> > > same as now.
> > 
> > Right, before that commit s->num_channels and avctx->channels were
> > always the same, but avctx->channel_layout was whatever came from
> > extradata, and its popcnt could be != avctx->channels.
> > After it, avctx->ch_layout.nb_channels is always the same as
> > popcnt(avctx->ch_layout.u.mask), which can be different than
> > s->num_channels.
> > 
> > I think my suggestion above to use the extradata channel mask and
> > ignoring the user set channel count is the best approach for this.
> 
> Like this maybe (channel_mask could in theory be zero, so in that case the
> user set value should be used).
> 
> > diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> > index cd05b22689..915add1962 100644
> > --- a/libavcodec/wmalosslessdec.c
> > +++ b/libavcodec/wmalosslessdec.c
> > @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >          return AVERROR_PATCHWELCOME;
> >      }
> > 
> > -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > -    if (!s->frame_data)
> > -        return AVERROR(ENOMEM);
> > -
> > -    s->avctx = avctx;
> > -    ff_llauddsp_init(&s->dsp);
> > -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > -
> >      if (avctx->extradata_size >= 18) {
> >          s->decode_flags    = AV_RL16(edata_ptr + 14);
> >          channel_mask       = AV_RL32(edata_ptr +  2);
> > @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >          return AVERROR_PATCHWELCOME;
> >      }
> > 
> > +    if (channel_mask) {
> > +        av_channel_layout_uninit(&avctx->ch_layout);
> > +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> > +    } else
> > +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
> > +
> > +    s->num_channels = avctx->ch_layout.nb_channels;
> > +
> > +    /* extract lfe channel position */
> > +    s->lfe_channel = -1;
> > +
> > +    if (channel_mask & 8) {
> > +        unsigned int mask;
> > +        for (mask = 1; mask < 16; mask <<= 1)
> > +            if (channel_mask & mask)
> > +                ++s->lfe_channel;
> > +    }
> > +
> > +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
> > +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > +    if (!s->frame_data)
> > +        return AVERROR(ENOMEM);
> > +
> > +    s->avctx = avctx;
> > +    ff_llauddsp_init(&s->dsp);
> > +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
> > +
> >      /* generic init */
> >      s->log2_frame_size = av_log2(avctx->block_align) + 4;
> > 
> > @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >          return AVERROR_INVALIDDATA;
> >      }
> > 
> > -    s->num_channels = avctx->ch_layout.nb_channels;
> > -
> > -    /* extract lfe channel position */
> > -    s->lfe_channel = -1;
> > -
> > -    if (channel_mask & 8) {
> > -        unsigned int mask;
> > -        for (mask = 1; mask < 16; mask <<= 1)
> > -            if (channel_mask & mask)
> > -                ++s->lfe_channel;
> > -    }
> > -
> >      s->frame = av_frame_alloc();
> >      if (!s->frame)
> >          return AVERROR(ENOMEM);
> > 
> > -    av_channel_layout_uninit(&avctx->ch_layout);
> > -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
> >      return 0;
> >  }

this will change the output of the "lossless" decoder if this case ever
occurs besides this

LGTM

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220318/1b986633/attachment.sig>