Bug 1172055 (CVE-2015-7174)

Overflow in nsAttrAndChildArray::GrowBy causes memory-safety bug

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: q1, Assigned: baku)

Tracking

({csectype-intoverflow, sec-moderate})

42 Branch
mozilla42
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed, firefox-esr3841+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 attachment, 1 obsolete attachment)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

nsAttrAndChildArray::GrowBy (38.0.1\dom\base\nsAttrAndChildArray.cpp) can overflow if mBufferSize becomes very large, causing a realloc to a tiny buffer size. Other code in the class subsequently writes to unallocated space, potentially permitting the execution of attacker-chosen code, as by modifying an object's virtual-function table:

763: bool
764: nsAttrAndChildArray::GrowBy(uint32_t aGrowSize)
765: {
766:   uint32_t size = mImpl ? mImpl->mBufferSize + NS_IMPL_EXTRA_SIZE : 0;
767:   uint32_t minSize = size + aGrowSize;
768: 
769:   if (minSize <= ATTRCHILD_ARRAY_LINEAR_THRESHOLD) {
770:     do {
771:       size += ATTRCHILD_ARRAY_GROWSIZE;
772:     } while (size < minSize);
773:   }
774:   else {
775:     size = 1u << mozilla::CeilingLog2(minSize);
776:   }
777: 
778:   bool needToInitialize = !mImpl;
779:   Impl* newImpl = static_cast<Impl*>(moz_realloc(mImpl, size * sizeof(void*)));
780:   ...

If size (from mImpl->mBufferSize) is very close to 0xffffffff, line 767 can overflow. If the condition on line 769 is satisfied, size also can overflow. If the condition on line 767 is not satisfied, line 775 computes a tiny size from minSize. In either case, line 779 allocates a buffer far too small to contain all the existing data, and other code in the class writes to the unallocated area.
Flags: sec-bounty?
Component: Untriaged → DOM
Product: Firefox → Core
Posted patch crash6.patch (obsolete) — Splinter Review
Attachment #8624155 - Flags: review?(ehsan)
Assignee: nobody → amarchesini
Attachment #8624155 - Flags: review?(ehsan) → review+
Comment on attachment 8624155 [details] [diff] [review]
crash6.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Quite hard. the string has to be 2^32 bytes.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

yes. we check the size of the growing string.

Which older supported branches are affected by this flaw?

all.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to create backport patches.

How likely is this patch to cause regressions; how much testing does it need?

No regressions.
Attachment #8624155 - Flags: sec-approval?
Comment on attachment 8624155 [details] [diff] [review]
crash6.patch

Clearing sec-approval since this has been rated a sec-moderate and you don't need approval to check those in.
Attachment #8624155 - Flags: sec-approval?
Posted patch 252140.diffSplinter Review
Attachment #8624155 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a9e266811633
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Version: 38 Branch → 42 Branch
Flags: sec-bounty? → sec-bounty+
Please nominate this for Beta/esr38/b2g37 approval.
Flags: needinfo?(amarchesini)
Target Milestone: --- → mozilla42
Comment on attachment 8631875 [details] [diff] [review]
252140.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Quite old feature/code.
User impact if declined: a crash *could* happen.
Testing completed: none
Risk to taking this patch (and alternatives if risky): not too much. We just check the size with CeckedUint32 before allocating.
String or UUID changes made by this patch: none

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fix Landed on Version: 42?
Risk to taking this patch (and alternatives if risky): none 
String or UUID changes made by this patch: none
Flags: needinfo?(amarchesini)
Attachment #8631875 - Flags: approval-mozilla-esr38?
Attachment #8631875 - Flags: approval-mozilla-beta?
Attachment #8631875 - Flags: approval-mozilla-b2g37?
Comment on attachment 8631875 [details] [diff] [review]
252140.diff

Approving for uplift to Beta41 and esr38.3.0. This is a sec fix that has been in Nightly and Aurora for about 6 weeks. It seems safe to uplift.
Attachment #8631875 - Flags: approval-mozilla-esr38?
Attachment #8631875 - Flags: approval-mozilla-esr38+
Attachment #8631875 - Flags: approval-mozilla-beta?
Attachment #8631875 - Flags: approval-mozilla-beta+
Mahe, n-i'ing you on the b2g uplift request. Not sure if you do it or somebody else.
Flags: needinfo?(mpotharaju)
Comment on attachment 8631875 [details] [diff] [review]
252140.diff

Ryan, Please uplift this to b2g-v2.2, b2g-v2.2r, b2g-37
Thanks
Flags: needinfo?(mpotharaju)
Attachment #8631875 - Flags: approval‑mozilla‑b2g37_v2_2r+
Attachment #8631875 - Flags: approval-mozilla-b2g37?
Attachment #8631875 - Flags: approval-mozilla-b2g37+
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Whiteboard: [post-critsmash-triage][adv-main41+] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
Alias: CVE-2015-7174
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.