1426988 - UAF crash in libvpx 1.6.1

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

When trying to start a screen share in Google Hangouts with Firefox shows a pretty clear UAF on ASAN builds.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: vpx_patch.txt

This patch appears to fix the problem on ASAN builds.

Comment 2

[Nils Ohlmeier [:drno]]

What is the best way to report and land this patch upstream in the libvpx project without disclosing it right away?

Comment 3

[johannkoenig]

Our security related bug template is https://bugs.chromium.org/p/webm/issues/entry?template=Security

Using that template will create a bug with restricted visibility.

Instructions for submitting a patch are https://www.webmproject.org/code/contribute/submitting-patches/

The patch will not have restricted visibility. I don't believe we have a process for hiding patches.

Comment 4

[Nils Ohlmeier [:drno]]

Thanks Johann for the quick response.

I created https://bugs.chromium.org/p/webm/issues/detail?id=1482 for tracking purpose.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Nils: can you add me to bug 1482?  You can use either my mozilla.com address or [e-mail] if that helps.  THanks

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

FYI I'm patch author.  It certainly seems to avoid the ASAN failures (under rr and not under rr).

Comment 7

[johannkoenig]

Please add [e-mail] (or [e-mail]) or post the patch in the webm bugtracker if you don't want to upload it to gerrit.

Comment 8

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

Tracking for 58 and 59 since this is sec-high and it is getting to be late in 58 beta.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8938783 [details] [diff] [review]
vpx_patch.txt

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

Ok, the two breaks are replaced by an "early" return still inside the encoder config loop, with the ctx->iface->init call conditionalized to not run when it wouldn't before. So we get cleanup before returning on either code path where a break would have happened.

That's not worse than what we had before, but the code is still confusing, and I think the patch isn't entirely correct. In the case where the dsf check fails, the patch will now call vpx_codec_destroy() when it wouldn't have before, which means a double-free inside ctx->iface for either vp8 or vp9. Unless we're currently leaking it instead further up the call chain; I didn't find any destructors associated with the vectors the ctx are passed from. So bad now or bad later.

Also, the new return doesn't go through the ctx-- after the end of the loop like the old one. I don't understand what that does. The code seems to be treating the case where the mr_get_mem_loc() call fails differently, but I don't understand why that affects where in the ctx stack we are, or why this INVALID_PARAM is different from the other guards higher in the function. Maybe the code is just wrong. In any case it ends up calling SAVE_STATUS with a different ctx pointer. I'm also unclear why it's safe to call this unconditionally since vpx_codec_vp9_cx appears to set it to NULL. Does it get fixed up somewhere? 

Good job writing a minimal change, but based on the above I'm not confident we've addressed the issues here. Please explain or update the patch to address comments. Is there a trace where we can see what code path was taken when the referenced memory was freed?

Thanks for looking at this and sorry for the review delay; was on vacation until today.

Comment 10

[Ralph Giles (:rillian)]

Johann, re #7 I don't see an existing account for marpan. Unfortunately bugzilla doesn't have an invite system, so they can't be added until then register an account themselves. We don't post security-sensitive patches to public trackers until we've landed them, so it will have to be here.

NB libaom has the same code, although it doesn't affect Firefox since we don't currently provide an encoding interface for AV1.

Comment 11

[johannkoenig]

Ralph, the chromium bug tracker is restricted as well and Randell posted a copy of the patch there. I've added you to that bug.

Comment 12

[johannkoenig]

Bug 1426449 is not visible to me. How is it related to this issue?

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

johann - that bug can cause a config to vp8 that fails in the encode initialization (ts_target_bitrates empty in an encoder with temporal layers)

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #9)
> Comment on attachment 8938783 [details] [diff] [review]
> vpx_patch.txt
> 
> Review of attachment 8938783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, the two breaks are replaced by an "early" return still inside the
> encoder config loop, with the ctx->iface->init call conditionalized to not
> run when it wouldn't before. So we get cleanup before returning on either
> code path where a break would have happened.

