604992 - (CVE-2010-4203) Malformed WebM file leads to crash [@vp8_setup_intra_recon]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Values are included inside the testcase.

Please execute the provided html file to reproduce.

Marked as a security bug because of write violation.

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Damon Sicore (:damons)]

Roc, can you find the right video guy to address this?

Comment 3

[Timothy B. Terriberry (:derf)]

I can't reproduce this locally, but my best guess is that

        if (partition + partition_size > user_data_end)

is overflowing in setup_token_decoder() in decodframe.c. Can you try replacing that with

        if (user_data_end - partition < partition_size)

and see if it fixes the problem?

There are a number of other buffer size checks that may face similar overflows if the input buffer is very near (within 16 MB of) the top of the heap.

Comment 4

[Timothy B. Terriberry (:derf)]

Never mind, I can reproduce it locally now, and it is something else. Debugging...

Comment 5

[Timothy B. Terriberry (:derf)]

Attachment: Proposed fix

Okay, the real problem is that they are not decrementing the ref count on a frame buffer when a frame fails to decode properly, and since there are only 4 frame buffers available, they run out on the 5th frame. The attached patch should fix this problem, as well as eliminate the potential partition size check overflow I mentioned earlier.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 484506 [details] [diff] [review]
Proposed fix

Looks correct. Better see if we can get this upstream.

Comment 7

[Timothy B. Terriberry (:derf)]

Yeah, I have pinged them about it, but no response yet. Cristoph, if you could confirm that this fixes the problem for you, I'll mark it checkin-needed.

Comment 8

[Christoph Diehl [:posidron]]

Yes. I can not reproduce it anymore.

Comment 9

[Chris Evans]

Thanks, great bug, affects Chrome (obviously, since Chrome has WebM support...)

When were you looking at shipping the fix? Would you like to co-ordinate or anything like that?

Comment 10

[Timothy B. Terriberry (:derf)]

We've requested blocking2.0? above. If that's approved, it should go in the next nightly, and would be released with Firefox 4.0b7. libvpx is planning a release soon as well (ostensibly targeted for tomorrow, though I've been warned they may miss that date), and I hope this fix is included.

Comment 11

[Chris Evans]

What about disclosure? My guess is that Mozilla won't disclose until 3.6.x is patched? Or does 3.6.x not have WebM yet?

Comment 12

[Timothy B. Terriberry (:derf)]

3.6.x does not have WebM (and AFAIK, there are no plans to back-port it there).

Comment 13

[Chris Evans]

Ok, since your stable isn't affected, it would be awesome if you could not draw too much attention to the bug :)

Comment 14

[Timothy B. Terriberry (:derf)]

Well, the check-in will have this bug #, which won't be accessible to anyone not on the CC list (until this bug is no longer marked [sg:crit], but that will be much later). So someone paying very close attention will be able to figure it out by looking at the diff. Otherwise I don't think there are any plans to draw attention to it.

Comment 15

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/cfc1a18e48fc

Comment 16

[Reed Loden [:reed]]

Ubuntu just patched this in a security update, I think.

http://www.listware.net/201011/full-disclosure/34400-full-disclosure-usn-1015-1-libvpx-vulnerability.html

Comment 17

[Chris Evans]

We found a regression with the fix, due to a subtle integer signedness issue.
The follow-on fix-to-the-fix is here:
https://review.webmproject.org/1098
https://review.webmproject.org/gitweb?p=libvpx.git;a=commit;h=9fb80f7170ec48e23c3c7b477149eeb37081c699

Comment 18

[Timothy B. Terriberry (:derf)]

I'm confused. partition_size in that code is of type ptrdiff_t (I know, because I made it that type), which is signed, unlike what the commit message claims.

The second part of the patch verifies that the user-allocated buffer doesn't wrap around the heap, which is something we already validate during Matroska parsing (i.e., when we allocate a separate buffer for each packet), and so shouldn't affect us.

Comment 19

[John Koleszar]

ptrdiff_t is signed, and the resulting LHS (user_data_end - partition) is signed, but the RHS partition_size is unsigned. The LHS then promoted to unsigned for the comparison. I don't have my copy of the standard handy atm, but there's a decent overview here[1] and you can also try this with gdb:

  (gdb) print (int)-1 < (unsigned int)1
  $1 = 0

[1]: https://www.securecoding.cert.org/confluence/display/seccode/INT02-C.+Understand+integer+conversion+rules

Comment 20

[Timothy B. Terriberry (:derf)]

Let me try this again:

> ptrdiff_t is signed

I agree.

From the code:

>         ptrdiff_t            partition_size;

Yet:

> but the RHS partition_size is unsigned.

All three of these statements cannot be true.

Comment 21

[John Koleszar]

I see what happened.

The code in your initial fix v0.9.2-36-g09bcc1f[1] used unsigned int for the RHS, which later became ptrdiff_t in your warning fix v0.9.2-188-gc4d7e5e. So that commit (c4d7e5e) is also a fix for this issue, and if you're shipping it, then no change necessary. So it looks like this breakage existed in the master branch from v0.9.2-125-g3b9e72b..v0.9.2-188-gc4d7e5e

For background, Chrome was shipping v0.9.2-35-? when this issue was originally reported, so I patched that version to avoid rolling in the other significant changes as part of this bugfix.

[1]: http://review.webmproject.org/#patch,sidebyside,928,1,vp8/decoder/decodframe.c

Comment 22

[Timothy B. Terriberry (:derf)]

Okay. I'm marking this as depending on bug 608066 (the libvpx v0.9.5 update) then.

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> What about disclosure? My guess is that Mozilla won't disclose until 3.6.x is
> patched? Or does 3.6.x not have WebM yet?

Chris: Since our shipping (3.6) version is not affected we don't plan an advisory, but we would like to un-hide the bug at some point to document the change and the testcase. This should be OK on the Google end by now, right?

Comment 24

[Timothy B. Terriberry (:derf)]

The official advisory has been published:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4203

Christoph: It should, in theory, be okay to un-hide this bug now.

Comment 25

[Chris Evans]

Yeah, please feel free to unhide, it's been fixed in Chrome for a while now!