995075 - OOM large null-offset write in update_filter() triggered with AudioBufferSourceNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #987679 +++

A memory allocation failure at
http://hg.mozilla.org/mozilla-central/annotate/d41e6e32df81/media/libspeex_resampler/src/resample.c#l698
can cause content-controlled data to be written to a large content-controlled offset from null.

There is eventually a null deref, which will crash the program, but there will be a period of time during which other threads will continue to run compromised.

The allocation request can be large, so it would not be difficult to generate an OOM under similar situations to bug 987679.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: unify alloc/realloc paths in update_filter()

This is to reduce the number of places where we need to check for allocation failure.

The only difference between speex_alloc(size) and speex_realloc(NULL, size)
is that speex_alloc zero initializes the memory, but this code sets every
byte of memory anyway.

It also means that st->sinc_table_length is initialized even on the first
allocation of sinc_table.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: unify remaining mem allocation paths in update_filter()

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: abort on OOM in update_filter()

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Karl what is the plan for this bug?

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to David Bolter [:davidb] from comment #4)
> Hi Karl what is the plan for this bug?

Attachment 8405185 [details] [diff] is not suitable because Benjamin says that Gecko should not use abort() (bug 987679 comment 17).

I haven't received direct feedback on the other attachments here, but IIUC from comments in bug 987679, Jean-Marc doesn't like the divergence from upstream.

So my plan is to submit the refactoring for review upstream in speexdsp, and probably address recovery from OOM there too.

Comment 6

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #5)
> So my plan is to submit the refactoring for review upstream in speexdsp, and
> probably address recovery from OOM there too.

iirc, either derf or jmspeex once told me current upstream for speex_resampler was opus-tools.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #6)
> iirc, either derf or jmspeex once told me current upstream for
> speex_resampler was opus-tools.

That sounds like https://bugzilla.mozilla.org/show_bug.cgi?id=903476#c0

I suspect that might have been because speex had no maintainer at the time.
https://bugzilla.mozilla.org/show_bug.cgi?id=987679#c20 suggests that jmspeex would like to see the versions unified.  speexdsp seems the logical upstream for that.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: update speex resampler to speexdsp 769dc295

This bug has now been reviewed and fixed upstream, so this patch just updates
our copy.

The bit that hasn't been reviewed previously is the merge of hugemem changes,
but that is trivial for this update, apart from the difficulty of reading
diffs of diffs.

I haven't included the next upstream commit, a fix for bug 987679, in this
update, because we should probably fix this bug on branches, but it is not
essential to fix bug 987679 on branches.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: patch for branches 30-33

This doesn't pull in all changes from upstream, but just those those related to update_filter().  It includes changes to update_filter() that landed on m-c in bug 1042504.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8469100 [details] [diff] [review]
update speex resampler to speexdsp 769dc295

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not sure.  An exploit must generate OOM.  It also needs to know which content controlled features can be used to make filt_len large here.

(In reply to Karl Tomlinson (:karlt) from comment #0)
> can cause content-controlled data to be written to a large
> content-controlled offset from null.

That not quite accurate.  The offset of the read is content-controlled.  The data itself may be harder to control.

Bug 991533 has reduced the maximum for nb_channels to 32 when used with Web Audio, which is where maximum control of filt_len is available.

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

The security problem is fixed directly by the patch.  There are some other changes to spam the issue a bit, but it can be seen if the whole patch is studied.

Which older supported branches are affected by this flaw? all.

If not all supported branches, which bug introduced the flaw?
An early web audio bug.  Don't know which.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I have a backport.  It is different in that it only changes affected code, but makes that code the same as what m-c will be and upstream is. 

How likely is this patch to cause regressions; how much testing does it need?
The changes are reasonably complex, even if localized, so there is potential for bugs.  Probably the most complex changes landed July 24, and so have had 2 weeks of testing.  There are automated tests for the common cases.  Corner use case are tested manually using the testcase in bug 964376.

Comment 11

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

Comment on attachment 8469100 [details] [diff] [review]
update speex resampler to speexdsp 769dc295

sec-approval+ for trunk. We should take this on Aurora, Beta, and ESR31 too. If it applied clean to ESR24, I'd suggest it but I can take it or leave it there.

Comment 12

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b9c248cbbd

(In reply to Al Billings [:abillings] from comment #11)
> If it applied clean to ESR24

Oh.  I forgot about that.

ESR24 is unaffected.  Web Audio was not enabled until 25 (bug 885505).

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/25b9c248cbbd

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8469665 [details] [diff] [review]
patch for branches 30-33

Approval Request Comment
[Feature/regressing bug #]: Web Audio.  (bug exists since first shipped - Gecko 25)
[User impact if declined]: sec-high
[Describe test coverage new/current, TBPL]:
There are automated tests for the common cases.  Some other use case are tested manually using the testcase in bug 964376.
[Risks and why]: 
The changes are reasonably complex, even if localized, so there is potential for bugs.  Probably the most complex changes landed July 24, and so have had 2 weeks of in-use testing.
[String/UUID change made/needed]: none.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8469665 [details] [diff] [review]
patch for branches 30-33

[Approval Request Comment]
https://wiki.mozilla.org/Release_Management/ESR_Landing_Process says "the tracking flag will be set to the first version of mainline Firefox that the patch is present in and the approval-mozilla-esr24 flag will be set to +."

It's a bit tricky to pick the "first version" before we have this landed on Beta, but I know that I'll be asked to request approval once/if this has landed on beta, so doing that now to reduce the number of round trips required.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/fc550f53b28a
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/cf15e3d04c41
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/0e67f9d389e9

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffcc0f0992dd
https://hg.mozilla.org/releases/mozilla-beta/rev/92c3567e5a0c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/92c3567e5a0c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/057566aa9e77

Comment 20

[Karl Tomlinson (:karlt)]

The testcase keyword was accidentally left over from cloning Bug 987679.

What does in-testsuite+ mean here?

Did you mean in-testsuite-?

I didn't write a testcase for this because it would be difficult as it would require tuning for each system and browsers available memory so as to generate OOM at the right time.  I don't think our CI automated testsuites are capable of testing out of memory recovery paths.

Bug 964376 has a test, and it is helpful to know that changes here didn't re-introduce that bug, but this bug existed even when that testcase was passing.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Sorry Karl, my mistake!

Comment 22

[Karl Tomlinson (:karlt)]

OK, thanks.

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

I tried the bug file from comment 20 as a sanity check in Fx32 RC and no issues there. However, due to lack of test case that specifically surfaces this bug, marking qe-verify-.