1012609 - (CVE-2014-1577) Out-of-Bounds Read in mozilla::dom::OscillatorNodeEngine::ComputeCustom with negative frequency

Status: VERIFIED FIXED

(Core :: Web Audio, defect, P1)

Description

[Holger Fuhrmannek]

Attachment: testcase.html

It's very similar to Bug 995289 but the source of the bug is now a negative frequency, not a high frequency. I have missed this because i have ignored the crash signature for a while.

Comment 1

[Holger Fuhrmannek]

Attachment: asan_report.txt

Comment 2

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

Presumably 33 is also affected.

Comment 3

[Andrew McCreight [:mccr8]]

Can you take a look at this, padenot?  Thanks.

Comment 4

[Paul Adenot (:padenot)]

Attachment: r=

This makes ComputeCustom work with negative frequency, and add a couple asserts
for extra safety. The issue was that the index where unsigned, so it would
access the array at (uint32_t)-1.

Turning `periodicWaveSize` into a signed integer prevents a warning.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Test. r=

This is the test case, turned into a crashtest. I put it into an other patch so
we can land it separately if needed (depending on the security-rating).

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8446569 [details] [diff] [review]
Test. r=

I assume this crash happened immediately and so there is no need for reftest-wait.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8446569 [details] [diff] [review]
Test. r=

Probably should reference the author of the testcase.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8446568 [details] [diff] [review]
r=

>+      if (mPhase < 0) {
>+        mPhase += periodicWaveSize;
>+      }

When phase is -epsilon, I assume mPhase + periodicWaveSize could round to
periodicWaveSize giving j1 = periodicWaveSize.
Perhaps the += periodicWaveSize can instead be performed on j1 after
sampleInterpolationFactor has been calculated?

>+      // Bilinear interpolation between adjacent samples in each table. If the
>+      // frequency is negative, we iterate on the table backward.
>+      int32_t direction = mFinalFrequency > 0 ? 1 : -1;
>+      int32_t j1 = floor(mPhase);
>+      int32_t j2 = j1 + direction;

>       float sampleInterpolationFactor = mPhase - j1;

When direction is -1, j2 < j1 < mPhase.
I don't understand how the interpolation works or why direction is necessary.

If mPhase is periodicWaveSize - epsilon, then we want
j1 = periodicWaveSize - 1 and j2 = 0, don't we?

The direction in which mPhase is moving doesn't need to change how it is interpreted.

Comment 9

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #8)
> Comment on attachment 8446568 [details] [diff] [review]
> r=
> 
> >+      if (mPhase < 0) {
> >+        mPhase += periodicWaveSize;
> >+      }
> 
> When phase is -epsilon, I assume mPhase + periodicWaveSize could round to
> periodicWaveSize giving j1 = periodicWaveSize.
> Perhaps the += periodicWaveSize can instead be performed on j1 after
> sampleInterpolationFactor has been calculated?

Phase is supposed to go between 0 and periodicWaveSize, so we need to wrap it here. I agree that in theory the epsilon issue can be an issue, but in practice, the numbers here (enforced at dom bindings level) make it safe. I added code to make sure.

> 
> >+      // Bilinear interpolation between adjacent samples in each table. If the
> >+      // frequency is negative, we iterate on the table backward.
> >+      int32_t direction = mFinalFrequency > 0 ? 1 : -1;
> >+      int32_t j1 = floor(mPhase);
> >+      int32_t j2 = j1 + direction;
> 
> >       float sampleInterpolationFactor = mPhase - j1;
> 
> When direction is -1, j2 < j1 < mPhase.
> I don't understand how the interpolation works or why direction is necessary.

This is kind like a wave table, but with separated tables for lower frequency data and higher frequency data. We do a bi-linear interpolation between the samples because we don't have enough values. The semantic of having a negative frequency is to iterate the table backward. When we interpolate, we need to find the next value we are going to interpolate with. When we iterate backward, it is the _previous_ value in the table, modulo periodicWaveSize.

Sampling the first value with a frequency of -1 needs to interpolate between the first value of the table and the last value of the table (but with sampleInterpolationFactor == 0, so in practice this samples the last value of the table). Then, some time after, we will interpolate between the last value of the table and the second-to-last value of the table, etc.

> If mPhase is periodicWaveSize - epsilon, then we want
> j1 = periodicWaveSize - 1 and j2 = 0, don't we?

If mPhase == periodicWaveSize - epsilon, it goes:

    j1 = floor(periodicWaveSize - epsilon)
