1090405 - Segfault on webm content - "out of memory: 0xFFFFFFFFFFFFFFFF bytes requested"

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ian Stakenvicius]

Attachment: webm: ensure 'samples' is not negative

Gentoo user reports the following WEBM file causes a segfault on linux firefox-33.0, and also on firefox for windows (no version specified).  I have reproduced this segfault on firefox-31.2 (mozilla binary release) as well as firefox-34.0_beta2 built from source.

Crash report ID: 8f0b41bc-3efc-43b4-9b51-fd7f02141028

The following patch seems to address the issue on 34.0b2 and applies clean to 31.2ESR as well.  Bug 1069660 has already addressed the code another way for mozilla-35, but it may be easier to use this patch than to backport the other commits.

Comment 1

[Ralph Giles (:rillian)]

Thanks for the patch! I've reproduced the bug as well.

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 8512839 [details] [diff] [review]
webm: ensure 'samples' is not negative

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

This is much cleaner that backporting the newer code, and lower risk for upload, thanks.

N.B. I think tighter bounds-checks are possible, but this does fix the issue.

::: content/media/webm/WebMReader.cpp
@@ +668,5 @@
>            return true;
>          }
>          int32_t keepFrames = frames - skipFrames;
> +        if (keepFrames < 0) {
> +          NS_WARNING("Int overflow in samples");

"Int overflow in keepFrames"

Comment 3

[Ian Stakenvicius]

Attachment: webm: ensure 'samples' is not negative

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Segfaults on content are unfriendly to end-users

User impact if declined: 
Some webm content will cause 31ESR packages to segfault, all platforms

Fix Landed on Version: 
mozilla35 but with much larger patchset (see above)

Risk to taking this patch (and alternatives if risky): none imo
String or UUID changes made by this patch: none (afaik)

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

Comment 4

[Ralph Giles (:rillian)]

(In reply to Ian Stakenvicius from comment #3)

> Fix Landed on Version: 
> mozilla35 but with much larger patchset (see above)

To clarify, Firefox 35 and later have a different fix. This is just about avoiding the crash in older versions.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8512878 [details] [diff] [review]
webm: ensure 'samples' is not negative

Beta+
ESR31+

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ea93efd4cf0a
https://hg.mozilla.org/releases/mozilla-esr31/rev/f25796531bbb

Comment 7

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

This is being asked about in the security-group as a possible security issue. Is this exploitable in some way? This has no security rating at this point.

Comment 8

[Ralph Giles (:rillian)]

I don't think it's exploitable beyond denial of service. The overflow causes us to allocate a huge buffer. The opus decoder won't write more than usual amount of data into them. Subsequent loops only modify the allocated buffer, they don't copy from another buffer we could read off the end of,

Do you agree, Ian?

Comment 10

[Ian Stakenvicius]

I concurr.  The issue relates to memory allocation itself, rather than overflowing any existing allocated memory.  Even if the malloc call was successful, it would just end up allocating a buffer that is massively too large for what webm would normally need.

Comment 11

[Ralph Giles (:rillian)]

I didn't ask for a security rating because it was filed as a public bug with a patch from an external contributor, and it didn't look exploitable. Sorry if that wasn't the correct procedure. Should I have done something different here?

Comment 12

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

No, that's fine, Ralph. You don't need to mark it as security sensitive unless it seems exploitable or severe. Since you looked at it and didn't think it was exploitable, there was no reason to get a bunch of feedback. This only came up because we were asked about it because it is being discussed on an email list.

Comment 13

[Ralph Giles (:rillian)]

Great, thanks.