987679 - ASAN: Heap-buffer-overflow in speex_resampler_process_native triggered with AudioBufferSourceNode

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Description

[Holger Fuhrmannek]

Attachment: poc.html

It is possible to trigger a heap buffer overflow with the AudioBufferSourceNode of the WebAudio API. Depending on the parameters to createBuffer and the value of playBackrate the crash occur at different locations and also with firefox 28.0.
The asan report was generated with this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-25-mozilla-central-debug/firefox-31.0a1.en-US.debug-linux-x86_64-asan.tar.bz2.

Comment 1

[Holger Fuhrmannek]

Attachment: asan_report.txt

Comment 3

[Karl Tomlinson (:karlt)]

Changing to sec-critical because it is possible to write to memory beyond the buffer.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: detect array size overflow and recover from larger resampler memory allocation failures

The memory allocations here only become especially large when down sampling
beyond the capabilities of the resampler and/or using a crazy number of
channels.

We should stop doing both of those, but there are few different paths to patch
up, so the first thing I've done is patch the common code, to catch anything that falls through.

I think we should do this on trunk at least, because there is always the risk
of new paths leading to this.  If this patch seems too risky for beta, then
let's see how the other patches work out as an alternative option.

This patch is enough for the resampler to return a null pointer and the test
case then crashes with a safe small offset from null.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: detect array size overflow and recover from larger resampler memory allocation failures v1.1

Self-review noticed I didn't finish recovering from failure in a couple of other allocations.

Comment 6

[Jean-Marc Valin]

Comment on attachment 8397619 [details] [diff] [review]
detect array size overflow and recover from larger resampler memory allocation failures v1.1

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

I think the patch is technically correct. However, I don't think a failure in speex_resampler_set_rate_frac() should just cause the resampler to keep the old sampling rate. It's far too easy to miss the first error and then end up with the wrong amount of data being generated each time. I'm not sure what the exact behaviour should be, but it should make it obvious that there's an error (e.g. failing the resampling, returning zeros, ...). For speex_resampler_set_quality(), not succeeding isn't that big a deal since the output will be pretty similar.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: detect array size overflow and recover from larger resampler memory allocation failures with zero output

