Closed Bug 812847 Opened 7 years ago Closed 7 years ago

Opus heap-buffer-overflow crash [@opus_multistream_decoder_init]

Categories

(Core :: Audio/Video, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox16 --- unaffected
firefox17 --- affected
firefox18 + fixed
firefox19 + fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 18+ fixed

People

(Reporter: posidron, Assigned: derf)

References

Details

(4 keywords, Whiteboard: [adv-main18+][adv-esr17+])

Attachments

(6 files, 3 obsolete files)

Attached audio testcase
media/libopus/src/opus_multistream.c:537

st->layout.nb_channels = channels;


We see the following warning before the crash:

WARNING: Opus end trimming removed more than a full packet.: file /Users/cdiehl/Code/Mozilla/mc-asan-opt/content/media/ogg/OggCodecState.cpp, line 1147

Tested with m-c changeset: 113558:9a6d708faf3f
Attached file callstack
Attached patch Validate Opus channel count (obsolete) — Splinter Review
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
Attachment #682844 - Flags: review?(kinetik)
OS: Mac OS X → All
Hardware: x86_64 → All
Oops, forgot to qref.
Attachment #682844 - Attachment is obsolete: true
Attachment #682844 - Flags: review?(kinetik)
Attachment #682845 - Flags: review?(kinetik)
Attachment #682845 - Flags: review?(kinetik) → review+
Try run: https://tbpl.mozilla.org/?tree=Try&rev=45902212727d
https://hg.mozilla.org/mozilla-central/rev/0ef0c9db3b2b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This patch is identical to the prior one, except that it applies to nsOggCodecState.cpp instead of OggCodecState.cpp, since the former was renamed in bug 811381. Carrying forward r=kinetik.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 748144
User impact if declined: Specially crafted files can trigger a buffer overflow that writes attacker-controlled values into the heap.
Testing completed (on m-c, etc.): Passes try, and now on m-c.
Risk to taking this patch (and alternatives if risky): Low. Only affects Opus file playback, and can only increase the number of files we refuse to play (all of which should be invalid files to begin with).
String or UUID changes made by this patch: None.
Attachment #682988 - Flags: review+
Attachment #682988 - Flags: approval-mozilla-beta?
Attachment #682988 - Flags: approval-mozilla-aurora?
This patch is identical to the aurora/beta one, except that one line of diff context was changed. Carrying forward r=kinetik.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec:crit bug.
User impact if declined: Specially crafted files can trigger a buffer overflow that writes attacker-controlled values into the heap.
Fix Landed on Version: firefox19
Risk to taking this patch (and alternatives if risky): Low. Only affects Opus file playback, and can only increase the number of files we refuse to play (all of which should be invalid files to begin with).
String or UUID changes made by this patch: None.
Attachment #682992 - Flags: review+
Attachment #682992 - Flags: approval-mozilla-esr17?
Comment on attachment 682988 [details] [diff] [review]
Validate Opus channel count v2 (rebased for m-a and m-b)

[Security approval request comment]
How easily can the security issue be deduced from the patch?

That there is a problem can be deduced fairly easily. What the exact problem is requires more knowledge of the code. It is easy to mistake this for an issue with the multichannel downmixing code (which would just be an invalid read from the static const downmixing matrices).

Discovering the actual issue requires knowing that opus_multistream_encoder_get_size() will return 0 when its arguments fail validation, and that jemalloc will successfully return a non-NULL pointer of a 0-byte allocation, which will then pass the NULL check in opus_multistream_encoder_create(), and cause it to try to write into the 0-byte buffer.

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

No.

Which older supported branches are affected by this flaw?

aurora, beta, and esr17.

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

Bug 748114.

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

Yes.

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

Unlikely to cause regressions. It rejects strictly more files than before as invalid, and has been tested not to reject previously valid files. Current automated tests should suffice for the latter.
Attachment #682988 - Flags: sec-approval?
Tim has asked me to make make testcases to verify the fix doesn't regress.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Work in progress. The files should verify all the new checks added by the patch against ff19. Mochitest wrapper isn't working yet.
Comment on attachment 682988 [details] [diff] [review]
Validate Opus channel count v2 (rebased for m-a and m-b)

sec-approval+
Attachment #682988 - Flags: sec-approval? → sec-approval+
Regression test is working now, and ready for review.
Attachment #683375 - Attachment is obsolete: true
Attachment #683467 - Flags: review?(kinetik)
(In reply to Ralph Giles (:rillian) from comment #11)
> Created attachment 683467 [details] [diff] [review]
> Add a mochitest to guard regressions
> 
> Regression test is working now, and ready for review.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c529b92abd52
Comment on attachment 683467 [details] [diff] [review]
Add a mochitest to guard regressions

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

::: content/media/test/test_invalid_reject.html
@@ +17,5 @@
> +  var v = document.createElement('video');
> +  v.token = token;
> +  manager.started(token);
> +
> +  v.name = test.name;

Nit: I don't think v.name and v.token are used anywhere.
Attachment #683467 - Flags: review?(kinetik) → review+
Try push is green, one unrelated orange on Linux, and tests are running, except on arm where we don't run media tests.

Updating patch to address nits, Carrying forward previous r+. Ready to land.
Attachment #683467 - Attachment is obsolete: true
Attachment #683657 - Flags: review+
Flags: in-testsuite+
(In reply to Ralph Giles (:rillian) from comment #14)

> except on arm where we don't run media tests.

I was incorrect here. Ted tells me the mochitest-<n> mechanism divides tests by index, not by directory, and the arm runs use more bins. Many of the media tests are run on arm, there's just part of mochitest-2, which wasn't part of the try push. Fortunately they were green on the inbound push.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Attachment #682988 - Flags: approval-mozilla-beta?
Attachment #682988 - Flags: approval-mozilla-beta+
Attachment #682988 - Flags: approval-mozilla-aurora?
Attachment #682988 - Flags: approval-mozilla-aurora+
Attachment #682992 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Whiteboard: [adv-main18+][adv-esr17+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.