Closed Bug 995075 Opened 6 years ago Closed 5 years ago
OOM large null-offset write in update
_filter() triggered with Audio Buffer Source Node
21.74 KB, patch
|Details | Diff | Splinter Review|
15.29 KB, patch
|Details | Diff | Splinter Review|
+++ 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.
Summary: ASAN: Heap-buffer-overflow in speex_resampler_process_native triggered with AudioBufferSourceNode → OOM large null-offset write in update_filter() triggered with AudioBufferSourceNode
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.
Hi Karl what is the plan for this bug?
(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.
(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.
(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.
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.
Attachment #8469100 - Flags: review?(paul)
Attachment #8405183 - Attachment is obsolete: true
Attachment #8405184 - Attachment is obsolete: true
Attachment #8405185 - Attachment is obsolete: true
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 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.
Attachment #8469100 - Flags: sec-approval?
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.
Attachment #8469100 - Flags: sec-approval? → sec-approval+
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 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 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.
Attachment #8469665 - Flags: approval-mozilla-esr31+
Attachment #8469665 - Flags: approval-mozilla-beta?
Attachment #8469665 - Flags: approval-mozilla-beta+
Attachment #8469665 - Flags: approval-mozilla-aurora?
Attachment #8469665 - Flags: approval-mozilla-aurora+
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.
Sorry Karl, my mistake!
Flags: in-testsuite+ → in-testsuite-
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-.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.