<=> j1 = periodicWaveSize - 1

    j2 = j1 + (-1) // because we are iterating backward.
<=> j2 = periodicWaveSize - 2

Indeed, we want to interpolate between the last value (that will contribute the most to the final value, because mPhase - j1 is close to 1.0), and the second-to-last value (that will contribute less).

> The direction in which mPhase is moving doesn't need to change how it is
> interpreted.

Not sure what you mean there. We just want to interpolate with the next value, and this depends on the direction in which we are iterating on the table.

Comment 10

[Paul Adenot (:padenot)]

Attachment: r=

This fixes the rounding with epsilon, and makes the phase increment happen after
the value sampling, so we correctly sample the first value (when mPhase == 0).

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8447201 [details] [diff] [review]
r=

>+        mPhase = std::min(periodicWaveSize - FLT_EPSILON, mPhase);

peridicWaveSize is typically > 1 and so periodicWaveSize - FLT_EPSILON ->
periodicWaveSize in float arithmetic.

(In reply to Paul Adenot (:padenot) from comment #9)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #8)
> > >+      if (mPhase < 0) {
> > >+        mPhase += periodicWaveSize;
> > >+      }
> > 
> > When phase is -epsilon, I assume mPhase + periodicWaveSize could round to
> > periodicWaveSize giving j1 = periodicWaveSize.
> > Perhaps the += periodicWaveSize can instead be performed on j1 after
> > sampleInterpolationFactor has been calculated?
> 
> Phase is supposed to go between 0 and periodicWaveSize, so we need to wrap
> it here.

It needs to be wrapped at some point, but what is the problem with doing the
wrapping on j1 instead of mPhase?

> I agree that in theory the epsilon issue can be an issue, but in
> practice, the numbers here (enforced at dom bindings level) make it safe.

mPhase is an accumulator.  The increment periodicWaveSize * mFinalFrequency /
mSource->SampleRate() is likely to have rounding errors, so I don't see how
dom level restrictions on the numbers will prevent mPhase becoming arbitrarily
close to zero.

> > >+      // Bilinear interpolation between adjacent samples in each table. If the
> > >+      // frequency is negative, we iterate on the table backward.
> > >+      int32_t direction = mFinalFrequency > 0 ? 1 : -1;
> > >+      int32_t j1 = floor(mPhase);
> > >+      int32_t j2 = j1 + direction;
> > 
> > >       float sampleInterpolationFactor = mPhase - j1;
> > 
> > When direction is -1, j2 < j1 < mPhase.
> > I don't understand how the interpolation works or why direction is necessary.
> 
> This is kind like a wave table, but with separated tables for lower
> frequency data and higher frequency data. We do a bi-linear interpolation
> between the samples because we don't have enough values. The semantic of
> having a negative frequency is to iterate the table backward. When we
> interpolate, we need to find the next value we are going to interpolate
> with. When we iterate backward, it is the _previous_ value in the table,
> modulo periodicWaveSize.

The comment in the code says interpolation between *adjacent* samples, and
that is how interpolation usually works, but the introduction of a direction
here causes the samples to be both on one side of the desired phase (for one
direction), an adjacent sample and the next sample beyond that.

Whatever the direction, surely we want adjacent samples on each side.

> > If mPhase is periodicWaveSize - epsilon, then we want
> > j1 = periodicWaveSize - 1 and j2 = 0, don't we?
> 
> If mPhase == periodicWaveSize - epsilon, it goes:
> 
>     j1 = floor(periodicWaveSize - epsilon)
> <=> j1 = periodicWaveSize - 1
> 
>     j2 = j1 + (-1) // because we are iterating backward.
> <=> j2 = periodicWaveSize - 2
> 
> Indeed, we want to interpolate between the last value (that will contribute
> the most to the final value, because mPhase - j1 is close to 1.0), and the
> second-to-last value (that will contribute less).

Ah, the existing interpolation seems to have j1 and j2 around the wrong way.

If mPhase - j1 is close to 1.0, then j1 is further from mPhase and so its
sample should contribute less to the interpolated value.  If mPhase - j1 is
close to zero, then j1 is closer to mPhase and so it should contribute more.

The nearest value when mPhase is periodicWaveSize - epsilon should be the
value for periodicWaveSize which is stored at index 0.

> > The direction in which mPhase is moving doesn't need to change how it is
> > interpreted.
> 
> Not sure what you mean there. We just want to interpolate with the next
> value, and this depends on the direction in which we are iterating on the
> table.

If the frequency is -ve then j1 is the next value.
If the frequency is +ve then j2 is the next value.

In each case, we also want to use the previous value, but it is easier to think in terms of adjacent values than next/previous because the concept of adjacent values need not depend on direction.

Comment 12

[Andrew McCreight [:mccr8]]

Paul, any updates here?  Thanks.

Comment 13

[Andrew McCreight [:mccr8]]

Any updates here?  This was filed in May and has sat around unchanged since August.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

NI to mreavy as padenot is on PTO for a while.  We should see if someone can update the patch to respond to the r- from karlt

Comment 15

[Maire Reavy [:mreavy]]

Hi Karl -- Is there any chance you could update padenot's patch for this bug?  And then put it up to Roc for review?   It's the only "sec high" media bug that is still open, and I need to get it resolved.  As Andrew says, it's been open far too long.

Comment 16

[Karl Tomlinson (:karlt)]

I'll see if I can have a look later this week, but I guess we're too late for 33 anyway.

Comment 17

[Robert Kaiser]

(In reply to Karl Tomlinson (:karlt) from comment #16)
> I'll see if I can have a look later this week, but I guess we're too late
> for 33 anyway.

No, we only just pushed b6, so next week we still have b8 and b9, so for a sec-high like this, AFAIK we should be good to take patches up to early Thursday, Oct 2.

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: improve PeriodicWave phase-wrapping logic

This approach takes advantage of the fact that periodicWaveSize is a power of
2 and uses unsigned integer modulo arithmetic to find the appropriate phase
of the period, even when the frequency is negative.

Comment 19

[Ralph Giles (:rillian)]

Comment on attachment 8497434 [details] [diff] [review]
improve PeriodicWave phase-wrapping logic

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

Ah, I see. I avoided doing this originally because the spec doesn't say the input array needs to be a power of two. But periodicWaveSize here just returns the constant 4096 from the underlying implementation, and we zero-pad the coefficients out to that size. So OscillatorNode in fact always sees a power of two.

Thanks for the clean-up.

Comment 20

[Andrew McCreight [:mccr8]]

Thanks for picking this up, Karl!

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8497434 [details] [diff] [review]
improve PeriodicWave phase-wrapping logic

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

A security bug number is enough to indicate a problem here, and it wouldn't take an interested party too long to work out what this was fixing.

The flaw makes possible reads from two 16kB blocks in the lower memory adjacent to allocations of the same size.

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

The checkin comment is chosen so as not to spell out what was going wrong.
The test is not to be landed at this stage.

Which older supported branches are affected by this flaw?

Versions 25 and more recent.

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

Bug 865256.

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

I suggest taking the fixes for bug 1074765 with this, because without that we don't have any automated test case to check correct behavior of the code.
Those patches modify only the same function.

If we prefer not to include fixes for bug 1074765, then a new similar patch could be created easily.

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

The scope of the risk is small, limited to one Web Audio feature, which would have automated testing if bug 1074765 is fixed.

Comment 22

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

Comment on attachment 8497434 [details] [diff] [review]
improve PeriodicWave phase-wrapping logic

sec-approval+.

We'll need this on Aurora, Beta, and ESR31. 

The last beta build is being made tomorrow, Oct 2, so this needs to land ASAP in order to make it. Please make Aurora, Beta, and ESR31 patches and nominate them.

Comment 23

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

Adding Sylvestre per LMandel's request.

Comment 24

[Karl Tomlinson (:karlt)]

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

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8497434 [details] [diff] [review]
improve PeriodicWave phase-wrapping logic

Approval Request Comment

This patch can land as is on other branches if approval is granted for the
patches in bug 1074765.

[Feature/regressing bug #]: bug 865256
[User impact if declined]: sec-high
[Describe test coverage new/current, TBPL]: mochitest from bug 1074765
[Risks and why]:
The scope of the risk is small, limited to one Web Audio feature, which would have automated testing if bug 1074765 is fixed.
[String/UUID change made/needed]: none

Fix Landed on Version: 35

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5943291a9752
https://hg.mozilla.org/releases/mozilla-beta/rev/7a21538bedac
https://hg.mozilla.org/releases/mozilla-esr31/rev/8c431dcec0ff

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/efea9646666d
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/af991b3e4c9c

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/efea9646666d

Comment 30

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

Confirmed crash in Fx32, ASan and release, 2014-08-26.
Verified fixed in Fx33, Fx34 and Fx35, 2014-10-08.
Verified fixed in Fx31.2.0esr, release candidate.

Comment 31

[Karl Tomlinson (:karlt)]

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

Comment 32

[Carsten Book [:Tomcat]]

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