Fix a bug that can cause DataSourceSurface buffers to be smaller than the DataSourceSurface expects them to be

RESOLVED FIXED in Firefox 31, Firefox OS v1.3

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

({csectype-bounds, regression, sec-critical})

Trunk
mozilla33
csectype-bounds, regression, sec-critical
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ wontfix, firefox31+ fixed, firefox32+ fixed, firefox33+ fixed, firefox-esr24 unaffected, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [adv-main31+][qa-])

Attachments

(1 obsolete attachment)

(Assignee)

Description

4 years ago
Yikes. In SourceSurfaceAlignedRawData::Init and SourceSurfaceAlignedRawData::InitWithStride the temporary that is passed to Realloc may not have enough precision to store the result of |mStride * aSize.height|. That can cause us to have a buffer that is much smaller than the DataSourceSurface expects and needs it to be.
(Assignee)

Comment 1

4 years ago
Created attachment 8434038 [details] [diff] [review]
patch
Attachment #8434038 - Flags: review?(bas)
Attachment #8434038 - Flags: review?(bas) → review+
Group: gfx-core-security
status-firefox29: --- → ?
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox-esr24: --- → ?
tracking-firefox30: --- → +
tracking-firefox31: --- → +
tracking-firefox32: --- → +
tracking-firefox-esr24: --- → ?
Tagged as a security issue after an IRC chat with Jonathan.
(Assignee)

Comment 3

4 years ago
The broken code came from bug 892505.
OK. Therefor ESR 24 is unaffected (this landed in 25).
status-firefox29: ? → wontfix
status-firefox-esr24: ? → unaffected
tracking-firefox-esr24: ? → ---
Keywords: csectype-bounds, sec-critical
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
We can take this in the FF30 release now please nominate for uplift all the way to mozilla-release asap.
Flags: needinfo?(jwatt)
(Assignee)

Comment 6

4 years ago
Comment on attachment 8434038 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 892505
User impact if declined: sg bug
Testing completed (on m-c, etc.): none (simultaneous landing?
Risk to taking this patch (and alternatives if risky): essentially zero risk
String or IDL/UUID changes made by this patch: none

The reason I say this is essentially zero risk is because it only changes the code to use a temporary of a larger type, which is the type of the parameter the function the temporary is being passed to is.
Attachment #8434038 - Flags: approval-mozilla-release?
Attachment #8434038 - Flags: approval-mozilla-beta?
Attachment #8434038 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jwatt)
Blocks: 892505
Keywords: regression
Comment on attachment 8434038 [details] [diff] [review]
patch

sec-approval+ for landing as long as this can land in Firefox 30. Otherwise we should wait a few weeks into the next cycle. I'd be happier without the comment pointing at the overflow risk though I guess the patch would be pretty obvious without it anyway.

a=dveditz for landing on the aurora, beta, and release branches.
Attachment #8434038 - Flags: sec-approval+
Attachment #8434038 - Flags: approval-mozilla-release?
Attachment #8434038 - Flags: approval-mozilla-release+
Attachment #8434038 - Flags: approval-mozilla-beta?
Attachment #8434038 - Flags: approval-mozilla-beta+
Attachment #8434038 - Flags: approval-mozilla-aurora?
Attachment #8434038 - Flags: approval-mozilla-aurora+
Comment on attachment 8434038 [details] [diff] [review]
patch

Removing approvals, we don't think this fixes Win32 builds (our most popular). Since we really need a more thorough fix in the future anyway we should just do that rather than rush an incomplete fix into Firefox 30.
Attachment #8434038 - Flags: sec-approval-
Attachment #8434038 - Flags: sec-approval+
Attachment #8434038 - Flags: approval-mozilla-release-
Attachment #8434038 - Flags: approval-mozilla-release+
Attachment #8434038 - Flags: approval-mozilla-beta-
Attachment #8434038 - Flags: approval-mozilla-beta+
Attachment #8434038 - Flags: approval-mozilla-aurora-
Attachment #8434038 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 9

4 years ago
(In reply to Daniel Veditz [:dveditz] from comment #8)
> we don't think this fixes Win32 builds

(Because size_t is 32-bits there, so AlignedArray::Realloc(size_t aSize) can't be passed a value big enough.)
status-firefox30: affected → wontfix
(Assignee)

Updated

4 years ago
Whiteboard: [being fixed in bug 1023336]
(Assignee)

Comment 10

4 years ago
Please don't anyone link the two bugs. The idea here is to get the patches in quietly.
status-firefox33: --- → affected
tracking-firefox33: --- → +
Bug 1023336 seems to be fixed in m-c. Do we want an uplift in aurora et beta?
Flags: needinfo?(jwatt)
(Assignee)

Comment 12

4 years ago
Patches in bug 1023336 have landed on branches.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox31: affected → fixed
status-firefox32: affected → fixed
status-firefox33: affected → fixed
Flags: needinfo?(jwatt)
Resolution: --- → FIXED
Whiteboard: [being fixed in bug 1023336]
Target Milestone: --- → mozilla33
How should we get this fixed on the B2G release branches, namely b2g28 (v1.3) and b2g30 (v1.4)?
status-b2g-v1.2: affected → wontfix
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: --- → fixed
Flags: needinfo?(jwatt)
(Assignee)

Comment 14

4 years ago
dveditz, what do you think?
Flags: needinfo?(jwatt) → needinfo?(dveditz)
Group: gfx-core-security → core-security
Flags: needinfo?(dveditz)
Can we just land bug 1023336 on those branches? It's quite plausible that we would take a bug to "Avoid jank caused by oversized data" for low memory devices and wouldn't attract attention that way.
Bug 1023336 applied pretty cleanly to b2g30 (just some context differences in the first patch), but it's causing crashtest crashes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=43281427&tree=Mozilla-B2g30-v1.4

b2g28 has more changes that complicate the backport. Jonathan, can you please take a look? :)
Flags: needinfo?(jwatt)
(Assignee)

Comment 17

4 years ago
Landed on both b2g30 and b2g28. See bug 1023336 comment 12 and 13.
Flags: needinfo?(jwatt)
status-b2g-v1.3: affected → fixed
status-b2g-v1.3T: affected → fixed
status-b2g-v1.4: affected → fixed
Whiteboard: [adv-main31+]
Marking [qa-], please feel free to include tests or STR if you'd like this tested. Thank you.
Whiteboard: [adv-main31+] → [adv-main31+][qa-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
(Assignee)

Comment 19

2 years ago
Comment on attachment 8434038 [details] [diff] [review]
patch

This is not what landed in bug 1023336.
Attachment #8434038 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.