1480374 - a memory region is not cleared completely: memset uses an incorrect third parameter. Bad.

Status: RESOLVED FIXED

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

Description

[ISHIKAWA, Chiaki]

Attachment: onyx_fd-incorrect-size-memset.patch

In a function of libvpx, a memset is called with an incorrect parameter: I believe the intention is to clear the fb->pbi[] array completely.

But due to the incorrect parameter, the array gets cleared only the first 1/4 elements (32-bit) or 1/8 elements (64-bits).
The element of pbi[] array is a pointer.

NOW, the big question is this.
Is failing to clear the array a bad thing?

I think so. It can cause a crash if 0-ness is relied upon for non-used element.

I have not read the source code completely,nor the time at this moment,
but if there is a piece of code that depends on elements of pbi[] to contain 0, i.e., null ptr (on most CPU architectures, including x86, x86_64, etc.), then we have a serious problem.

The second invocation of this data structuremay cause a crash: it seems memset() is called to clear the array when an error occurs and vpx_clear_system_state() is called.

Thus, eventual crash due to referencing a bogus pointer is a possibility.

Excercise to the reader:
Create  a video/audio data stream so that fb->pbi[] contains a bogus pointer
and let the program crash.

(*) Advanced Excersize:
Ditto, above, but furthermore, try placing a pointer that points to sensitive data with TB or FF, and then leave the value of that area in registers before crashing.

Seriously, I noticed this problem when I was compiling TB source using GCC v7 which seems to have a very special memset-related warning: it warns that memset was called to clear memory area (0 in the second parameter), but the third parameter is a constantt\ less than its whole length. Clever (!), erh?
We often use memset(&data, 0, sizeof(data) to clear the data area. So if the third parameter is shorter than the area of the data, it is fishy, indeed.

I am afraid, though, that this means crackers backed up by nation-states and/or big underground syndicates, or those who go to Blackhat conferences probably have been aware of the issue already. Amen.

Yes, I know there is a hurdle to concoct such data, but who knows?
I mean revealing a value of random area is much more difficult, but crashing may be easier.
That is why I set the security flag below.

Comment 1

[Daniel Veditz [:dveditz]]

the original code went out of its way to null out only the initial element, and it's not for speed (the array is only 32 elements). Why? Makes me suspect they only ever check the first element to see if the array is used.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Here's an upstream change in libvpx where this was fixed: https://chromium.googlesource.com/webm/libvpx/+/9c3c1f3725762715c402b63fc852f62e715b491e%5E%21/

Comment 3

[Daniel Veditz [:dveditz]]

Our VPX is well out of date and this appears fixed upstream, though not as a security bug?

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #2)
> Here's an upstream change in libvpx where this was fixed:
> https://chromium.googlesource.com/webm/libvpx/+/
> 9c3c1f3725762715c402b63fc852f62e715b491e%5E%21/

I see. Mozilla source tree ought to reflect that change somehow, then.

Wow, it was fixed like last year? Not neat when you think of the possible security implications.

Simply applying the patch here would be good.
Or better still, the whole libvpx source ought to be replaced (presumably with other security-related changes?).


(In reply to Daniel Veditz [:dveditz] from comment #3)
> Our VPX is well out of date and this appears fixed upstream, though not as a
> security bug?

I have no idea. But this definitely is in the realm of a security bug (crash, ...) in the broad sense.
It depends on the policy of chromium (upstream?) to mention or not to mention the security implications.

But failing to clear a data structure containing pointers is a source of dangled pointer reference and crash.
That is why I mark this bugzilla as security-related.

Oh well. Why aren't wee updating VPX?
It is used for video decoding. 
I wonder if the outdated VPX may be the cause of TB's crashing after an extended use of video playing since early this year, but I digress.

Let us fix the bug one way or the other.

TIA

Comment 5

[Nils Ohlmeier [:drno]]

Dan, this is the motivation why I asked you over in bug 1433158 if you take care of the upgrade to libvpx 1.7.0.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Can we get a rating on this bug? Also, regardless of the 1.7.0 update for trunk, I assume we're going to want to cherry-pick the upstream patch for backports at least.

Comment 7

[Nils Ohlmeier [:drno]]

https://bugs.chromium.org/p/webm/issues/detail?id=1364 and our own old report https://bugzilla.mozilla.org/show_bug.cgi?id=1333752 both seem to agree that this probably not a real security issue.

Dan assuming it's easier to just cherry pick the fix into 63, can you do that first, before we look into updating libvpx?

Comment 8

[ISHIKAWA, Chiaki]

Ouch, I wonder why this was not patched in the days of bug 1333752 (?!).

But anyway, I will trust people's judgement and this is fixed one way or the other.

TIA

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Dan, are you looking at this? We're building the final beta of Fx62 today and the RC on Monday.

Comment 10

[Dan Minor [:dminor]]

Sorry, the bug mail for this came through while I was on PTO / stat holiday and I didn't see it. This has been fixed for 63 by Bug 1433158, but I guess we would want to take the attached patch for 62.

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8996970 [details] [diff] [review]
onyx_fd-incorrect-size-memset.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug in upstream libvpx.
[User impact if declined]:
Potential security issues.
[Is this code covered by automated tests?]:
No.
[Has the fix been verified in Nightly?]:
The update to libvpx 1.7.0 included this fix.
[Needs manual test from QE? If yes, steps to reproduce]: 
No.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
Has already been patched upstream for some time.
[String changes made/needed]:
None.

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 8996970 [details] [diff] [review]
onyx_fd-incorrect-size-memset.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, and this has been fixed upstream for some time.
Which older supported branches are affected by this flaw?
All.
If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The attached patch should apply cleanly.
How likely is this patch to cause regressions; how much testing does it need?
Unlikely, this has been fixed by a separate bug for 63 already.

Comment 13

[Dan Minor [:dminor]]

The attached patch should apply cleanly to beta. Since this was already fixed by an update in 63, I'm not sure which approvals are needed, I'm setting them all just in case.

Comment 14

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

Fixed in 63 in bug 1433158.

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8996970 [details] [diff] [review]
onyx_fd-incorrect-size-memset.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a security bug that has already been fixed upstream.
User impact if declined:
Possibly security problems. 
Fix Landed on Version:
63 (fixed by upstream update)
Risk to taking this patch (and alternatives if risky): 
Not risky.
String or UUID changes made by this patch: 
None.

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

Comment 16

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

dveditz, can you help out with a security rating?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8996970 [details] [diff] [review]
onyx_fd-incorrect-size-memset.patch

This is already on trunk, so it doesn't need sec approval. I'm approving for Beta and ESR60, but won't land it until drno officially rubberstamps it.

Comment 18

[Nils Ohlmeier [:drno]]

Comment on attachment 8996970 [details] [diff] [review]
onyx_fd-incorrect-size-memset.patch

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

LGTM

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b63eb86ff55a

Comment 20

[Daniel Veditz [:dveditz]]

The upstream folks fixed this based on our own reporting in bug 1333752 and a separate bug was split out for this (bug 1338498), got this identical patch, then stalled on process and got lost. It looks like clearing only the initial sentinel element as it was doing is safe enough, though clearing the whole array is better future-proofing. https://bugs.chromium.org/p/webm/issues/detail?id=1364#c2

Doesn't need to be hidden.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9d0543f4cbe7 (FIREFOX_62b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/5d19b7f2173a