WebAudio ASSERTION: Stream did not produce enough data: 'stream->mBuffer.GetEnd() >= GraphTimeToStreamTime(stream, mStateComputedTime)'

RESOLVED FIXED in Firefox 24

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: posidron, Assigned: padenot)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla25
x86_64
Linux
assertion, crash, csectype-uaf, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22 unaffected, firefox23 unaffected, firefox24 fixed, firefox25+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 763189 [details]
testcase

Testcase needs location.reload() to trigger the assertion. This assertion gets called repeatedly and messes with the output of upcoming testcases made by the fuzzer -- this is not caused by the recursion through location.reload() in the testcase.

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/fbcd6734691f

Comment 1

6 years ago
Josh, can you please take a look?
Flags: needinfo?(josh)
(Reporter)

Updated

6 years ago
Group: core-security

Updated

6 years ago
Duplicate of this bug: 883938
(Reporter)

Comment 3

6 years ago
Interestingly it is not crashing with my MacOS build but on Linux it crashes very fast. The stack is different with each crash.
(Assignee)

Comment 4

6 years ago
Comment on attachment 763189 [details]
testcase

Setting as text plain so I don't crash my browser when opening it :-)
Attachment #763189 - Attachment mime type: text/html → text/plain
(Assignee)

Comment 5

6 years ago
So, this is cause by the fact that we use an OfflineAudioContext at 44100Hz. If I modify the test case to use 48000Hz, it works just fine.

In fact, I'm sure sure if it is actually allowed for now, OfflineAudioContext should throw if the rate is not 48000Hz. Maybe this is just a matter of throwing the assertion for now.
Flags: needinfo?(josh) → needinfo?(ehsan)
(Assignee)

Updated

6 years ago
Assignee: nobody → paul

Comment 6

6 years ago
Oh, right.  So we talked about this at one of the Audio WG teleconfs.  We should make the IDL return value for createMediaStreamDestination() return nullable, and return null if called on an OAC with a "non-standard" sampling rate.

Can you please post on public-audio to remind crogers to spec this?  Thanks!
Flags: needinfo?(ehsan)
(Assignee)

Comment 7

6 years ago
Maybe I can needinfo myself as a reminder?
Flags: needinfo?(paul)
status-firefox22: --- → unaffected
status-firefox23: --- → unaffected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox25: --- → +
If this is a spec error does that mean it's not a vulnerability? Or do we need to change our code to protect against this illegal value?(In reply to 

Paul Adenot (:padenot) from comment #4)
> Setting as text plain so I don't crash my browser when opening it :-)

