TempAllocPolicy::pod_* suffer from integer overflow issues

RESOLVED FIXED in Firefox 42

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Waldo, Assigned: jonco)

Tracking

({csectype-intoverflow, sec-high})

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox41 wontfix, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr3842+ fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 affected, b2g-v2.2r affected, b2g-master affected, thunderbird_esr38 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main42+][adv-esr38.4+])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
For example:

    template <typename T>
    T* pod_malloc(size_t numElems) {
        T* p = js_pod_malloc<T>(numElems);
        if (MOZ_UNLIKELY(!p))
            p = static_cast<T*>(onOutOfMemory(AllocFunction::Malloc, numElems * sizeof(T)));
        return p;
    }

js_pod_malloc does just fine with integer overflow, returning nullptr in that case.  But then we call the onOutOfMemory function, passing in a multiplication that may well have overflowed.  And that function ultimately delegates to this:

JS_FRIEND_API(void*)
JSRuntime::onOutOfMemory(AllocFunction allocFunc, size_t nbytes, void* reallocPtr,
                         JSContext* maybecx)
{
    MOZ_ASSERT_IF(allocFunc != AllocFunction::Realloc, !reallocPtr);

    if (isHeapBusy())
        return nullptr;

    if (!oom::IsSimulatedOOMAllocation()) {
        /*
         * Retry when we are done with the background sweeping and have stopped
         * all the allocations and released the empty GC chunks.
         */
        gc.onOutOfMallocMemory();
        void* p;
        switch (allocFunc) {
          case AllocFunction::Malloc:
            p = js_malloc(nbytes);
            break;
          ...
        }
        if (p)
            return p;
    }

So something like this:

    typedef struct { uint32_t arr[1024]; } Bunch;

    TempAllocPolicy talloc(cx);
    Bunch* b = talloc.pod_malloc<Bunch>(2 * 1048576);
    if (!b)
        return false;
    for (size_t i = 0; i < 2 * 1048576; i++)
        b[i].arr[1023] = 17;

would result in JSRuntime::onOutOfMemory being called (on a 32-bit system) with nbytes=0, and so the above loop would write beyond the (0-byte, probably 4-byte under the hood) allocation that backs far, far less than the entirety of |b|.

This is obviously bad, but I don't know if it's used in a place that doesn't have its *own* integer overflow detection.  Vector has its own, and Vector is probably one of the bigger users of this.  But it seems a fool's errand to try to check every caller, so I think we should assume the worst and fix this.

I found this while investigating bug 1207519, for what it's worth.  We probably should audit all the other AllocPolicy implementations and ensure none of them have a similar problem, too, while we're at it.
(Reporter)

Comment 1

4 years ago
The MallocProvider class I *think* doesn't have the problems noted here, but it has some very screwy-reading code where multiplications are performed, then there's reliance on integer overflow checks in other method calls, then yet another integer overflow check in case of failure:

    template <class T>
    T* pod_calloc(size_t numElems) {
        size_t bytes = numElems * sizeof(T);  // overflow
        T* p = js_pod_calloc<T>(numElems); // but this checks for intoverflow, returns null
        if (MOZ_LIKELY(p)) {
            client()->updateMallocCounter(bytes);  // not overflown because not null
            return p;
        }
        if (numElems & mozilla::tl::MulOverflowMask<sizeof(T)>::value) { // intoverflow if null returned
            client()->reportAllocationOverflow();
            return nullptr;
        }
        p = (T*)client()->onOutOfMemory(AllocFunction::Calloc, bytes); // safe, not overflown
        if (p)
            client()->updateMallocCounter(bytes);
        return p;
    }

I think this is safe, but it's very unclearly so, and it could use some clarification -- perhaps when this bug is fixed, as camouflage.
Summary: TempAlloc::pod_* suffer from integer overflow issues → TempAllocPolicy::pod_* suffer from integer overflow issues
(Assignee)

Updated

4 years ago
Assignee: nobody → jcoppeard
(Assignee)

Comment 2

4 years ago
Posted patch temp-alloc-policy-overflow (obsolete) — Splinter Review
Patch to add onOutOfMemoryTyped() method to TempAllocPolicy to check for overflow and call onOutOfMemory() if necessary.

I looked through the other AllocPolicy classes and I didn't find any other instances of this problem.  I did tidy up a couple of details however:
 - made maybe_pod_* call straight to maybe_pod_malloc rather then plain pod_malloc where this itself delegated to maybe_pod_malloc
 - made LifoAlloc::maybe_pod_calloc check for failure before calling memset() regardless of whether the allocation is infallible or not, to catch overflow
 - moved the calculation of bytes to its only use in MallocProvider methods
Attachment #8668526 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

4 years ago
Posted patch abstract-byte-calculations (obsolete) — Splinter Review
To tidy things up some more, here's a patch to abstract the calculation of allocation size into functions which also check for overflow.
Attachment #8668527 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

4 years ago
Attachment #8668526 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 4

4 years ago
Comment on attachment 8668527 [details] [diff] [review]
abstract-byte-calculations

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

Has anyone observed yet that _with_extra is an awful hack?  So fugly and fragile and pfui.

::: js/public/Utility.h
@@ +346,5 @@
> + * Calculate the number of bytes needed to allocate |numElems| contiguous
> + * instances of type |T|.  Return false if the calculation overflowed.
> + */
> +template <typename T>
> +inline bool CalculateAllocSize(size_t numElems, size_t& bytesOut)

size_t*, please, for an outparam.  Also blank line between type and method name.

@@ +358,5 @@
> + * |T| followed by |numExtra| contiguous instances of type |U|.  Return false if
> + * the calculation overflowed.
> + */
> +template <typename T, typename U>
> +inline bool CalculateAllocSizeWithExtra(size_t numExtra, size_t& bytesOut)

Same.

Also, could you make the types T for array-of and Extra for the extra?  With T/U I can't easily tell which is which without looking at this arithmetic, *and* that at call sites.

@@ +412,2 @@
>          return nullptr;
> +    return (T*)js_malloc(bytes);

static_cast<>, C++-style.  And below.

::: js/src/jit/JitAllocPolicy.h
@@ -48,5 @@
>          return p;
>      }
>  
> -    template <size_t ElemSize>
> -    void* allocateArray(size_t n)

