New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 161639 link

Starred by 1 user

Issue metadata

Status: Fixed
Closed: Dec 2012
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Security

Sign in to add a comment

Security: ffmpeg oob write4 (222)

Reported by, Nov 17 2012

Issue description


==12054== ERROR: AddressSanitizer stack-buffer-overflow on address 0xbf8f2730 at pc 0x8c02b4d bp 0xbf8f25b8 sp 0xbf8f25b0
WRITE of size 4 at 0xbf8f2730 thread T0
    #0 0x8c02b4c in apply_tns /home/p/ffmpeg-1.0/libavcodec/aacdec.c:2051
    #1 0x8c02188 in apply_ltp /home/p/ffmpeg-1.0/libavcodec/aacdec.c:2108
    #2 0x8c015ed in spectral_to_sample /home/p/ffmpeg-1.0/libavcodec/aacdec.c:2322
    #3 0x8bfd735 in aac_decode_frame_int /home/p/ffmpeg-1.0/libavcodec/aacdec.c:2527
    #4 0x8bfc209 in aac_decode_frame /home/p/ffmpeg-1.0/libavcodec/aacdec.c:2636
    #5 0x8a2e8e0 in avcodec_decode_audio4 /home/p/ffmpeg-1.0/libavcodec/utils.c:1683
    #6 0x80a451c in decode_audio /home/p/ffmpeg-1.0/ffmpeg.c:1445
    #7 0x80a168a in output_packet /home/p/ffmpeg-1.0/ffmpeg.c:1772
    #8 0x80ae426 in process_input /home/p/ffmpeg-1.0/ffmpeg.c:2840
    #9 0x809e7bd in transcode_step /home/p/ffmpeg-1.0/ffmpeg.c:2936
    #10 0x8095bc3 in transcode /home/p/ffmpeg-1.0/ffmpeg.c:2988
    #11 0x8095906 in main /home/p/ffmpeg-1.0/ffmpeg.c:3168
    #12 0xb756c4d2 in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
Address 0xbf8f2730 is located at offset 240 in frame <apply_tns> of T0's stack:
  This frame has 2 object(s):
    [32, 112) 'lpc'
    [160, 240) 'tmp'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism
      (longjmp and C++ exceptions *are* supported)
Shadow byte and word:
  0x37f1e4e6: f4
  0x37f1e4e4: 00 00 f4 f4
More shadow bytes:
  0x37f1e4d4: 00 00 f4 f4
  0x37f1e4d8: f2 f2 f2 f2
  0x37f1e4dc: 00 00 00 00
  0x37f1e4e0: 00 00 00 00
=>0x37f1e4e4: 00 00 f4 f4
  0x37f1e4e8: f3 f3 f3 f3
  0x37f1e4ec: 00 00 00 00
  0x37f1e4f0: 00 00 00 00
  0x37f1e4f4: 00 00 00 00
Stats: 3M malloced (1M for red zones) by 1637 calls
Stats: 0M realloced by 98 calls
Stats: 2M freed by 1406 calls
Stats: 0M really freed by 0 calls
Stats: 40M (10246 full pages) mmaped in 10 calls
  mmaps   by size class: 8:16383; 9:8191; 10:4095; 11:2047; 12:1024; 13:512; 14:256; 15:128; 16:64; 19:8;
  mallocs by size class: 8:1086; 9:313; 10:27; 11:56; 12:4; 13:8; 14:132; 15:5; 16:2; 19:4;
  frees   by size class: 8:951; 9:241; 10:23; 11:50; 12:3; 13:4; 14:128; 15:3; 16:1; 19:2;
  rfrees  by size class:
Stats: malloc large: 4 small slow: 29
==12054== ABORTING



Compile ffmpeg with ASAN and run:
./ffmpeg -i 222.min -f null - 2>&1 |
31.9 KB Download
Can you reproduce this in chrome ?

Comment 2 by, Nov 18 2012

I wasn't able to mux this stream into something digestible by chrome like mp4.
Maybe super Dale has a few minutes to see if this can be turned into something that affects Chrome?
+rbultje will know better than I.

