Closed Bug 604992 (CVE-2010-4203) Opened 9 years ago Closed 9 years ago

Malformed WebM file leads to crash [@vp8_setup_intra_recon]

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: posidron, Assigned: derf)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: [sg:critical?])

Crash Data

Attachments

(3 files)

Attached file testcase
Values are included inside the testcase.

Please execute the provided html file to reproduce.

Marked as a security bug because of write violation.
Attached file callstack
Attachment #483833 - Attachment mime type: application/octet-stream → application/java-archive
Whiteboard: [sg:critical?]
Roc, can you find the right video guy to address this?
Assignee: nobody → roc
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.
Never mind, I can reproduce it locally now, and it is something else. Debugging...
Attached patch Proposed fixSplinter Review
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.
Assignee: roc → tterribe
Status: NEW → ASSIGNED
Attachment #484506 - Flags: review?(chris)
Comment on attachment 484506 [details] [diff] [review]
Proposed fix

Looks correct. Better see if we can get this upstream.
Attachment #484506 - Flags: review?(chris) → review+
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.
Yes. I can not reproduce it anymore.
blocking2.0: --- → ?
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?
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.
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?
3.6.x does not have WebM (and AFAIK, there are no plans to back-port it there).
Ok, since your stable isn't affected, it would be awesome if you could not draw too much attention to the bug :)
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.
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?], [needs landing]
http://hg.mozilla.org/mozilla-central/rev/cfc1a18e48fc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:critical?], [needs landing] → [sg:critical?]
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
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.
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
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.
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
Okay. I'm marking this as depending on bug 608066 (the libvpx v0.9.5 update) then.
Depends on: 608066
(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?
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.
Yeah, please feel free to unhide, it's been fixed in Chrome for a while now!
Crash Signature: [@vp8_setup_intra_recon]
Blocks: fuzzing-webm
Group: core-security
You need to log in before you can comment on or make changes to this bug.