1443865 - OOB write in libvpx 1.6.1

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

We should uplift this one-line fix from libvpx 1.7.

https://chromium.googlesource.com/webm/libvpx/+/84a7263d4c124919718aca2a7eef1a218216917b%5E%21/#F0

Comment 1

[Ralph Giles (:rillian)]

Attachment: vp9: use 16-bit eobs count

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.

Comment 2

[Ralph Giles (:rillian)]

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[Ralph Giles (:rillian)]

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.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 6

[Ralph Giles (:rillian)]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e9128973a480
https://hg.mozilla.org/releases/mozilla-esr52/rev/ef21d2157756

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8956902 [details] [diff] [review]
vp9: use 16-bit eobs count

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

LGTM

Comment 10

[Ralph Giles (:rillian)]

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.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/a628b2125a1b860c6d8124387040ec9589c219ab
https://hg.mozilla.org/mozilla-central/rev/a628b2125a1b