816994 - Opus crash due to race [@opus_multistream_decode_native]

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

I am guessing that this crash is triggered by a race condition inside the media decoder state machine. To reproduce reload the testcase with cmd+r or f5 a few times. The crashing location will vary on each execution.

Tested with changeset: 114362:c63d5cff18ba

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Christoph Diehl [:posidron]]

Attachment: callstack_crashreporter

Comment 3

[Christoph Diehl [:posidron]]

Marked as s-s because I have seen ASan reporting heap-buffer-overflow signatures as well.

Comment 4

[Ralph Giles (:rillian)]

Attachment: testcase

I ran the testcase through rogg_crcfix so it fails without needing to patch out the libogg crc verification.

Comment 5

[Josh Aas]

Can we get an assignee here?

Comment 6

[Timothy B. Terriberry (:derf)]

I tried to reproduce this last week and failed, so assigning to rillian, who says he has managed to reproduce.

Comment 7

[Ralph Giles (:rillian)]

Attachment: gc callstack

I can reproduce this easily with a normal build, no asan required, but I've not made much progress tracking this down. Here's another backtrace, from a segfault inside the garbage collector, in case that's helpful.

Any ideas how to pursue this?

Comment 8

[Daniel Veditz [:dveditz]]

Although this could be a more recent regression, for now I'm going to assume it's been there in Opus all along until we know better.

Comment 9

[Ralph Giles (:rillian)]

That will speed up my bisection. Thanks!

Comment 10

[Ralph Giles (:rillian)]

Oh, wait. Did you actually verify the problem, or just guess?

Comment 11

[Ralph Giles (:rillian)]

I did a besection, just testing a normal build with the crc-fixed testcase, and hitting reload 10-20 times to see if I could get it to segfault. The first bad commit was:

commit d682b69bcedcbbf041e9a2ba67bed073736217da (git)
Author: Alexandros Chronopoulos <[e-mail]>
Date:   Thu Aug 9 15:53:23 2012 -0700

    Bug 748144 - Support multichannel Opus files. r=rillian

Which looks more helpful than I was expecting.

Comment 12

[Ralph Giles (:rillian)]

Another variation of the crash:

-919025920[7fff9c009dc0]: 2e300d0 Decoder playing 960 frames of data to stream for AudioData at 396000
Assertion failure: addr % CellSize == 0, at /home/giles/mozilla/firefox/js/src/gc/Heap.h:819
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4922d45 in js::gc::Cell::address (this=0x7fff7800ada2)
    at /home/giles/mozilla/firefox/js/src/gc/Heap.h:819
819	    JS_ASSERT(addr % CellSize == 0);
Missing separate debuginfos, use: debuginfo-install alsa-plugins-pulseaudio-1.0.26-1.fc17.x86_64 gsm-1.0.13-6.fc17.x86_64 json-c-0.9-4.fc17.x86_64
(gdb) bt
#0  0x00007ffff4922d45 in js::gc::Cell::address (this=0x7fff7800ada2)
    at /home/giles/mozilla/firefox/js/src/gc/Heap.h:819
#1  0x00007ffff4922daa in js::gc::Cell::arenaHeader (this=0x7fff7800ada2)
    at /home/giles/mozilla/firefox/js/src/gc/Heap.h:935
#2  0x00007ffff4924c1c in js::gc::Cell::compartment (this=0x7fff7800ada2)
    at /home/giles/mozilla/firefox/js/src/gc/Heap.h:980
#3  0x00007ffff4c6a8dd in CheckMarkedThing<JSString> (trc=0x7fffffff5050, 
    thing=0x7fff7800ada2)
    at /home/giles/mozilla/firefox/js/src/gc/Marking.cpp:106
#4  0x00007ffff4c68da8 in MarkInternal<JSString> (trc=0x7fffffff5050, thingp=
    0x7fffcaa17268) at /home/giles/mozilla/firefox/js/src/gc/Marking.cpp:145
#5  0x00007ffff4c68365 in MarkUnbarriered<JSString> (trc=0x7fffffff5050, 
    thingp=0x7fffcaa17268, name=0x7ffff56e0ce7 "left child")
    at /home/giles/mozilla/firefox/js/src/gc/Marking.cpp:175
#6  0x00007ffff4c62b19 in js::gc::MarkStringUnbarriered (trc=0x7fffffff5050, 
    thingp=0x7fffcaa17268, name=0x7ffff56e0ce7 "left child")
    at /home/giles/mozilla/firefox/js/src/gc/Marking.cpp:317
...

Comment 13

[Ralph Giles (:rillian)]

Stack pointer seems to be getting corrupted somehow. When it crashes in the opus code, it's pretty commonly here:

