Skip to content

Commit

Permalink
syntax.c: check for syntax element inconsistencies
Browse files Browse the repository at this point in the history 
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 #26) and possibly other
related issues.
  • Loading branch information
@hlef
hlef committed Apr 11, 2019
1 parent 7da4a83 commit 466b01d
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions libfaad/syntax.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,12 @@ static void decode_sce_lfe(NeAACDecStruct *hDecoder,
can become 2 when some form of Parametric Stereo coding is used
*/

if (hDecoder->frame && hDecoder->element_id[hDecoder->fr_ch_ele] != id_syn_ele) {
/* element inconsistency */
hInfo->error = 21;
return;
}

/* save the syntax element id */
hDecoder->element_id[hDecoder->fr_ch_ele] = id_syn_ele;

Expand Down Expand Up @@ -395,6 +401,12 @@ static void decode_cpe(NeAACDecStruct *hDecoder, NeAACDecFrameInfo *hInfo, bitfi
return;
}

if (hDecoder->frame && hDecoder->element_id[hDecoder->fr_ch_ele] != id_syn_ele) {
/* element inconsistency */
hInfo->error = 21;
return;
}

/* save the syntax element id */
hDecoder->element_id[hDecoder->fr_ch_ele] = id_syn_ele;

Expand Down

6 comments on commit 466b01d

@fabiangreffrath
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hlef Apparently, this commit breaks decoding for many files that should be valid otherwise e.g. mixxxdj/mixxx#2738.

IIRC, the issue arises if the number of channels in a frame is more than the number of channels for which memory is allocated. However, there may be valid uses cases out there in the wild, e.g. when parametric stereo is involved or something like this, IDK.

However, couldn't we probably make these checks less strict if we replace the != condition with a < (or >?) check so that it's okay to have, say, only one channel in the frame altough memory is allocated for two?

@uklotzde
Copy link

@uklotzde uklotzde commented on 466b01d Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error seems to affect files both encoded with iTunes or files purchased from the store.

We need to skip some frames manually (consisting of 6 bytes each as I noticed) after this error occurs and then everything works fine. I have no idea what these frame contain as they are too short for ADTS frames:
mixxxdj/mixxx#2850

We receives these framesblocks from libmp4v2 / MP4ReadSample and forward them into the decoder.

@awesie
Copy link
Contributor

@awesie awesie commented on 466b01d Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the commit seems to be related to checking that hDecoder->frame is non-zero. While the logic detailed in the commit messages is true for the faad2 frontend, it is not necessarily true for other users of the library (for instance mixxx) that use NeAACDecPostSeekReset. This is why the issue was not reproducible with the faad2 frontend.

When mixxx opens an audio file, it seeks to the first frame, which they call frame 1. While frame numbering starts from 1 for mp4, I'm not sure if this is true for faad2, so this may be incorrect API usage by mixxx. But, in any case, this should be fixed in faad2 because it should presumably be valid to call NeAACDecPostSeekReset with a non-zero frame number before decoding any frames.

After NeAACDecPostSeekReset is called with a non-zero frame number, this updates hDecoder->frame but the element_id array is still invalid (e.g. zero initialized). Then the code in this patch will cause any future decodings to fail until hDecoder->frame is zero.

My suggestion for fixing this is to stop overloading the use of hDecoder->frame. Maybe initialize element_id to an invalid ID (e.g. 255) when you open the decoder, and modify this check to:

if (hDecoder->element_id[hDecoder->fr_ch_ele] != INVALID_ELEMENT_ID && hDecoder->element_id[hDecoder->fr_ch_ele] != id_syn_ele) {

@fabiangreffrath
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive! Could you possibly prepare a PR for this if you see it fit? Thank you!

@uklotzde
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a 0-based index doesn't work either. The decoding errors at the start are are fixed but then I get mismatches between the seek-and-decode vs. continuous-decoding results. And the number of MP4 sample frames has to be decreased by 1, otherwise seeking to and decoding the last addressable frame fails. We have tests for all those edge cases.

I am not able exclude errors in our index calculation, though.

@fabiangreffrath
Copy link
Collaborator

@fabiangreffrath fabiangreffrath commented on 466b01d Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is about switching to a 0-based index, but about initializing the element_id field with a known-invalid value. Could you please check if https://github.com/knik0/faad2/pull/64/files fixes the issues you found with mixxx and faad2 for all the edge cases?

Please sign in to comment.