(In reply to Jean-Marc Valin (:jmspeex) from comment #6)
> I think the patch is technically correct. However, I don't think a failure
> in speex_resampler_set_rate_frac() should just cause the resampler to keep
> the old sampling rate. It's far too easy to miss the first error and then
> end up with the wrong amount of data being generated each time. I'm not sure
> what the exact behaviour should be, but it should make it obvious that
> there's an error (e.g. failing the resampling, returning zeros, ...).

OK, I could imagine code expecting a certain amount of output.  This version
keeps producing (zero) output at the expected rate.

My first idea was to just set filt_len to zero so as to produce zero output.
However, this would erase the history (old_length/2) in mem, and it couldn't
be recovered on subsequent update_filter calls, even if successful.

> For speex_resampler_set_quality(), not succeeding isn't that big a deal
> since the output will be pretty similar.

It adds complexity to make update_filter behave differently depending on the
caller.  I don't think that is necessary, given we are near disaster here, and it would seem odd to have different behaviour depending on the
order of calls.  This version produces zero output when there is a failure
from either caller.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: detect array size overflow and recover from larger resampler memory allocation failures with zero output v2.1

Switching back the order of allocations makes it simple to only update mem_alloc_size when all allocations have succeeded and the array channels will be appropriately repositioned.

Comment 9

[Jean-Marc Valin]

Comment on attachment 8398405 [details] [diff] [review]
detect array size overflow and recover from larger resampler memory allocation failures with zero output v2.1

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

I don't see how this patch can guarantee that we'll output only zeros regardless of the previous settings of the resampler and in fact I'm not sure that can be done with a simple enough patch. I would suggest a much simpler patch that doesn't refactor all of update_filter() (this code is messy and I haven't touched it in years, so the fewer changes the safer), but instead just sets a st->broken flag when an error is detected. From there, the resampler could simply input/output zero samples (*in_len = *out_len = 0).

Comment 10

[Jean-Marc Valin]

Oh, and as a general comment about the security of this code, you need to keep in mind that I only ever considered the actual samples values to come from an untrusted source. All other values (buffer lengths, settings) have always been assume to be trusted. This explains why I never guarded against things like extreme ratios. So I don't know how the code is used exactly, but if there are other inputs that could be manipulated by an attacker, they're also worth checking.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Jean-Marc Valin (:jmspeex) from comment #9)
> I don't see how this patch can guarantee that we'll output only zeros
> regardless of the previous settings of the resampler

For each output sample, every contributing input sample, including the historic samples, is multiplied by the sinc_table coefficients.  If all the coefficients are zero, then the output will be zero.

When using resampler_basic_interpolate_single with oversample == 0, only 4
coefficients are used and so only 4 need to be set to zero. 

http://mxr.mozilla.org/mozilla-central/source/media/libspeex_resampler/src/resample.c?rev=441506ef7062&mark=490-493#490

Comment 12

[Jean-Marc Valin]

Yeah OK I see how that can work (had missed the part about only needing 4 coeffs). That being said, the code is currently mostly unmaintained, so for now I think the fewer lines changed, the better. So I still think it may be preferable to go with the "*in_len = *out_len = 0" solution since it should be implementable by changing only a handful of lines.

Comment 13

[Karl Tomlinson (:karlt)]

Selecting a recovery mode is slowing progress here, so I'm going to break things up into separate patches to fix the important things, and file new bugs for the other issues.

We can use another bug to sort out the best way to recover in a way that does not leak after realloc failure, retains consistent state, and does not "end up with the wrong amount of data being generated each time".  I need quite a big hand to hold only the lines changed to detect the buffer overflows, so let's start with them.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: use a single common statement to calculate required size per channel in st->mem

to reduce the number of places where overflow checks are required.

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: abort on ovrfl in update_filter

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8405182 [details] [diff] [review]
abort on ovrfl in update_filter

Looking at bug 345118, I'm not sure whether abort is the right function to use
on WINNT, but there seem to be other abort calls in third party libraries
imported into Gecko.  Is there a better XP way to abort in non-Gecko code?

On WINNT, it seems that abort's "Multi-thread version does not raise SIGABRT".
https://bugzilla.mozilla.org/show_bug.cgi?id=345118#c0

I don't know how "abort does not return control to the calling process" and
"If the user chooses Ignore, abort continues normal processing" can be
consistent on http://msdn.microsoft.com/en-us/library/k089yyh0.aspx
This is disturbing, but only for debug builds, I assume?

Comment 17

[Benjamin Smedberg]

You should not use abort(). Please copy the MOZ_REALLY_CRASH logic from http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#180 if you can't use MFBT.

Comment 18

[Jean-Marc Valin]

Attachment: Proposed way to fix the problem (not tested in any way)

Here's a rough minimal patch that I think would solve the problem. I haven't tested it in any way (not even to check if it compiles). In fact, I can't even compile the file because it deviated too much from what I have.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Jean-Marc Valin (:jmspeex) from comment #18)
> Here's a rough minimal patch that I think would solve the problem.

Did you mean to attach this to bug 995075?
Are you demonstrating what you want me to do, or is this a WIP and you would like to take over?

Apart from not fixing this bug:

* This doesn't yet indicate how you want to recover the memory lost in realloc failure.

* It doesn't yet leave mem_alloc_size or filt_len set appropriately for the next update_filter() call.

* It doesn't address comment 6.  It is still just as "easy to miss the first error and then end up with the wrong amount of data being generated each time."  A common pattern is to repeatedly resample until all input has been processed, or until an output buffer is full.  Returning 0 for in-length and out-length will surprise such clients with an infinite loop.  It doesn't take many lines to produce zero output, and there are different options for doing that, but most of the changes are not related to producing zero output;  I'd like to address the security issues first.

* It doesn't yet handle failure in speex_alloc, though the same pattern that you demonstrate can be repeated there too.

Would you prefer repeated expressions / code patterns over sharing code?

I've tried to break up my proposed changes into the simplest concepts.
Would you be willing to look at each of them and say which ones you don't like, or do you dislike all of them?

> In fact, I can't
> even compile the file because it deviated too much from what I have.

I'm not familiar with the issues here.
Has it deviated in a way that you are not happy with?

Comment 20

[Jean-Marc Valin]

(In reply to Karl Tomlinson (:karlt) from comment #19)
> Did you mean to attach this to bug 995075?
> Are you demonstrating what you want me to do, or is this a WIP and you would
> like to take over?

I was only trying to show an approach that I think would be simple and I think it applies to both this bug and bug 995075.

> * This doesn't yet indicate how you want to recover the memory lost in
> realloc failure.

Right, I didn't consider that problem.

> * It doesn't yet leave mem_alloc_size or filt_len set appropriately for the
> next update_filter() call.

Though I didn't write that part, my thought was to simply fail at all subsequent attempts to change params and force a reinit. These are conditions that are never supposed to occur under normal circumstances, so just about any solution that doesn't blow up is probably reasonable. 

> * It doesn't address comment 6.  It is still just as "easy to miss the first
> error and then end up with the wrong amount of data being generated each
> time."  A common pattern is to repeatedly resample until all input has been
> processed, or until an output buffer is full.  Returning 0 for in-length and
> out-length will surprise such clients with an infinite loop.  It doesn't
> take many lines to produce zero output, and there are different options for
> doing that, but most of the changes are not related to producing zero
> output;  I'd like to address the security issues first.

In the end, if the code doesn't check for errors, something bad will happen no matter what. The option of not changing the sampling rate is clearly the worse in terms of weird unexpected results. On the two other options, it's a tradeoff. Outputing zeros causes less breakage in the application, but it may be harder to diagnose (why is this suddenly silent). OTOH, outputing zero samples -- if it causes an infinite loop -- is pretty bad for the application, but it would be pretty obvious and more quickly fixed. I don't have that strong an opinion, if only that outputing zero samples appears simpler.

> Would you prefer repeated expressions / code patterns over sharing code?

Not sure what you mean here.

> I've tried to break up my proposed changes into the simplest concepts.
> Would you be willing to look at each of them and say which ones you don't
> like, or do you dislike all of them?

Sure.

> > In fact, I can't
> > even compile the file because it deviated too much from what I have.
> 
> I'm not familiar with the issues here.
> Has it deviated in a way that you are not happy with?

It's not about being happy or unhappy, but about maintaining the code. This is code I have not modified in about 5 years now. It was part of libspeex, but from the start it was made easy to use outside of libspeex, which many different projects -- like Firefox -- chose to do. The good news is that Speex now finally has a maintainer again (I haven't touched the rest of Speex in ~5 years either), and I thought it would be a good idea to re-unify the different forks of my resampler. If this is going to happen, then I guess minimizing divergence is probably a good idea in the mean time. Another option would be to have the resampler as its own project, but again it would need a maintainer. Is this something you'd be interested in taking on?

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8405182 [details] [diff] [review]
abort on ovrfl in update_filter

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> You should not use abort(). Please copy the MOZ_REALLY_CRASH logic from
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#180 if you
> can't use MFBT.

The MOZ_REALLY_CRASH code isn't directly appropriate for an external library, unless we want to fork the library, because it tests the compiler instead of the system.

If we don't like the way abort() behaves, then we should override it so that it behaves the way we want when called from any library.  I don't know how best to do that on WINNT - one way might be to use a preprocessor macro.

However, it's not worth doing all that for this bug, as we can recover without aborting.  I'll do this differently.

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to Jean-Marc Valin (:jmspeex) from comment #20)
> It's not about being happy or unhappy, but about maintaining the code. This
> is code I have not modified in about 5 years now. It was part of libspeex,
> but from the start it was made easy to use outside of libspeex, which many
> different projects -- like Firefox -- chose to do. The good news is that
> Speex now finally has a maintainer again (I haven't touched the rest of
> Speex in ~5 years either), and I thought it would be a good idea to re-unify
> the different forks of my resampler. If this is going to happen, then I
> guess minimizing divergence is probably a good idea in the mean time.

OK.  I understand the value of minimizing divergence.  I think the problem is
that there are several paths that need to be changed if we want to fix the
issues here, so any approach will modify many lines.  We can still aim to
minimize the number of parts of the code that are changed, but this also needs
to be weighed against the cost of adding modifications that are not changing
the code in the best direction.

I'm hoping that presenting the changes as a series of patches will make it
clearer why the changes are necessary and easier to compare our ideas.  As
Mozilla doesn't like abort(), I'll have to order them differently, but I
expect there will be a way to do this.

> Another option would be to have the resampler as its own project, but again
> it would need a maintainer. Is this something you'd be interested in taking
> on?

I don't think I would be able to take this on, sorry.
If it helps, I'm happy to rebase patches against any repository.

Comment 23

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

Karl, Critsmash triage isn't sure what's going on with this bug and patches. Can you clarify?

Comment 24

[Karl Tomlinson (:karlt)]

This bug needs another round of patches since abort() is not available for reasons that are not completely clear to me (comment 17).

Differing opinions between assignee and reviewer (need to read the whole bug) will likely mean that it will take quite some time to agree on a solution here.

In the meantime, bug 991533 has patches ready to land that prevent all known means to trigger this bug, including the poc.html testcase here.

This bug should still be fixed to protect from new code paths making it exploitable again.

Comment 25

[Andrew McCreight [:mccr8]]

I'm going to change this to sec-other because there's no immediate issue here.  Instead the actual current problem was fixed in bug 991533.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

This won't be fixed for 30 and I am going to untrack it for 31 & 32 (sec-other).

Comment 27

[Kamil Jozwiak [:kjozwiak]]

Quick update for QA:

As mentioned in comment # 24, Bug # 991533 prevents all known means of triggering this issue and was verified in Bug # 991533 Comment # 40.

I quickly re-checked and ensured that Bug # 991533 has stopped this crash. Tested with the latest m-c, aurora and beta ASan builds under linux.

This will be tested once again under this bug once a patch has been created/landed that addresses possible new code paths that could be exploited.

Comment 28

[David Chan [:dchan]]

Clearing bounty flag, we're paying the bounty in bug 991533

Comment 29

[Karl Tomlinson (:karlt)]

Attachment: b= update speex resampler to speexdsp d60e75b2

This fix for this bug has been reviewed and landed upstream, so this patch
just updates our copy.

The bit that hasn't been reviewed previously is the merge of hugemem changes.

Comment 30

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b912d66f27d5

Comment 31

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/mozilla-central/rev/b912d66f27d5

Comment 32

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

(In reply to Karl Tomlinson (:karlt) from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/b912d66f27d5

This was fixed in 34. Please mark the status-firefoxXX flag when you fix it.

Comment 33

[Karl Tomlinson (:karlt)]

(In reply to Al Billings [:abillings] from comment #32)
> Please mark the status-firefoxXX flag when you fix it.

I thought that was what "Target Milestone: --- → mozilla34" was for.
This bug wasn't "tracking-firefox34: +".

Comment 34

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

I have no idea who uses "Target Milestone" but Release Management and I use status-firefoxXX to track which branches things are fixed in (and what releases).

Comment 35

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-25-03-02-01-mozilla-central/
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1395750823/

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-03-03-02-06-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-03-00-40-02-mozilla-aurora/
- Used ASan and regular builds for each branch

Using the above builds, went through the following test cases:

- opened the poc in 10 different tabs/windows without issues
- opened the poc in 10 different private tabs/windows without issues
- opened the poc in 10 different e10s tabs/windows (m-c only)

Comment 36

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

No advisory for this issue because we wrote one for bug 991533.