Invalid memory address dereference in sbr_process_channel (in libfaad/sbr_dec.c:375)

[fantasy7082]

Hi, i found a issue in Freeware Advanced Audio Decoder 2 (FAAD2) 2.8.8. It crashed in function sbr_process_channel .the details are below(ASAN):

./faad faad_res/012-invalid-def-sbr_dec_375 -o out.wav
 *********** Ahead Software MPEG-4 AAC Decoder V2.8.8 ******************

 Build: Dec 13 2018
 Copyright 2002-2004: Ahead Software AG
 http://www.audiocoding.com
 bug tracking: https://sourceforge.net/p/faac/bugs/
 Floating point version

 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License.

 **************************************************************************

faad_res/012-invalid-def-sbr_dec_375 file info:
ADTS, 0.043 sec, 37 kbps, 48000 Hz

  ---------------------
 | Config: 15 Ch       |
  ---------------------
 | Ch |    Position    |
  ---------------------
 | 00 | Left front     |
 | 01 | Right front    |
 | 02 | Unknown        |
 | 03 | Unknown        |
 | 04 | Unknown        |
 | 05 | Unknown        |
 | 06 | Unknown        |
 | 07 | Unknown        |
 | 08 | Unknown        |
 | 09 | Unknown        |
 | 10 | Unknown        |
 | 11 | Unknown        |
 | 12 | Unknown        |
 | 13 | Unknown        |
 | 14 | Unknown        |
  ---------------------

ASAN:SIGSEGVfaad_res/012-invalid-def-sbr_dec_375.
=================================================================
==7096==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000009 (pc 0x7f9edd65e76f bp 0x7ffff6126140 sp 0x7ffff6126100 T0)
    #0 0x7f9edd65e76e in sbr_process_channel /root/faad2_asan/libfaad/sbr_dec.c:375
    #1 0x7f9edd660049 in sbrDecodeSingleFrame /root/faad2_asan/libfaad/sbr_dec.c:562
    #2 0x7f9edd6089a1 in reconstruct_single_channel /root/faad2_asan/libfaad/specrec.c:1067
    #3 0x7f9edd610e28 in single_lfe_channel_element /root/faad2_asan/libfaad/syntax.c:631
    #4 0x7f9edd60f354 in decode_sce_lfe /root/faad2_asan/libfaad/syntax.c:351
    #5 0x7f9edd6102da in raw_data_block /root/faad2_asan/libfaad/syntax.c:441
    #6 0x7f9edd5ca9c3 in aac_frame_decode /root/faad2_asan/libfaad/decoder.c:990
    #7 0x7f9edd5ca566 in NeAACDecDecode /root/faad2_asan/libfaad/decoder.c:821
    #8 0x40f8ae in decodeAACfile /root/faad2_asan/frontend/main.c:679
    #9 0x411dd4 in faad_main /root/faad2_asan/frontend/main.c:1323
    #10 0x411fe5 in main /root/faad2_asan/frontend/main.c:1366
    #11 0x7f9edd20282f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #12 0x401aa8 in _start (/usr/local/faad-asan/bin/faad+0x401aa8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /root/faad2_asan/libfaad/sbr_dec.c:375 sbr_process_channel
==7096==ABORTING

POC FILE:https://github.com/fantasy7082/image_test/blob/master/012-invalid-def-sbr_dec_375

[hlef]

Reproducible on the latest master. I'm taking a look at it.

This was assigned CVE-2018-20360.

[hlef]

This is a complicated problem. I'll try to summarize my current understanding of the issue:

The first frame has 15 channels, each channel has exactly one output channel.

The second frame still has 15 channels, but the first frame channel suddenly uses parametric stereo (PS). As a result, there are now 16 output channels, and the first frame channel has now two outputs. This is handled by the following code, in reconstruct_single_channel:

 913     if (hDecoder->element_output_channels[hDecoder->fr_ch_ele] == 0)
 914     {
 915         /* element_output_channels not set yet */
 916         hDecoder->element_output_channels[hDecoder->fr_ch_ele] = output_channels;
 917     } else if (hDecoder->element_output_channels[hDecoder->fr_ch_ele] != output_channels) {
 918         /* element inconsistency */
 919
 920         /* this only happens if PS is actually found but not in the first frame
 921          * this means that there is only 1 bitstream element!
 922          */
 923
 924         /* reset the allocation */
 925         hDecoder->element_alloced[hDecoder->fr_ch_ele] = 0;
 926
 927         hDecoder->element_output_channels[hDecoder->fr_ch_ele] = output_channels;
 928
 929         //return 21;
 930     }

Frame channel 1 has two outputs, so we set hDecoder->element_output_channels[1] to 2.

