Bug 1178033 (CVE-2015-7220)

Overflow in XDRBuffer::grow can cause memory-safety bug

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: nbp)

Tracking

({sec-moderate})

Trunk
mozilla43
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox41 wontfix, firefox42 wontfix, firefox43 fixed, firefox-esr38 wontfix)

Details

(Whiteboard: [adv-main43+][post-critsmash-triage][adv-main43+] seems untriggerable from content)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

4 years ago
XDRBuffer::grow (js/src/vm/Xdr.cpp, current trunk) can cause an overflow when computing the new buffer size. If this happens, grow allocates a buffer that is much too small, and its callers write far beyond its end:

27: bool
28: XDRBuffer::grow(size_t n)
29: {
30:     MOZ_ASSERT(n > size_t(limit - cursor));
31: 
32:     const size_t MIN_CAPACITY = 8192;
33:     size_t offset = cursor - base;
34:     size_t newCapacity = mozilla::RoundUpPow2(offset + n);
35:     if (newCapacity < MIN_CAPACITY)
36:         newCapacity = MIN_CAPACITY;
37:     if (isUint32Overflow(newCapacity)) {
38:         js::gc::AutoSuppressGC suppressGC(cx());
39:         JS_ReportErrorNumber(cx(), GetErrorMessage, nullptr, JSMSG_TOO_BIG_TO_ENCODE);
40:         return false;
41:     }
42: 
43:     void* data = js_realloc(base, newCapacity);
44:     if (!data) {
45:         ReportOutOfMemory(cx());
46:         return false;
47:     }
48:     base = static_cast<uint8_t*>(data);
49:     cursor = base + offset;
50:     limit = base + newCapacity;
51:     return true;
52: }

The problem occurs on 32-bit platforms only. If offset + n on line 34 exceeds 0x80000001, RoundUpPow2 returns 1 (1 << 32) under Windows 32-bit (note that this is actually undefined per C++11 s.5.8(1)). Lines 35-6 then calculate newCapacity==MIN_CAPACITY (8192). The test on line 37 is useless because newCapacity has already overflowed. Line 43 then allocates the tiny buffer, line 49 computes a cursor well outside the buffer's bounds, and the function returns true. Its caller then uses cursor to write into the buffer.

XDRBuffer appears to be used to serialize compiled javascripts. Is it possible for a web page to contain one or more gigantic javascripts that would invoke this bug?
Assignee

Comment 1

4 years ago
(In reply to q1 from comment #0)
> XDRBuffer appears to be used to serialize compiled javascripts. Is it
> possible for a web page to contain one or more gigantic javascripts that
> would invoke this bug?

AFAIK, XDR is only used for the JS of the browser at the moment, and in the JS Shell.  In the worst case it might be available to Addons, but as we do not have a bytecode cache for the web pages yet, thus this is probably hard to use it as a first vector of attack.

Still, this is a valid bug and we should fix it.
Keywords: sec-moderate
Whiteboard: seems untriggerable from content
Flags: sec-bounty?
Reporter

Comment 3

4 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Created attachment 8632852 [details] [diff] [review]
> XDRBuffer: Replace isUint32Overflow by a simple check of the capacity max.

33:    const size_t MAX_CAPACITY = size_t(INT32_MAX) + 1;
34:    size_t offset = cursor - base;
35:    if (offset + n > MAX_CAPACITY) {

Line 35 could still overflow if n is very large. Is it possible to use CheckedInt here?
Comment on attachment 8632852 [details] [diff] [review]
XDRBuffer: Replace isUint32Overflow by a simple check of the capacity max.

Review of attachment 8632852 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Xdr.cpp
@@ +34,2 @@
>      size_t offset = cursor - base;
> +    if (offset + n > MAX_CAPACITY) {

Unless we have a hard guarantee that n < INT32_MAX, I think we need (MAX_CAPACITY - offset < n) and add MOZ_ASSERT(offset <= MAX_CAPACITY).
Assignee

Comment 5

4 years ago
Oh, thanks for catching this issue.  I guess I was too focus on the
RoundUpPow2 assertion that I did not noticed this one.

Luke, we have inputs where "n" is computed from some inputs, so I think the
second approach is the safest.

About CheckedInt, this sounds like a good idea, but it does not provide an
easy way to set a different upper-bound, thus we will still have to check
against RoundUpPow2 limit case.
Attachment #8632852 - Attachment is obsolete: true
Attachment #8632852 - Flags: review?(luke)
Attachment #8634095 - Flags: review?(luke)

Updated

4 years ago
Attachment #8634095 - Flags: review?(luke) → review+
Comment hidden (off-topic)
Assignee

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b9e72f934f

Hum … the previous links were for an unrelated issue, and I just noticed that I did not push this change.
https://hg.mozilla.org/mozilla-central/rev/e4b9e72f934f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Will you need to request aurora uplift to fix 42?
Flags: needinfo?(nicolas.b.pierron)
Assignee

Comment 11

4 years ago
(In reply to Wes Kocher (:KWierso) from comment #10)
> Will you need to request aurora uplift to fix 42?

I don't think this would be necessary, based on comment 1.
Flags: needinfo?(nicolas.b.pierron)
Flags: sec-bounty? → sec-bounty+
Assignee: nobody → nicolas.b.pierron

Updated

4 years ago
Group: core-security → core-security-release
Whiteboard: seems untriggerable from content → [post-critsmash-triage]seems untriggerable from content
Whiteboard: [post-critsmash-triage]seems untriggerable from content → [post-critsmash-triage][adv-main43+] seems untriggerable from content
Alias: CVE-2015-7220
Whiteboard: [post-critsmash-triage][adv-main43+] seems untriggerable from content → [adv-main43+][post-critsmash-triage][adv-main43+] seems untriggerable from content
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.