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

Submitted by Michael Niedermayer on June 15, 2019, 10 p.m.

Details

Message ID 20190615220056.21784-2-michael@niedermayer.cc
State Accepted
Commit c5f265bb2468c3e0996329ada11b2792dd9bd1a2
Headers show

Commit Message

Michael Niedermayer June 15, 2019, 10 p.m.
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@niedermayer.cc>
---
 libavcodec/atrac9dec.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Lynne June 16, 2019, 10:20 a.m.
Jun 15, 2019, 11:00 PM by michael@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@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.
Michael Niedermayer June 16, 2019, 7:11 p.m.
On Sun, Jun 16, 2019 at 12:20:35PM +0200, Lynne wrote:
> Jun 15, 2019, 11:00 PM by michael@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@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.

Thanks

PS: If anyone has atrac9 samples for testing, or could add atrac9 samples to FATE
that would be very helpfull in ensuring that none of these changes break anything

[...]
Michael Niedermayer June 29, 2019, 4:57 p.m.
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@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@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

[...]

Patch hide | download patch | download mbox

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);
 
 imdct:
     for (int i = 0; i <= stereo; i++) {