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

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: axs, Assigned: axs)

Tracking

31 Branch
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 unaffected, firefox-esr31 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8512839 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8512839 - Flags: review?(giles)
Thanks for the patch! I've reproduced the bug as well.
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → unaffected
status-firefox36: --- → unaffected
status-firefox-esr31: --- → affected
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+
(Assignee)

Comment 3

4 years ago
Created attachment 8512878 [details] [diff] [review]
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.
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

Beta+
ESR31+
Attachment #8512878 - Flags: approval-mozilla-esr31?
Attachment #8512878 - Flags: approval-mozilla-esr31+
Attachment #8512878 - Flags: approval-mozilla-beta?
Attachment #8512878 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/ea93efd4cf0a
https://hg.mozilla.org/releases/mozilla-esr31/rev/f25796531bbb
Assignee: nobody → axs
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox33: affected → wontfix
status-firefox34: affected → fixed
status-firefox35: unaffected → fixed
status-firefox-esr31: affected → fixed
Depends on: 1069660
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
(Assignee)

Comment 10

4 years ago
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.