Now, there is a problem.

At the end of decode_sce_lfe we call

 372     hDecoder->fr_channels += hDecoder->element_output_channels[hDecoder->fr_ch_ele];
 373     hDecoder->fr_ch_ele++;

This means that, once we have arrived to frame channel 14, hDecoder->fr_ch_ele is 14 but hDecoder->fr_channels is 15.

Because frame channel 1 has two output channels, output channel 14 actually belongs to frame channel 13. Frame channel 14's output channel is output channel 15. This channel is not allocated.

Why? We don't allocate output channel 15 because hDecoder->element_alloced[14] is 1.

We did allocation while processing the first frame, which only had 15 output channels.

In short:

• Parametric Stereo (PS) can arrive at any moment in the file.
• The current FAAD2 code tries to handle it, fails badly.

[hlef]

@fabiangreffrath I'd like to hear your thoughts about this, but I don't think I'll have time to really fix this. I don't even know whether this situation is legal or not.

I'd just remove this "feature" which is broken anyways.

diff --git a/libfaad/specrec.c b/libfaad/specrec.c
index 9797d6e..d6fdb35 100644
--- a/libfaad/specrec.c
+++ b/libfaad/specrec.c
@@ -917,16 +917,7 @@ uint8_t reconstruct_single_channel(NeAACDecStruct *hDecoder, ic_stream *ics,
     } else if (hDecoder->element_output_channels[hDecoder->fr_ch_ele] != output_channels) {
         /* element inconsistency */

-        /* this only happens if PS is actually found but not in the first frame
-         * this means that there is only 1 bitstream element!
-         */
-
-        /* reset the allocation */
-        hDecoder->element_alloced[hDecoder->fr_ch_ele] = 0;
-
-        hDecoder->element_output_channels[hDecoder->fr_ch_ele] = output_channels;
-
-        //return 21;
+        return 21;
     }

     if (hDecoder->element_alloced[hDecoder->fr_ch_ele] == 0)

Second alternative could be:

diff --git a/libfaad/specrec.c b/libfaad/specrec.c
index 9797d6e..d07f86b 100644
--- a/libfaad/specrec.c
+++ b/libfaad/specrec.c
@@ -915,18 +915,18 @@ uint8_t reconstruct_single_channel(NeAACDecStruct *hDecoder, ic_stream *ics,
         /* element_output_channels not set yet */
         hDecoder->element_output_channels[hDecoder->fr_ch_ele] = output_channels;
     } else if (hDecoder->element_output_channels[hDecoder->fr_ch_ele] != output_channels) {
-        /* element inconsistency */
-
-        /* this only happens if PS is actually found but not in the first frame
+        /* element inconsistency
+         * this only happens if PS is actually found but not in the first frame
          * this means that there is only 1 bitstream element!
          */

-        /* reset the allocation */
-        hDecoder->element_alloced[hDecoder->fr_ch_ele] = 0;
-
-        hDecoder->element_output_channels[hDecoder->fr_ch_ele] = output_channels;
-
-        //return 21;
+        if (hDecoder->fr_channels == 1) {
+            /* reset the allocation */
+            hDecoder->element_alloced[hDecoder->fr_ch_ele] = 0;
+            hDecoder->element_output_channels[hDecoder->fr_ch_ele] = output_channels;
+        } else {
+            return 21;
+        }
     }

     if (hDecoder->element_alloced[hDecoder->fr_ch_ele] == 0)

[fabiangreffrath]

The question is how often this feature is used and thus how many actual tracks are affected by this. Anyway, it is always better to return cleanly when encountering this than pretending to support it and then falling on the face. I like the second approach more, because it does at least something, but I'll leave it up to you which one to use for a PR.

[fcartegnie]

This breaks with legitimate PS streams where the channel number is "unknown" before PS. (fr_channels == 0)

[fabiangreffrath]

This breaks with legitimate PS streams where the channel number is "unknown" before PS. (fr_channels == 0)

Maybe this is the missing hint @hlef ?

[basicmaster]

This commit also affects DAB+ channels with PS: Opendigitalradio/dablin#61

It takes some frames (e.g. 20) until ps_data is present as part of an AAC frame. When this frame is then tried to be decoded, the decoding fails.

[fcartegnie]

This commit also affects DAB+ channels with PS: Opendigitalradio/dablin#61

It takes some frames (e.g. 20) until ps_data is present as part of an AAC frame. When this frame is then tried to be decoded, the decoding fails.

Yes, because DMB is exclusively PS based.

I'm working on a real fix.

[fcartegnie]

See #51 which should be sufficient.
The real issue being dereferencing null pointer structs because we're off by one after PS.