I'll take a look on Monday if it can be muxed.
I can confirm this crashes chrome (ffmpeg -i 222.min -f mp4 -acodec copy file.m4a). Alex Converse (youtube team, he knows more about AAC than me) may be able to tell you what the correct fix is, but it seems that there's a one-off in the TNS MA filter code (see patch coming in next msg):

    float tmp[TNS_MAX_ORDER];
                    for (i = order; i > 0; i--)
                        tmp[i] = tmp[i - 1];

order goes up until TNS_MAX_ORDER, so writing into tmp[order] is invalid. It seems to me it should simply start the loop from order - 1.
34.0 KB View Download
Proposed patch.
886 bytes View Download
Quick update, some "fate" tests fail in ffmpeg with that patch applied, so it may be incorrect and we may have to increase the size of the "tmp" array mentioned earlier by one. Again, Alex Converse will know better than me...

Comment 8 by, Nov 18 2012

"order" is set here:

file: ffmpeg-1.0\libavcodec\aacdec.c
func: decode_tns
(1)              if ((tns->order[w][filt] = get_bits(gb, 5 - 2 * is8)) > tns_max_order) {
                    av_log(ac->avctx, AV_LOG_ERROR, "TNS filter order %d is greater than maximum %d.\n",
                           tns->order[w][filt], tns_max_order);
                    tns->order[w][filt] = 0;
                    return -1;

1 - tns_max_order is equal to 20 or 12. Perhaps patching ">" to ">=" would be better?

Comment 9 by, Nov 18 2012

Btw, are there any 'google chrome' [0] ASAN builds which are able to consume h264/aac?

0 -
Do you guys mind if I CC Alex (aconverse@)? He'll probably know which of the proposed solutions is likely best.
Labels: reward-topanel SecSeverity-High SecImpacts-Stable SecImpacts-Beta Feature-Media-Codecs Mstone-23
@rbultje: go right ahead!

@pawlkt: I don't think we provide any officially but if you were building your own Chromium under ASAN, you can make it more Chrome-like with something like

export GYP_DEFINES='branding=Chrome buildtype=Official'

Status: Assigned
To build chrome with all codecs you can set ffmpeg_branding=Chrome (or ChromeOS on Linux to get all the CrOS codecs too) and proprietary_codecs=1. No need for the branding=Chrome buildtype=Official.

Alex, it seems the TNS ma filter code has a stack overwrite that can crash chrome (see earlier comments or sample .m4a file, aacdec.c line 2016 in current Libav git master, or line 2051 in ffmpeg git master). Can you comment on which of the proposed fixes (increase stack array size by 1, clamp order by one less, fix the bounds of the ma filter loop to start at order-1, something else?) is the right thing to do from an algorithmic point of view without risking to break AAC decoding of valid files?
order = 20 should never occur during TNS forward mode (ma). Order 20 can only occur in AOT MAIN and forward operation can only happen when LTP is used (the LTP tool does not operate in AOT MAIN). So the decoder is already in an illegal state when we enter this function, which isn't great. 

For this particular crash, you definitely want to make tmp one larger. I'm looking deeper into how we go into a the illegal state now. 
The order=20 in LTP issue is a result from switching from MAIN profile to LTP profile while not coding the the channel with the large TNS value in the first frame after the switch of to MAIN. 
@Alex are you working on a correct fix for this or should we just increase tmp?
Increase tmp for now. I may try to reject cases like this earlier in the decoding process, but that will take some further thought and even then it still may not have 100% coverage. 
Project Member

Comment 19 by, Nov 19 2012

The following revision refers to this bug:

r31003 | | 2012-11-19T22:45:31.790129Z

M25 fix is in the CQ now. I'll need to branch the M24 and M23 version of FFmpeg in order to update them since they're not tracking M25 at present.
Project Member

Comment 21 by, Nov 20 2012

The following revision refers to this bug:

r168731 | | 2012-11-20T04:36:55.170313Z

Changed paths:

Roll FFMpeg DEPS to pick up security fixes.

- Adds a test case for the issue.
- Fixes ordering issues with EXPECT_EQ()
- Notes a recent memory leak regression.
- Fixes stale errors and hashes for unit test.

82ae69c Prevent stack buffer overflow during AAC decoding.

BUG= 161639 
TEST=unit test under asan/valgrind.

Review URL:
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased
Project Member

Comment 23 by, Nov 21 2012

Labels: -Merge-Approved merge-merged-ffmpeg
The following revision refers to this bug:

r168939 | | 2012-11-21T01:06:39.555363Z

Changed paths:

Prevent stack buffer overflow during AAC decoding. BUG= 161639 
Project Member

Comment 24 by, Nov 21 2012

Labels: merge-merged-1312
The following revision refers to this bug:

r31048 | | 2012-11-21T01:11:25.041937Z

Merged for M24 which thankfully requires no binary rolls now! (Yay!) But still need to merge to M23 tomorrow with binary roll :(
Labels: Merge-Approved
Marking Merge-Approved, we'll merge this to the final M23 patch.
Project Member

Comment 27 by, Dec 1 2012

Labels: -Merge-Approved
The following revision refers to this bug:

r170612 | | 2012-12-01T00:26:35.052646Z

Changed paths:

Prevent stack buffer overflow during AAC decoding. BUG= 161639 
Thanks, Dale. Are the Windows binaries taken care of too?

@pawlkt: do you want to make a case about exploitability? It seems to me that writing an arbitrary 4-byte value exactly 4 bytes out-of-bounds on the stack would exactly clobber the canary value and nothing else.

Is there a Chromium platform that hasn't opted in to stack canaries?
Project Member

Comment 29 by, Dec 1 2012

The following revision refers to this bug:

r31319 | | 2012-12-01T01:37:29.659192Z

Just rolled the M23 deps, so everything should be taken care of. I verified this does not crash anymore with an M23 build and the new DLLs.
Project Member

Comment 31 by, Dec 1 2012

Labels: merge-merged-1271
The following revision refers to this bug:

r31320 | | 2012-12-01T01:44:25.382088Z


Comment 32 by, Dec 1 2012


Here's where chrome for windows (23.0.1271.91 m) is crashing when parsing test.m4a:

(1ea0.14f4): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00000000 ebx=00000000 ecx=00000001 edx=00000003 esi=03d2f980 edi=03d31980
eip=6ac737e1 esp=0427f3d0 ebp=00000000 iopl=0         nv up ei pl zr na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010246
6ac737e1 0fb603          movzx   eax,byte ptr [ebx]         ds:002b:00000000=??

This isn't a canary fail.

avcodec-54.dll doesn't seem to be compiled with /GS -- I don't see any canary checks anywhere.
@pawlkt: thanks! That is very helpful and I trust your analysis. Looks bad to me. ebp is NULL so probably 0 was written as the 4-byte out of bounds write.
Labels: -reward-topanel reward-2000 reward-unpaid
@pawlkt: thanks for reporting this bug.

We're happy to reward it at $2000.
$1000 for the base bug and a $1000 bonus for finding an issue in an area of code that has been reasonably well fuzzed.

Boilerplate text:
Please do NOT publicly disclose details until a fix has been released to all our
users. Early public disclosure may cancel the provisional reward.
Also, please be considerate about disclosure when the bug affects a core library
that may be used by other products.
Please do NOT share this information with third parties who are not directly
involved in fixing the bug. Doing so may cancel the provisional reward.
Please be honest if you have already disclosed anything publicly or to third parties.
Labels: CVE-2012-5144
Labels: -reward-unpaid
Payment in system.
Status: Fixed
Project Member

Comment 38 by, Mar 10 2013

Labels: -Type-Security -SecSeverity-High -SecImpacts-Stable -SecImpacts-Beta -Feature-Media-Codecs -Mstone-23 Security-Impact-Stable Security-Impact-Beta M-23 Security-Severity-High Cr-Internals-Media-Codecs Type-Bug-Security
Project Member

Comment 39 by, Mar 11 2013

Labels: -Area-Undefined
Labels: -Restrict-View-SecurityNotify
Project Member

Comment 41 by, Mar 21 2013

Labels: -Security-Severity-High Security_Severity-High
Project Member

Comment 42 by, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 43 by, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 44 by, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 45 by, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member

Comment 46 by, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic

Comment 48 Deleted

Labels: CVE_description-submitted
Project Member

Comment 50 by, Jul 29

Labels: -Pri-0 Pri-1

Sign in to add a comment