Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null pointer dereference vulnerability in ifilter_bank (libfaad/filtbank.c:275) #26

Closed
fantasy7082 opened this issue Dec 17, 2018 · 1 comment

Comments

@fantasy7082
Copy link

@fantasy7082 fantasy7082 commented Dec 17, 2018

Hi, i found a null pointer dereference bug in Freeware Advanced Audio Decoder 2 (FAAD2) 2.8.8. It crashed in function ifilter_bank.the details are below(ASAN):

./faad faad_res/008-null-point-filtbank_275 -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/008-null-point-filtbank_275 file info:
ADTS, 0.043 sec, 74 kbps, 48000 Hz

  ---------------------
 | Config:  2 Ch       |
  ---------------------
 | Ch |    Position    |
  ---------------------
 | 00 | Left front     |
 | 01 | Right front    |
  ---------------------

ASAN:SIGSEGV faad_res/008-null-point-filtbank_275.
=================================================================
==7076==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f5946ec66b1 bp 0x7fff7f757780 sp 0x7fff7f755690 T0)
    #0 0x7f5946ec66b0 in ifilter_bank /root/faad2_asan/libfaad/filtbank.c:275
    #1 0x7f5946f0119d in reconstruct_channel_pair /root/faad2_asan/libfaad/specrec.c:1258
    #2 0x7f5946f07823 in channel_pair_element /root/faad2_asan/libfaad/syntax.c:759
    #3 0x7f5946f05cbf in decode_cpe /root/faad2_asan/libfaad/syntax.c:402
    #4 0x7f5946f06398 in raw_data_block /root/faad2_asan/libfaad/syntax.c:448
    #5 0x7f5946ec09c3 in aac_frame_decode /root/faad2_asan/libfaad/decoder.c:990
    #6 0x7f5946ec0566 in NeAACDecDecode /root/faad2_asan/libfaad/decoder.c:821
    #7 0x40f8ae in decodeAACfile /root/faad2_asan/frontend/main.c:679
    #8 0x411dd4 in faad_main /root/faad2_asan/frontend/main.c:1323
    #9 0x411fe5 in main /root/faad2_asan/frontend/main.c:1366
    #10 0x7f5946af882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #11 0x401aa8 in _start (/usr/local/faad-asan/bin/faad+0x401aa8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /root/faad2_asan/libfaad/filtbank.c:275 ifilter_bank
==7076==ABORTING

POC FILE:https://github.com/fantasy7082/image_test/blob/master/008-null-point-filtbank_275

@hlef
Copy link
Contributor

@hlef hlef commented Feb 11, 2019

I had a look at this issue. This crash appears to happen with all files defining SCE (single channel) frames followed (directly or not) by CPE (stereo) frames.

Rationale: The decoder struct contains a mem_alloced array used to keep track of allocated resources. This array is used to avoid allocating and freeing memory for each single frame. Instead, buffers are allocated when they are first needed for a frame, and the mem_alloced flag corresponding to these buffers is set. Following frames just check for the mem_alloced flag, allowing them to skip the allocation part and use the buffers directly.

Problem: some buffers like hDecoder->fb_intermed are initalized differently depending on whether the frame is mono or stereo.

The result is that reconstruct_channel_pair checks whether the buffer as been initialized, which returns true because reconstruct_single_channel initialized it before, but in fact it is only 'half' initialized because only buffers for the first channel have been allocated, they are still 0x0 for the second channel.

This is a design issue.

Possible fix:

  • we should keep the changes small for now, this is a security patch, so I guess this excludes refactoring around this mem_alloced array.
  • we could unalloc the buffers and unset the mem_alloced flag when switching between frames but this might have a terrible perf cost.
  • we could store more information in mem_alloced (not only 0/1 but the number of channels for example?) but this is not very elegant and might require too many changes.
  • we could add a small check to functions like reconstruct_channel_pair to make sure that both channels are well allocated, even if the bit is set. This is not very elegant because it comes down to kind of stop trusting mem_alloced, but this is probably the best complexity / perf tradeoff

I'll PR the last suggestion for now.

FTR: this issue was assigned CVE-2018-20362.

hlef added a commit to hlef/faad2 that referenced this issue Feb 25, 2019
The faad2 code base appears to assume that all frames of a same AAC
file share the same syntax element structure. However, input files
are not strictly verified again this assumption, or too late during
file processing.

This leads to security vulnerabilities when processing crafted AAC
files where these assumptions are false. For example, files declaring
two frames, the first SCE+FIL and the second CPE.

Add checks to decode_sce_lfe and decode_cpe to make sure inter-frame
inconsistencies are detected as early as possible.

These checks first read hDecoder->frame: if this is not the first
frame then we make sure that the syntax element at the same position
in the previous frame also had element_id id_syn_ele. If not, return
21 as this is a fatal file structure issue.

This patch addresses CVE-2018-20362 (fixes knik0#26) and possibly other
related issues.
hlef added a commit to hlef/faad2 that referenced this issue Apr 11, 2019
Implicit channel mapping reconfiguration is explicitely forbidden by
ISO/IEC 13818-7:2006 (8.5.3.3). Decoders should be able to detect such
files and reject them. FAAD2 does not perform any kind of checks
regarding this.

This leads to security vulnerabilities when processing crafted AAC
files performing such reconfigurations.

Add checks to decode_sce_lfe and decode_cpe to make sure such
inconsistencies are detected as early as possible.

These checks first read hDecoder->frame: if this is not the first
frame then we make sure that the syntax element at the same position
in the previous frame also had element_id id_syn_ele. If not, return
21 as this is a fatal file structure issue.

This patch addresses CVE-2018-20362 (fixes knik0#26) and possibly other
related issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants