817179 - Potential OOB read on Opus packets with a very large amount of padding

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

See the original report on the mailing list: <http://lists.xiph.org/pipermail/opus/2012-November/001834.html>, as well as the upstream commit: <https://git.xiph.org/?p=opus.git;a=commit;h=9345aaa5>.

This should be nothing more than a simple DoS, if we can even manage to trigger a crash at all (thanks to allocation size rounding). No test case yet.

Comment 1

[Ralph Giles (:rillian)]

Attachment: backport fix from upstream

Comment 2

[Ralph Giles (:rillian)]

Attachment: backport fix from upstream

Updated patch to include the padding.patch. Sorry, forgot to add it to version control.

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 687324 [details] [diff] [review]
backport fix from upstream

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

Thank you! r=me

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae83dcf774e

Should this have a test?

Comment 5

[Timothy B. Terriberry (:derf)]

If rillian wants to do the work, I won't object, but I think we should skip the test. We haven't actually found a testcase that will cause a real crash, and even if we did, the file would have to be somewhere around 16 MB. This bug is also specific enough it's also unlikely to test anything except whether or not this patch has been reverted.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ae83dcf774e

Comment 7

[Ralph Giles (:rillian)]

I generally agree with Tim. I don't expect I'll get to making a useful testfile for this.

We could do it in less repo space with a C unittest, but there's already one in the upstream library code; we'd be better off just running those.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 687324 [details] [diff] [review]
backport fix from upstream

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):

Bug 674225
 
User impact if declined:

It's probably possible for malicious content to crash firefox with a specially constructed, very large audio file.

Testing completed (on m-c, etc.):

Fix landed on m-c several weeks ago with no problem reports. Fix is also in an upstream release of the affected library.

Risk to taking this patch (and alternatives if risky):

Risk is low. I only affects an edge case where tens of megabytes of padding are added to compressed files. This shouldn't affect normal content handling at all.

String or UUID changes made by this patch:

None

Comment 9

[Ralph Giles (:rillian)]

Attachment: backport fix to esr17

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is not a security issue, but I believe it's a possible crash issue. I think we should provide the same robustness in our extended support release whenever possible.

> User impact if declined:

Users could experience crashes from malicious web content.

> Fix Landed on Version:

Firefox 20

> Risk to taking this patch (and alternatives if risky):

The risk is low. It's a small change to code not used by most files. The fix has been in nightly for some weeks with no problem reports, and is part of an upstream release of the affected library code.

The alternative is to leave the bug in.

> String or UUID changes made by this patch:

None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[bhavana bajaj [:bajaj]]

Comment on attachment 687324 [details] [diff] [review]
backport fix from upstream

low risk enough to land on aurora.
Considering where we are closer to last beta, not approving for FF18 beta.

Comment 11

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd847c486519