812847 - Opus heap-buffer-overflow crash [@opus_multistream_decoder_init]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Timothy B. Terriberry (:derf)]

Attachment: Validate Opus channel count

Comment 3

[Timothy B. Terriberry (:derf)]

Attachment: Validate Opus channel count v2

Oops, forgot to qref.

Comment 4

[Timothy B. Terriberry (:derf)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=45902212727d
https://hg.mozilla.org/mozilla-central/rev/0ef0c9db3b2b

Comment 5

[Timothy B. Terriberry (:derf)]

Attachment: Validate Opus channel count v2 (rebased for m-a and m-b)

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.

Comment 6

[Timothy B. Terriberry (:derf)]

Attachment: Validate Opus channel count v2 (rebased for esr17)

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.

Comment 7

[Timothy B. Terriberry (:derf)]

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.

Comment 8

[Ralph Giles (:rillian)]

Tim has asked me to make make testcases to verify the fix doesn't regress.

Comment 9

[Ralph Giles (:rillian)]

Attachment: test files to verify the new header checks

Work in progress. The files should verify all the new checks added by the patch against ff19. Mochitest wrapper isn't working yet.

Comment 10

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

Comment on attachment 682988 [details] [diff] [review]
Validate Opus channel count v2 (rebased for m-a and m-b)

sec-approval+

Comment 11

[Ralph Giles (:rillian)]

Attachment: Add a mochitest to guard regressions

Regression test is working now, and ready for review.

Comment 12

[Matthew Gregan [: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 13

[Matthew Gregan [:kinetik]]

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.

Comment 14

[Ralph Giles (:rillian)]

Attachment: Add mochitest to guard regressions

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.

Comment 15

[Timothy B. Terriberry (:derf)]

Comment on attachment 683657 [details] [diff] [review]
Add mochitest to guard regressions

https://hg.mozilla.org/integration/mozilla-inbound/rev/18201f6cee4c

Comment 16

[Ralph Giles (:rillian)]

(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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(In reply to Timothy B. Terriberry (:derf) from comment #15)
> Comment on attachment 683657 [details] [diff] [review]
> Add mochitest to guard regressions
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/18201f6cee4c

https://hg.mozilla.org/mozilla-central/rev/18201f6cee4c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0996d275581d
https://hg.mozilla.org/releases/mozilla-esr17/rev/1a9a28682df6

I'm assuming we'll want to uplift the tests at some point too.