Closed Bug 1172055 (CVE-2015-7174) Opened 6 years ago Closed 5 years ago

Overflow in nsAttrAndChildArray::GrowBy causes memory-safety bug

Categories

(Core :: DOM: Core & HTML, defect)

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed
firefox-esr38 41+ 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

People

(Reporter: q1, Assigned: baku)

Details

(Keywords: csectype-intoverflow, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 file, 1 obsolete file)

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
Attached 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?
Attached 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: 5 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.