Bug 1012609 (CVE-2014-1577)

Out-of-Bounds Read in mozilla::dom::OscillatorNodeEngine::ComputeCustom with negative frequency

VERIFIED FIXED in Firefox 33

Status

()

defect
P1
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: hofusec, Assigned: karlt)

Tracking

(4 keywords)

32 Branch
mozilla35
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ verified, firefox34+ verified, firefox35+ verified, firefox-esr3133+ verified, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main33+][adv-esr31.2+][b2g-adv-main2.2+])

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
Posted file 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.
Reporter

Comment 1

5 years ago
Posted file asan_report.txt
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Assignee

Updated

5 years ago
Priority: -- → P1
Presumably 33 is also affected.
Can you take a look at this, padenot?  Thanks.
Flags: needinfo?(paul)
Posted patch r= (obsolete) — Splinter Review
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.
Attachment #8446568 - Flags: review?(karlt)
Assignee: nobody → paul
Status: NEW → ASSIGNED
Posted patch Test. r=Splinter Review
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).
Attachment #8446569 - Flags: review?(karlt)
Assignee

Comment 6

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

I assume this crash happened immediately and so there is no need for reftest-wait.
Attachment #8446569 - Flags: review?(karlt) → review+
Assignee

Comment 7

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

Probably should reference the author of the testcase.
Assignee

Comment 8

5 years ago
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.
Attachment #8446568 - Flags: review?(karlt) → review-
(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.
Flags: needinfo?(paul)
Posted patch r= (obsolete) — Splinter Review
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).
Attachment #8447201 - Flags: review?(karlt)
Attachment #8446568 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
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.
Attachment #8447201 - Flags: review?(karlt) → review-
Group: media-core-security
Paul, any updates here?  Thanks.
Flags: needinfo?(paul)
Any updates here?  This was filed in May and has sat around unchanged since August.
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
Flags: needinfo?(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.
Flags: needinfo?(mreavy) → needinfo?(karlt)
Assignee

Comment 16

5 years ago
I'll see if I can have a look later this week, but I guess we're too late for 33 anyway.
Flags: needinfo?(karlt)

Comment 17

5 years ago
(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.
Assignee

Updated

5 years ago
Assignee: padenot → karlt
Assignee

Comment 18

5 years ago
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.
Attachment #8497434 - Flags: review?(giles)
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.
Attachment #8497434 - Flags: review?(giles) → review+
Thanks for picking this up, Karl!
Flags: needinfo?(padenot)
Assignee

Comment 21

5 years ago
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.
Attachment #8497434 - Flags: sec-approval?
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.
Attachment #8497434 - Flags: sec-approval? → sec-approval+
Adding Sylvestre per LMandel's request.
Assignee

Comment 25

5 years ago
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
Attachment #8497434 - Flags: approval-mozilla-esr31?
Attachment #8497434 - Flags: approval-mozilla-beta?
Attachment #8497434 - Flags: approval-mozilla-aurora?
Attachment #8497434 - Flags: approval-mozilla-esr31?
Attachment #8497434 - Flags: approval-mozilla-esr31+
Attachment #8497434 - Flags: approval-mozilla-beta?
Attachment #8497434 - Flags: approval-mozilla-beta+
Attachment #8497434 - Flags: approval-mozilla-aurora?
Attachment #8497434 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/fce56d505b2e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee

Updated

5 years ago
Flags: in-testsuite?
Whiteboard: [adv-main33+][adv-esr31.2+]
Alias: CVE-2014-1577
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.
Status: RESOLVED → VERIFIED
Blocks: 865256
Keywords: regression
Group: media-core-security
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main33+][adv-esr31.2+] → [adv-main33+][adv-esr31.2+][b2g-adv-main2.2+]
Assignee

Updated

4 years ago
Attachment #8447201 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Group: core-security
You need to log in before you can comment on or make changes to this bug.