[FFmpeg-devel] [PATCH 2/5] avcodec/atrac9dec: Check conditions before apply_band_extension() to avoid out of array read in initialization of unused variables

[Michael Niedermayer]

On Sun, Jun 16, 2019 at 09:11:05PM +0200, Michael Niedermayer wrote:
> On Sun, Jun 16, 2019 at 12:20:35PM +0200, Lynne wrote:
> > Jun 15, 2019, 11:00 PM by michael at niedermayer.cc:
> > 
> > > Fixes: global-buffer-overflow
> > > Fixes: 15247/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ATRAC9_fuzzer-5671602181636096
> > >
> > > 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/atrac9dec.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/libavcodec/atrac9dec.c b/libavcodec/atrac9dec.c
> > > index 805d46f3b8..5401d6e19e 100644
> > > --- a/libavcodec/atrac9dec.c
> > > +++ b/libavcodec/atrac9dec.c
> > > @@ -535,9 +535,6 @@ static inline void apply_band_extension(ATRAC9Context *s, ATRAC9BlockData *b,
> > >  at9_q_unit_to_coeff_idx[g_units[3]],
> > >  };
> > >  
> > > -    if (!b->has_band_ext || !b->has_band_ext_data)
> > > -        return;
> > > -
> > >  for (int ch = 0; ch <= stereo; ch++) {
> > >  ATRAC9ChannelData *c = &b->channel[ch];
> > >  
> > > @@ -741,7 +738,9 @@ static int atrac9_decode_block(ATRAC9Context *s, GetBitContext *gb,
> > >  
> > >  apply_intensity_stereo(s, b, stereo);
> > >  apply_scalefactors    (s, b, stereo);
> > > -    apply_band_extension  (s, b, stereo);
> > > +
> > > +    if (b->has_band_ext && b->has_band_ext_data)
> > > +        apply_band_extension  (s, b, stereo); 
> > >
> > 
> > False positive as usual, q_unit_cnt can't be anything out of array since its looked up from
> > at9_tab_band_q_unit_map.
> > I'd really appreciate it if you stopped fixing complaint messages from automated tools.
> > Especially from overflows and fuzzing timeouts. The latter are completely useless and
> > often make the code look worse and weird, and the former are all useless except when
> > outside of DSP code (e.g. malloc). And most of our code is DSP.
> 
> Calm down please, ill explain how this is reading out of array
> 
> In fact there seem to be more ways than i realized before that this can read
> out of array, so i will post 2 more patches to fix this more completely
> 
> First q_unit_cnt is only set from at9_tab_band_q_unit_map if you are lucky as
> the code is conditional on a bit read from the bitstream.
> 
> Second the values in at9_tab_band_q_unit_map start like this 0, 4, 8, 10, 12
> 
> The code reading out of array is this:
> 
>     const int g_units[4] = { /* A, B, C, total units */
>         b->q_unit_cnt,
>         at9_tab_band_ext_group[b->q_unit_cnt - 13][0],
>         at9_tab_band_ext_group[b->q_unit_cnt - 13][1],
>         
> if q_unit_cnt is less than 13 the index is negative and that reads out of the array
> teh value is not used later in the sample but the program could have crashed already
> 
> It is very good that we discuss this here though as the fix from the patch
> does not appear to be enough. There are more pathes that can lead to this.

this patch here is still needed, so i will apply it in the next days unless
someone objects or has a better idea

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190629/56a6b434/attachment.sig>