OOB write in libvpx 1.6.1

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

({csectype-bounds, sec-high})

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox-esr5259+ fixed, firefox58 wontfix, firefox59+ fixed, firefox60+ fixed)

Details

(Whiteboard: [adv-main59+][adv-esr52.7+])

Attachments

(1 attachment)

Summary: OOB in libvpx 1.6.1 → OOB write in libvpx 1.6.1
Backport OOB write fix from upstream. the eobs count is an int16_t, but the ssse3 implementation was writing 64 bits to it, corrupting adjacent data.
Attachment #8956902 - Flags: review?(kinetik)
Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

Approval Request Comment
[Feature/Bug causing the regression]: WebRTC
[User impact if declined]: Possible crash from OOB write.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: We don't have a test case. Fix is backported from a newer upstream release.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: I'd characterize the risk as low. 
[Why is the change risky/not risky?]: We don't have a test case to verify, nor much time to test it on nightly. That's not great. However, the fix is a one line correctness change, easy to analyze, and easy to revert if there are issues. It was published in a stable upstream release, and they've had the patch in their tree for three months.
[String changes made/needed]: None
Attachment #8956902 - Flags: approval-mozilla-beta?
Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

We're building 59 RC2 today, so my gut instinct is that this'll need to wait for Fx60/ESR52.8 at this point, but I'll defer to Liz on making the final call.

Also, I assume this is at least sec-high, so it'll need a sec-approval nomination as well.
Flags: needinfo?(giles)
Attachment #8956902 - Flags: approval-mozilla-release?
Attachment #8956902 - Flags: approval-mozilla-esr52?
Attachment #8956902 - Flags: approval-mozilla-beta?
Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The flaw allows writing 48 bits to the stack. It apparently caused crashes in upstream testing, depending on the stack configuration. The value written is under only indirect control of web content, through complex transformations performed by the video encoder. OOB write is bad, but I think this one would be part of a more complex exploit, and not itself a direct path in.

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

I've made the commit message as boring as possible. The corresponding upstream fix mentions 'store', 'crash' and the size mismatch, which could be found by searching their repo for changes in the same area.

Which older supported branches are affected by this flaw?

All.

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

N/A

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

ESR52 requires a path change (the affected source file has moved) but otherwise the fix is identical.

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

Given the evident correctness of the fix, and that it's been upstream for 3 months now, I think we're better off uplifting this to 59 before release without waiting for more testing on 60.

If we see new crashes in the vp9 quantizer, we'd know to revert.
Attachment #8956902 - Flags: sec-approval?
Flags: needinfo?(giles)
If this were last week I wouldn't worry too much, but this is the absolute last minute. We don't have time to land, test, and backout again without delaying the release. 

So I'd rather defer this to either 60 or a dot release (where we would at least have more time to test and react). I would like feedback from dveditz though, especially since this is unrated.
Flags: needinfo?(dveditz)
Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

Nils, feel free to take this review if you can get to it before Matthew.
Attachment #8956902 - Flags: review?(drno)
Attachment #8956902 - Flags: review?(kinetik) → review+
Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

I got IRC approval from Dan to go ahead with taking this. Approved for 59rc2 and ESR 52.7.0.

https://hg.mozilla.org/integration/autoland/rev/a628b2125a1b860c6d8124387040ec9589c219ab
Attachment #8956902 - Flags: review?(drno)
Attachment #8956902 - Flags: approval-mozilla-release?
Attachment #8956902 - Flags: approval-mozilla-release+
Attachment #8956902 - Flags: approval-mozilla-esr52?
Attachment #8956902 - Flags: approval-mozilla-esr52+
Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

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

LGTM
Thanks Matthew, Ryan, Nils, and Liz!

Another point on risk from irc: this code isn't exercised by the common webrtc sites, since Firefox and Chrome both prefer vp8 or h.264. Malicious content is free to select the vp9 encoder, but we're unlikely to see a regression in normal use.
Flags: needinfo?(dveditz)
Attachment #8956902 - Flags: sec-approval? → sec-approval+
Whiteboard: [adv-main59+][adv-esr52.7+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.