Closed Bug 1090405 Opened 6 years ago Closed 6 years ago

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


(Core :: Audio/Video, defect)

31 Branch
Not set



Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- unaffected
firefox-esr31 --- fixed


(Reporter: axs, Assigned: axs)




(1 file, 1 obsolete file)

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.
Attachment #8512839 - Flags: review?(giles)
Thanks for the patch! I've reproduced the bug as well.
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"
Attachment #8512839 - Flags: review?(giles) → review+
[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 for more info.
Attachment #8512839 - Attachment is obsolete: true
Attachment #8512878 - Flags: review+
Attachment #8512878 - Flags: approval-mozilla-esr31?
Attachment #8512878 - Flags: approval-mozilla-beta?
(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 on attachment 8512878 [details] [diff] [review]
webm: ensure 'samples' is not negative

Attachment #8512878 - Flags: approval-mozilla-esr31?
Attachment #8512878 - Flags: approval-mozilla-esr31+
Attachment #8512878 - Flags: approval-mozilla-beta?
Attachment #8512878 - Flags: approval-mozilla-beta+
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.
Flags: needinfo?(axs)
Flags: needinfo?(giles)
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?
Flags: needinfo?(giles)
Duplicate of this bug: 1102998
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.
Flags: needinfo?(axs)
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?
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
Great, thanks.
You need to log in before you can comment on or make changes to this bug.