1178033 - (CVE-2015-7220) Overflow in XDRBuffer::grow can cause memory-safety bug

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[q1]

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?

Comment 1

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(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.

Comment 2

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: XDRBuffer: Replace isUint32Overflow by a simple check of the capacity max.

Comment 3

[q1]

(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 4

[Luke Wagner [:luke]]

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).

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: XDRBuffer: Replace isUint32Overflow by a simple check of the capacity max.

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.

Comment 6

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c0afcf6ed324

Comment 7

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

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

(Oops, wrong link)

Comment 8

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

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.

Comment 9

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

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

Comment 10

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

Will you need to request aurora uplift to fix 42?

Comment 11

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(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.