1172055 - (CVE-2015-7174) Overflow in nsAttrAndChildArray::GrowBy causes memory-safety bug

Status: RESOLVED FIXED

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

Description

[q1]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: 252140.diff

Comment 5

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e266811633

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a9e266811633

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta/esr38/b2g37 approval.

Comment 8

[Andrea Marchesini [:baku]]

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

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Mahe, n-i'ing you on the b2g uplift request. Not sure if you do it or somebody else.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74ae15bf87b4

Comment 12

[Mahendra Potharaju [:mahe]]

Comment on attachment 8631875 [details] [diff] [review]
252140.diff

Ryan, Please uplift this to b2g-v2.2, b2g-v2.2r, b2g-37
Thanks

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/d4e3f9f61dad

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c5295e651b78
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/c5295e651b78
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/71aa93494a06

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Fix for non-unified bustage. I'll push it to other branches as a ride-along as well.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c03e2bc6a3a4
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/c03e2bc6a3a4
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/d058b8819614

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/705bdd58cee6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32f4ac616613
https://hg.mozilla.org/releases/mozilla-beta/rev/bacde2f7a48b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/705bdd58cee6

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/e50a237d38b6