So, context is everything, and splinter here hid the important bit.  Right after ctx-- at the bottom, at exit from the function, it does SAVE_STATUS(ctx, res).  The old code (which tripped ASAN) would in some cases write to ctx[-1] in the SAVE_STATUS.  If it hit the first break on the first iteration, ctx is still the initial value.  It would then do ctx-- (now pointing to invalid memory -- ctx[-1], and call SAVE_STATUS which would trip ASAN.

If it failed init(), it would destroy the codec for all ctx's that had been inited -- good.  it decrements ctx to where it's now the initial value.  it then breaks, does ctx-- (now pointing to ctx[-1], and does SAVE_STATUS -- boom again.

ctx-- only makes sense in the case of success, and then only because it does ctx++ (etc) right before that at the end of the loop (this is UGLY code; it desperately should be rewritten but I was making a minimal-fix-it change).  (Johann - you can take my patch, but I advise rewriting the function to have a saner loop.)

Also, if the first break happened on an iteration after the first, it would have leaked codecs that had been inited (didn't call vpx_codec_destroy).  Note that we were only hitting a failure in init().  In the first break case, ctx->iface isn't set yet, so vpx_codec_destroy() will just return VPX_CODEC_ERROR.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8938783 [details] [diff] [review]
vpx_patch.txt

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

::: media/libvpx/libvpx/vpx/src/vpx_encoder.c
@@ +112,2 @@
>  
>          if (res) {

Ok, so the final ctx-- just undoes the ctx++ at the end of the loop block. Thanks. I missed that before, and thanks for the extra context.

Looking at the code some more, in the case that the dsf check fails, res will be VPX_CODEC_INVALID_PARAM so we'll enter this block without ctx->iface->init() having been called for the current ctx. I think that *might* avoid the double-free, since vpx_codec_destroy checks for NULL ctx->priv before destroying the interface.

We can't just change this conditional to `res && res != VPX_CODEC_INVALID_PARAM` because ctx->iface->init can return that error. I'm now comfortable with this landing.

Comment 16

[Nils Ohlmeier [:drno]]

Just for awareness: there is still some discussion in the Chromium bug going on if this needs co-ordination with other projects, or if Firefox is the only affected product and we can roll-out on our own schedule.

Comment 17

[Nils Ohlmeier [:drno]]

A patch for this problem has landed in the public libvpx code https://chromium.googlesource.com/webm/libvpx/+/5b6ae020b6a972b67b59b3dbf7cf9cbd3140a80d

We assume that means we can go ahead and land this in Firefox as well.

Comment 18

[Nils Ohlmeier [:drno]]

Comment on attachment 8938783 [details] [diff] [review]
vpx_patch.txt

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 
Since this only triggers with simulcast turned on and not enough bandwidth being available I think it is non trivial.

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?
All branches are affected. But we landed other patches already to prevent the bandwidth problem from getting us into this bug here.

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?
The patch should apply cleanly to all branches as they all use the same version of libvpx.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely as Firefox should no longer be able to get into the situation which triggered this problem in the first place. We manually verified on local ASAN builds that screen sharing in Google Hangouts no longer was triggering this issue with the patch applied. And since it is hard to manually get into the required low bandwidth condition, plus we don't know the exact bandwidth constraints needed, I think this would eat a lot of time from a manual test verification for very little return.

Comment 19

[johannkoenig]

Comment on attachment 8938783 [details] [diff] [review]
vpx_patch.txt

https://chromium.googlesource.com/webm/libvpx/+/9f36419bf21a5922ffc32c289e09dd9fa0eb4eb2
https://chromium.googlesource.com/webm/libvpx/+/81d66e2cc6ca895051f51373b540a842b888df9e

Comment 20

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

Comment on attachment 8938783 [details] [diff] [review]
vpx_patch.txt

sec-approval+

Comment 21

[Nils Ohlmeier [:drno]]

Attachment: bug1426988.patch

This patch combines all the fixes from the libvpx repository upstream into a single commit for easy uplifting.

Comment 22

[Nils Ohlmeier [:drno]]

Comment on attachment 8946078 [details] [diff] [review]
bug1426988.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 
Since this only triggers with simulcast turned on and not enough bandwidth being available I think it is non trivial.

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?
All branches are affected. We landed other patches already in bug 1426449 to prevent the bandwidth problem from getting us into this bug here in Fx 58. But ESR doesn't use the same surrounding/invoking code, thus ESR doesn't have the same level of protection.

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?
The patch should apply cleanly to all branches as they all use the same version of libvpx.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely as Firefox should no longer be able to get into the situation which triggered this problem in the first place. We manually verified on local ASAN builds that screen sharing in Google Hangouts no longer was triggering this issue with the patch applied. And since it is hard to manually get into the required low bandwidth condition, plus we don't know the exact bandwidth constraints needed, I think this would eat a lot of time from a manual test verification for very little return.

Comment 23

[Nils Ohlmeier [:drno]]

Comment on attachment 8946078 [details] [diff] [review]
bug1426988.patch

Approval Request Comment
[Feature/Bug causing the regression]: Looks like libvpx 1.3.0 import in bug 918550 landed this code.
[User impact if declined]: Crashes in certain WebRTC calls.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: We have only manually tried it with ASAN builds. Patch hasn't landed in Nightly, because I would prefer to land the version update to libvpx 1.7.0 in Nightly (bug 1433158) instead.
[Needs manual test from QE? If yes, steps to reproduce]: No, since no this is not a functional change and we only check for a clean exit in an unlikely error condition (which should no longer happen in 58 or 59 since bug 1426449 landed already), plus there isn't anything to verify except Firefox no longer crashing.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Not really.
[Why is the change risky/not risky?]: It only ensures that in an error case the code cleanly exits (so far the exit was not clean) - so no functional change.
[String changes made/needed]: N/A

Comment 24

[Nils Ohlmeier [:drno]]

Since the patch for bug 1426449 made it into 58 I don't think we need to uplift this into release. 

But since bug 1426449 didn't apply to ESR52 I think we should consider this to get into ESR52.

And for 60 I would propose to import libvpx 1.7.0 in bug 1433158 instead of this patch.

Al: what do you think about that proposal?

Comment 25

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

What about 59? I'd like the next ESR52 and mainline releases to be somewhat in synch. If we take 1.7.0 for ESR52.7, I'd like it to be in 59 as well.

Comment 26

[Nils Ohlmeier [:drno]]

Sorry my proposal in comment #24 obviously wasn't as clear as it should have been. Another more structured attempt:

58 - has the patches from bug 1426449 and we will leave it at that

59 - has the patches from bug 1426449 already, but additionally we will also land the latest patch from this bug 1426988 (which combines the security patches from libvpx 1.7.0)

60 - has the patches from bug 1426449 already, but instead of landing the patches from this bug we will land the libvpx update to 1.7.0 over in bug 1433158 - this should avoid merge conflicts when landing bug 1433158

ESR52 - the patches from bug 1426449 don't aplly, uplifting a whole libvpx update into an ESR release so late seem risky, so we only land the patch in this bug 1426988

Comment 27

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

OK, you would need to request uplift to ESR as well.

Comment 28

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

Comment on attachment 8946078 [details] [diff] [review]
bug1426988.patch

Giving sec-approval+ and beta approval as well.

Comment 29

[Nils Ohlmeier [:drno]]

Comment on attachment 8946078 [details] [diff] [review]
bug1426988.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: it is sec-high.

User impact if declined: potential Firefox crashes.

Fix Landed on Version: Not landed yet.

Risk to taking this patch (and alternatives if risky): It is pretty low risk as it only cleans up memory management in case of failing to initialize the video encoder. The scenario under which that happens is pretty rare.

String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/737f7d2ba619

I'd kind of like to land this on trunk too just in case we run into any issues with the libvpx upgrade. Do you foresee any major issues with doing that? I would think that update would just clobber this fix anyway.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=159349748&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/87c23b241c1d

Comment 32

[Nils Ohlmeier [:drno]]

Attachment: bug1426988_with_inc.patch

Sorry about that. I should have known better to compile after modifying the patch.

I just added the missing includes. Carrying forward all approvals.

Going to check if this compiles on ESR52.

Comment 33

[Nils Ohlmeier [:drno]]

Patch doesn't apply to ESR52. Working on a patch for that.

Comment 34

[Nils Ohlmeier [:drno]]

Attachment: bug1426988_for_esr52.patch

The same patch for ESR52

Comment 35

[Nils Ohlmeier [:drno]]

I think you are right we can land this on central and later over-write it with libvpx 1.7.0.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bc73c747e847db4243cf1167720fd69d62fe51
https://hg.mozilla.org/releases/mozilla-beta/rev/42e8f9c29d3b

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/f3bc73c747e8

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8946947 [details] [diff] [review]
bug1426988_for_esr52.patch

Fixes a sec-high, let's get this on ESR52 too.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9993a990e4af