Ye gods.  How did you JIT people ever live with yourselves?  ;-)

::: js/src/vm/Runtime.h
@@ +1464,4 @@
>              reportAllocationOverflow();
>              return nullptr;
>          }
> +        return (T*)onOutOfMemoryCanGC(js::AllocFunction::Calloc, bytes);

static_cast<> these too
Attachment #8668527 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 5

4 years ago
Oh, smack MOZ_WARN_UNUSED_RESULT onto the size-computing functions, please.
(Assignee)

Comment 6

4 years ago
Comment on attachment 8668526 [details] [diff] [review]
temp-alloc-policy-overflow

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

Possible with a bit of effort but not trivial. 

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

No.

Which older supported branches are affected by this flaw?

Everything back to FF35.

If not all supported branches, which bug introduced the flaw?

Bug 1033442.

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

This patch should apply or if not it should be a simple merge.

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

Unlikely to cause regressions.
Attachment #8668526 - Flags: sec-approval?
Keywords: sec-high
Comment on attachment 8668526 [details] [diff] [review]
temp-alloc-policy-overflow

Sec-approval+ for trunk. Please nominate for branches, including ESR38 if the patch applies (or add patches and nominate those if it doesn't).
Attachment #8668526 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 8

4 years ago
This is a rolled up patch containing the two previous patches and incorporating review feedback which is I'm going to land.  Carrying r+.
Attachment #8668526 - Attachment is obsolete: true
Attachment #8668527 - Attachment is obsolete: true
Attachment #8670690 - Flags: review+
(Assignee)

Comment 9

4 years ago
Patch for aurora.
(Assignee)

Comment 10

4 years ago
Patch for beta.
(Assignee)

Updated

4 years ago
Attachment #8670712 - Attachment description: bug1208665-temp-alloc-policy → bug1208665-temp-alloc-policy-beta
(Assignee)

Comment 11

4 years ago
Patch for ESR38.
https://hg.mozilla.org/mozilla-central/rev/e2feb3a13f83
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: javascript-core-security → core-security-release
Jon, could you fill the uplift requests? thanks
Flags: needinfo?(jcoppeard)
(Assignee)

Updated

4 years ago
Blocks: 1033442
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 15

4 years ago
Comment on attachment 8670711 [details] [diff] [review]
bug1208665-temp-alloc-policy-aurora

Approval Request Comment
[Feature/regressing bug #]: Bug 1033442.
[User impact if declined]: Potential security vulnerability.
[Describe test coverage new/current, TreeHerder]: On central for two days.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8670711 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

4 years ago
Comment on attachment 8670712 [details] [diff] [review]
bug1208665-temp-alloc-policy-beta

Approval Request Comment
[Feature/regressing bug #]: Bug 1033442.
[User impact if declined]: Potential security vulnerability.
[Describe test coverage new/current, TreeHerder]: On central for two days.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8670712 - Flags: approval-mozilla-beta?
(Assignee)

Comment 17

4 years ago
Comment on attachment 8670730 [details] [diff] [review]
bug1208665-temp-alloc-policy-esr38

[Approval Request Comment]
[User impact if declined]: Potential security vulnerability.
Fix Landed on Version: 44
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8670730 - Flags: approval-mozilla-esr38?
Comment on attachment 8670711 [details] [diff] [review]
bug1208665-temp-alloc-policy-aurora

Taking it in aurora to have some backing time.
Attachment #8670711 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi, this failed to apply:

patching file js/src/vm/MallocProvider.h
Hunk #1 FAILED at 53
Hunk #2 FAILED at 104
Hunk #3 FAILED at 159
3 out of 3 hunks FAILED -- saving rejects to file js/src/vm/MallocProvider.h.rej
patching file js/src/vm/Runtime.h
Hunk #1 FAILED at 1451
1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/Runtime.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug1208665-temp-alloc-policy-aurora.patch
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 20

4 years ago
(In reply to Carsten Book [:Tomcat] from comment #19)
I had landed this myself but forgot to post the link:

https://hg.mozilla.org/releases/mozilla-aurora/rev/71d66bec5570
Flags: needinfo?(jcoppeard)
Is there a reason we haven't taken this on Beta? It is a sec-high security bug.
Flags: needinfo?(sledru)
(Same question applies to ESR38 as well)
Comment on attachment 8670712 [details] [diff] [review]
bug1208665-temp-alloc-policy-beta

Because the patch was pretty big and I wanted to check for potential fall out in aurora.
Flags: needinfo?(sledru)
Attachment #8670712 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8670730 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Hi, this failed for 2.2

merging js/public/Utility.h
warning: conflicts during merge.
merging js/public/Utility.h incomplete! (edit conflicts, then use 'hg resolve --mark')
merging js/src/ds/LifoAlloc.h
warning: conflicts during merge.
merging js/src/ds/LifoAlloc.h incomplete! (edit conflicts, then use 'hg resolve --mark')
merging js/src/jit/FixedList.h
warning: conflicts during merge.
merging js/src/jit/FixedList.h incomplete! (edit conflicts, then use 'hg resolve --mark')
merging js/src/jit/JitAllocPolicy.h
warning: conflicts during merge.
merging js/src/jit/JitAllocPolicy.h incomplete! (edit conflicts, then use 'hg resolve --mark')
merging js/src/jit/LIR.cpp
merging js/src/jit/MIRGenerator.h
merging js/src/jit/MIRGraph.cpp
merging js/src/vm/MallocProvider.h
warning: conflicts during merge.
merging js/src/vm/MallocProvider.h incomplete! (edit conflicts, then use 'hg resolve --mark')
merging js/src/vm/Runtime.h
abort: unresolved conflicts, can't continue

could you take a look, thanks!
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 28

4 years ago
Here's a merged patch.
Flags: needinfo?(jcoppeard)
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+][adv-esr38.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.