614	   ptr = (char*)st + align(sizeof(OpusMSDecoder));
   0x00007ffff43c68cd <+111>:	mov    $0x10c,%edi
=> 0x00007ffff43c68d2 <+116>:	callq  0x7ffff43c53e8 <align>

and in some instances rsp points to unmapped memory, so the callq segfaults trying to push rip.

Comment 14

[Timothy B. Terriberry (:derf)]

Attachment: Don't try to decode Opus packets larger than 120 ms

(In reply to Ralph Giles (:rillian) from comment #13)
> Stack pointer seems to be getting corrupted somehow. When it crashes in the
> opus code, it's pretty commonly here:

Okay, that was enough to start making some guesses. media/libopus/src/opus_multistream.c:613 reads
  ALLOC(buf, 2*frame_size, opus_val16)

frame_size is documented to have a maximum of 63*2880 in nsOggReader::DecodeOpus() (and this documentation is correct). 2*63*2880*sizeof(opus_val16) (which in this case is float, since we're not using a fixed-point build) is 1,451,520 bytes, or 1.38 MB. Our media thread stacks are 128 kB. So that's a problem.

This is obviously ridiculous: 63*2880 is 3.78 seconds of audio, and the maximum valid Opus packet size is 120 ms (giving a buffer size of 2*5760*4==45 kB here). It will get rejected by libopus later, but the damage to the stack has already been done.

The simple fix is to reject the packet earlier. The best place is before we allocate the buffer for the frame.

Comment 15

[Timothy B. Terriberry (:derf)]

Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=9840e1d7fab9

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 690665 [details] [diff] [review]
Don't try to decode Opus packets larger than 120 ms

Review of attachment 690665 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your help with this, Tim. I'd seen the allocation was large, but didn't think about the thread's stack size being the problem.

Looks good to me, and I can't make it crash with this patch. Some comments below.

::: content/media/ogg/OggReader.cpp
@@ +424,5 @@
>    int32_t samples = opus_packet_get_samples_per_frame(aPacket->packet,
>                                                        (opus_int32) mOpusState->mRate);
>    int32_t frames = frames_number*samples;
>  
> +  // The maximum valid value is 5760 (120 ms).

I would be be more verbose here, e.g.

// Valid Opus packets must be no more than 120 ms,
// so reject any reporting more than 5760 samples.

@@ +425,5 @@
>                                                        (opus_int32) mOpusState->mRate);
>    int32_t frames = frames_number*samples;
>  
> +  // The maximum valid value is 5760 (120 ms).
> +  if (frames <= 0 || frames > 5760)

The <= zero check is an important guard for the subsequent allocation, and detects error returns from opus_packet_get_nb_frames, but I suppose we could tighten this to the minimum as well?

The minimum opus frame size is 2.5 ms, or 120 samples, so we could check for (frames < 120) instead.

Currently opus_packet_get_samples_per_frame() will never return a negative, so checking for frames < 0 is sufficient to detect a failure. But, get_nb_frames only returns an error if the buffer is short, in which case the subsequent get_samples_per_frame call could segfault reading the toc byte for the packetS o really we should check right immediately afterward.

Comment 17

[Ralph Giles (:rillian)]

Christoph, can you please confirm this fixes your original testcase?

Comment 18

[Ralph Giles (:rillian)]

I believe I understand the crash in the opus code now, but I don't understand how this was causing the similar segfaults in js::gc. The jit does fancy things with the stack frames, but I would have thought the context switch in and out of the decode thread would just save and restore the invalid stack pointer without affecting other code.

Is it just that we managed to write into someone else's stack frame with our bogus pointer?

Comment 19

[Ralph Giles (:rillian)]

Validated test case file on Fedora Linux 17 x86_64 with official builds:

firefox-19.0a2: crashed
firefox-18.0b3: crashed
firefox-17.0.1: crashed
firefox-16.0.2: can't reproduce

Which is as expected. The call into the code with the stack overflow was added in ff17.

dveditz, why is firefox-17 wontfix? If we're indeed writing to the ionmonkey stack, couldn't this be exploitable?

Comment 20

[Christoph Diehl [:posidron]]

Have tested again with the applied patch but still crashes.

Comment 21

[Ralph Giles (:rillian)]

(In reply to Christoph Diehl [:cdiehl] from comment #20)
> Have tested again with the applied patch but still crashes.

Well, darn. We should still land Tim's fix though; that's a real bug.

Comment 22

[Christoph Diehl [:posidron]]

I am terrible sorry, I thought doing 'make obj/content/media' was enough.

The patch does fix the problem.

Comment 23

[Ralph Giles (:rillian)]

whew! thanks for retesting.

Comment 24

[Ralph Giles (:rillian)]

Attachment: Reject packets with invalid lengths

Tim, here's a patch implementing my comments from #16. Let me know if you like it better.

Comment 25

[Timothy B. Terriberry (:derf)]

Comment on attachment 691018 [details] [diff] [review]
Reject packets with invalid lengths

Review of attachment 691018 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Ralph Giles (:rillian) from comment #18)
> Is it just that we managed to write into someone else's stack frame with our
> bogus pointer?

That seems the most likely to me.

(In reply to Ralph Giles (:rillian) from comment #19)
> dveditz, why is firefox-17 wontfix? If we're indeed writing to the ionmonkey
> stack, couldn't this be exploitable?

I think he just means that this should land on the ESR branch (once we request and get approval).

Anyway, patch LGTM.

::: content/media/ogg/OggReader.cpp
@@ +427,5 @@
>                                                        (opus_int32) mOpusState->mRate);
>    int32_t frames = frames_number*samples;
>  
> +  // Valid Opus packets must be no shorter than 2.5 ms
> +  // and no longer than 120 ms. Reject any reporting

It seems kind of odd to call out one case when we reject in two different cases.

Comment 26

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #18)
> Currently opus_packet_get_samples_per_frame() will never return a negative,
> so checking for frames < 0 is sufficient to detect a failure. But,
> get_nb_frames only returns an error if the buffer is short, in which case
> the subsequent get_samples_per_frame call could segfault reading the toc
> byte for the packetS o really we should check right immediately afterward.

Good catch, BTW. I've known this in the half-dozen times I implemented this function (including GetOpusDeltaGP() in OggCodecState.cpp!), but managed to miss that here.

Comment 27

[Ralph Giles (:rillian)]

Comment on attachment 690665 [details] [diff] [review]
Don't try to decode Opus packets larger than 120 ms

Marking obsolete in favour of the newer patch.

Comment 28

[Ralph Giles (:rillian)]

(In reply to Timothy B. Terriberry (:derf) from comment #25)

> > +  // Valid Opus packets must be no shorter than 2.5 ms
> > +  // and no longer than 120 ms. Reject any reporting
> 
> It seems kind of odd to call out one case when we reject in two different
> cases.

I don't understand what you mean here. I tried to describe both bounds checks.

Comment 29

[Ralph Giles (:rillian)]

(In reply to Ralph Giles (:rillian) from comment #28)

> I don't understand what you mean here. I tried to describe both bounds
> checks.

Oh, you meant the next line. Sorry. How about:

// Valid Opus packets must be no shorter than 2.5 ms
// and no longer than 120 ms. Reject any reporting
// a duration outside that range in samples.

Or is it clear enough without the second sentence?

Comment 30

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #29)
> Or is it clear enough without the second sentence?

Either way is fine.

Comment 31

[Ralph Giles (:rillian)]

clear try https://tbpl.mozilla.org/?tree=Try&rev=edc54ecea8c1

Comment 32

[Ralph Giles (:rillian)]

Attachment: Reject packets with invalid lengths

Updated patch with improved comments. Carrying forward previous r+.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Before this lands, be sure to ask for sec-approval on the patch and, ideally, I think we'd want branch patches for affected versions depending on the risk involved in taking it.

Comment 34

[Ralph Giles (:rillian)]

Thanks for the reminder.

Comment 35

[Ralph Giles (:rillian)]

Comment on attachment 691399 [details] [diff] [review]
Reject packets with invalid lengths

[Security approval request comment]
> How easily can the security issue be deduced from the patch?

The patch doesn't refer to the stack overflow, it just describes rejecting packets with invalid header data. There's also a reference to a potential--but non-exploitable--heap segfault fix which could mislead an attacker.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

We are limiting the return values we use from a fairly simple function, so someone digging deeper could see what part of the data to play with mutating. I don't think that qualifies as a bulls-eye though; the stack overflow happens away from the site of this change.

> Which older supported branches are affected by this flaw?

Supported branches back to Firefox 17 are affected, when this code initially landed.

> If not all supported branches, which bug introduced the flaw?

Bug 748144.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I can post backports. They're straightforward; the code itself hasn't changed, but the enclosing class and file names have, so the patch must be adapted to that.

> How likely is this patch to cause regressions; how much testing does it need?

Risk of causing other problems is low. The patch simply rejects invalid data earlier in the parsing sequence.

To verify, check that the testcase from this bug does not crash the browser, and that the content/media/test/test_playback.html mochitest still passes.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

I'm giving this sec-approval+ for central. We should prepare branch patches and nominate them. I'm cc'ing release management so they can decide when they want to take this on what branches.

Comment 37

[Ralph Giles (:rillian)]

Comment on attachment 691399 [details] [diff] [review]
Reject packets with invalid lengths

Patch applies unmodified to the Aurora tree.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 748144.

User impact if declined:

Users will be exposed to a security issue: crashes possibly exploitable by malicious web content.

Testing completed (on m-c, etc.):

Passes try push on m-c. Verified patch fixes the crash on aurora.

Risk to taking this patch (and alternatives if risky):

Risk is low. The patch merely rejects invalid data earlier in the parsing sequence.

String or UUID changes made by this patch:

No changes, only interior code is affected.

Comment 38

[Ralph Giles (:rillian)]

Attachment: backport to earlier releases

Backported to Firefox 18 (current beta tree). This is the same code change, just updated for enclosing class and filename changes.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):

Bug 748144.

> User impact if declined: 

Users will be exposed to a security issue: crashes possibly exploitable by malicious web content.

> Testing completed (on m-c, etc.):

Passes try push on m-c. Verified patch fixes the crash on beta.

> Risk to taking this patch (and alternatives if risky):

Risk is low. The patch merely rejects invalid data earlier in the parsing sequence.

> String or UUID changes made by this patch: 

No changes, only interior code is affected.

Comment 39

[bhavana bajaj [:bajaj]]

Approving patch for aurora/beta as it is low risk.Juan, can you please help with some testing here around opus/ogg to verify this patch ?

Comment 40

[Ralph Giles (:rillian)]

Thanks bajaj, Juan.

Comment 41

[Ralph Giles (:rillian)]

Comment on attachment 692455 [details] [diff] [review]
backport to earlier releases

This bug also affects Firefox 17. The ff18 patch applies to the esr17 branch and I've verified it resolves the issue and that the content/media mochitests are unaffected in my local testing.

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is a sec-critical bug.

> User impact if declined:

Users will be exposed to crashes possibly exploitable by malicious web content.

> Fix Landed on Version:

We have approval for m-c, aurora, and beta. Waiting for coordination with release management before landing anywhere.

> Risk to taking this patch (and alternatives if risky):

Risk is low. The patch merely rejects invalid data earlier in the parsing sequence.

> String or UUID changes made by this patch:

No changes, only interior code is affected.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 42

[Alex Keybl [:akeybl]]

Comment on attachment 692455 [details] [diff] [review]
backport to earlier releases

Feel free to land to all branches given your testing. QA will continue to test opus/ogg in parallel to beta 5, but we don't need to block the landing on this testing.

Comment 43

[u279076]

I've spoken with Alex and Ralph about this bug and the feeling is that the risk to uplifting this to ESR is low. My suggestion is that QA coordinates exploratory testing around Opus in Beta5. If we find any issues, we can always backout of ESR and Beta at that time. However that remains a very low risk.

Comment 44

[u279076]

Taking QA Contact so I can coordinate some Opus testing.

Comment 45

[Christoph Diehl [:posidron]]

Btw.
It runs and will run also as a fuzzing job on my private and our future cloud servers.

Comment 46

[Ralph Giles (:rillian)]

Thanks everyone.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/746f31757355

Once this hits m-c, it can be uplifted to the release branches.

Comment 48

[Ralph Giles (:rillian)]

Since the crash isn't reliable I don't think I can make a useful testsuite entry for it. The steps to reproduce in the description are the best way to verify the bug.

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/746f31757355

Comment 50

[u279076]

Looks like this got approval for Beta but didn't land in time. Can we make sure it gets landed for Beta 6?

Comment 51

[Ralph Giles (:rillian)]

I think we were waiting for inbound to cycle into nightly. What's the cut-off date for Beta 6?

Comment 52

[Ralph Giles (:rillian)]

After consultation with bajaj, the plan is to review this for beta6 in tomorrow's triage, and then land or not based on the decision there.

I'll go ahead and push to aurora.

Comment 53

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76f0542c5ee9

Comment 54

[u279076]

Dropping qawanted since there's nothing for us to investigate. Will continue to track for verification.

Comment 55

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3e6bd7f9736

Comment 56

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/fd5e61ad71da

Comment 57

[Ralph Giles (:rillian)]

I...have no idea how I messed up the flags on this bug. I thought I just set status-firefox-esr17 to fixed.

I've put back what I can, but I don't seem to have an option to set the tracking flags for ff18 and ff19 to '+'.

Comment 58

[u279076]

(In reply to Christoph Diehl [:cdiehl] from comment #0)
> Created attachment 687099 [details]
> testcase

Testcase failed to crash the following builds:
 * Firefox Nightly 20.0a1 2012-12-21
 * Firefox Aurora 19.0a2 2012-12-21
 * Firefox 17.0.1esrpre 2012-12-21

Will need to wait for 18.0b6 builds next week to verify for Fx18.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a3e6bd7f9736