Closed Bug 1020219 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
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

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Keywords: csectype-bounds, regression, sec-critical, Whiteboard: [adv-main31+][qa-])

Attachments

(1 obsolete file)

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.
Attached patch patch (obsolete) — — Splinter Review
Attachment #8434038 - Flags: review?(bas)
Attachment #8434038 - Flags: review?(bas) → review+
Group: gfx-core-security
Tagged as a security issue after an IRC chat with Jonathan.
The broken code came from bug 892505.
OK. Therefor ESR 24 is unaffected (this landed in 25).
We can take this in the FF30 release now please nominate for uplift all the way to mozilla-release asap.
Flags: needinfo?(jwatt)
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+
(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.)
Whiteboard: [being fixed in bug 1023336]
Please don't anyone link the two bugs. The idea here is to get the patches in quietly.
Bug 1023336 seems to be fixed in m-c. Do we want an uplift in aurora et beta?
Flags: needinfo?(jwatt)
Patches in bug 1023336 have landed on branches.
Status: NEW → RESOLVED
Closed: 10 years ago
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)?
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)
Landed on both b2g30 and b2g28. See bug 1023336 comment 12 and 13.
Flags: needinfo?(jwatt)
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-]
Group: core-security → core-security-release
Group: core-security-release
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.