Get the bugzilla Tweaks addon which adds a "source" link next to attachments. That way people can use the attachment as a testcase in place, but you can also easily open it for inspection.  (also the Details link will open HTML safely but you don't get as much real estate to view it.)
(Assignee)

Comment 9

6 years ago
I need to investigate more, there are fishy things here, like use-after-free of messages. Seems to be related to some kind of race because when I put printfs to log, the use-after-free went away.

What we could do is to throw for now when the user specifies a sample rate that we don't support, so it gives us time to investigate. This only buys us time.
Flags: needinfo?(paul)

Comment 10

6 years ago
What does the stack look like here?  This might indeed be specific to OAC.  In that case, we should just throw, and we won't need to worry about this bug beyond that.
(Assignee)

Comment 11

6 years ago
There are a couple different stacks for this testcase, maybe three. I'll post them tomorrow.
needinfoing for the stacks
Flags: needinfo?(paul)
Marking critical per comment 9.
Keywords: csec-uaf, sec-critical
(Assignee)

Comment 14

6 years ago
Created attachment 772059 [details]
stack

For some reason, I can't repro the use-after-free and other ASAN errors, but I get this nice assert, which is kind of expected. I think we should throw instead of all this mess.
Flags: needinfo?(paul)
(Reporter)

Updated

6 years ago
OS: Mac OS X → Linux
(Reporter)

Comment 15

6 years ago
:padenot, based on comment 4 I assumed that you can reproduce the crash.

Please see also comment 3.

Comment 16

6 years ago
(In reply to Paul Adenot (:padenot) from comment #14)
> Created attachment 772059 [details]
> stack
> 
> For some reason, I can't repro the use-after-free and other ASAN errors, but
> I get this nice assert, which is kind of expected. I think we should throw
> instead of all this mess.

Agreed.
(Assignee)

Comment 17

6 years ago
Christoph, I used to be able to repro with at least 3 different stacks, and iirc I have not updated this tree, and I can't repro anymore. Not sure why.
(Assignee)

Comment 18

6 years ago
Created attachment 772084 [details] [diff] [review]
Throw when creating an MediaStreamDestination node on an OfflineAudioContext
Attachment #772084 - Flags: review?(ehsan)

Comment 19

6 years ago
Comment on attachment 772084 [details] [diff] [review]
Throw when creating an MediaStreamDestination node on an OfflineAudioContext

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

Can you please post to public-audio to make sure that we spec this?  Thanks!
Attachment #772084 - Flags: review?(ehsan) → review+
(Assignee)

Comment 20

6 years ago
I was thinking about that again the other day, and I was wondering if using a MediaStreamDestination node on an OfflineAudioContext would make sense with the new MediaRecorder API? In which case we don't want this patch.
Flags: needinfo?(roc)
I think we should take this patch for now and later figure out our story around MediaStreams and OfflineAudioContext. It's not trivial because we'll need to define some kind of offline MediaStream and that will require spec work.
Flags: needinfo?(roc)

Updated

6 years ago
Whiteboard: [blocking-webaudio+][checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a29a65a3cba
status-firefox24: affected → fixed
status-firefox25: affected → fixed
Whiteboard: [blocking-webaudio+][checkin-needed-aurora] → [blocking-webaudio+]
How did this land on Aurora before it was checked into trunk? Bugs need explicit aurora approval from release management before landing there and they need to be checked into trunk first.

Also, this bug affects more than one branch so it should have gotten sec-approval before going in.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 25

6 years ago
(In reply to Al Billings [:abillings] from comment #24)
> How did this land on Aurora before it was checked into trunk? Bugs need
> explicit aurora approval from release management before landing there and
> they need to be checked into trunk first.

Web Audio is being developed pretty much in parallel on trunk and Aurora.  This has been discussed on release-drivers, sorry there are no public archives to link to.

> Also, this bug affects more than one branch so it should have gotten
> sec-approval before going in.
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process

This fix wasn't really a security fix, we just disabled code which used to hit this problem...
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)

> Web Audio is being developed pretty much in parallel on trunk and Aurora. 
> This has been discussed on release-drivers, sorry there are no public
> archives to link to.

I understand this but, still, fixes that aren't branch only don't normally go into branches before they go into trunk and trunk is green. Aurora fixes don't go in without approval from Release Management. From what you say above, I can't tell if this had Aurora approval from them.

> This fix wasn't really a security fix, we just disabled code which used to
> hit this problem...

Well, the bug is sec-critical here and gets tracked all over as such. Does that mean we should open this bug to the public now?
(Reporter)

Updated

6 years ago
Keywords: crash, testcase
https://hg.mozilla.org/mozilla-central/rev/4a86241a8aae
Flags: in-testsuite+
Target Milestone: --- → mozilla25

Comment 28

6 years ago
(In reply to Al Billings [:abillings] from comment #26)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)
> 
> > Web Audio is being developed pretty much in parallel on trunk and Aurora. 
> > This has been discussed on release-drivers, sorry there are no public
> > archives to link to.
> 
> I understand this but, still, fixes that aren't branch only don't normally
> go into branches before they go into trunk and trunk is green.

There are sometimes good reasons to wait (such as when you're not sure if the patch is the right fix, or where you're afraid of regressions, etc.)  In other cases that is just unnecessary formality which slows people down.

> Aurora fixes
> don't go in without approval from Release Management. From what you say
> above, I can't tell if this had Aurora approval from them.

Sorry for the confusion, these patches are pre-approved.

> > This fix wasn't really a security fix, we just disabled code which used to
> > hit this problem...
> 
> Well, the bug is sec-critical here and gets tracked all over as such. Does
> that mean we should open this bug to the public now?

Yes, I don't see why we can't do that.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)
> This fix wasn't really a security fix, we just disabled code which used to
> hit this problem...

... the problem which was almost certainly exploitable (e.g. bug 883938)? Sounds like a "security fix" to me.

But sure, since the fix is now available on every branch that has the feature we don't need to keep the bug hidden.
Group: core-security
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
(Assignee)

Comment 30

6 years ago
This bug had its patch landed on every needed branch, closing.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.