937132 - LifoAlloc missing overflow checks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Gohman [:sunfish]]

Attachment: ds-overflow-check.patch

js/src/ds/LifoAlloc.h contains generic array allocation code which does sizeof(T) * count without checking for overflow.

I have not audited the codebase to determine if this is exploitable. I was just trying to figure out the intended difference between newArray and newArrayUninitialized (and it's still not clear) and noticed this while I was there.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 830194 [details] [diff] [review]
ds-overflow-check.patch

Nice catch!

Comment 2

[Dan Gohman [:sunfish]]

The bug was introduced in 4d10127fd106 for bug 684039, which appears to be on all release branches.

If exploitable, this could easily be sec-critical, but it's hard to tell if it's exploitable or not. I'm not comfortable assigning a security rating myself.

Comment 3

[Luke Wagner [:luke]]

This seems theoretically exploitable (request large alloc, the small alloc succeeds, you get to stomp over real memory).  OTOH, it seems hard to trigger the large alloc (Lifos are only used for certain things, most of which are tiny).  Given that and that this was discovered internally, it seems reasonable to let it ride the trains.

Comment 4

[Dan Gohman [:sunfish]]

By "ride the trains", do you mean just check it into mozilla-incoming and call it done?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Yes, but note that without a sec-rating, this can't land without approval, FWIW.

Comment 6

[Dan Gohman [:sunfish]]

Ok. According to [0], sec-moderate means that the fix should just be landed without explicit approval. Is that appropriate here?

[0] https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 7

[Dan Gohman [:sunfish]]

Attachment: ds-fixedlist.patch

While we're here, here are a few more instances of the same problem.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8337738 [details] [diff] [review]
ds-fixedlist.patch

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

::: js/src/jit/FixedList.h
@@ +51,5 @@
>          length_ -= num;
>      }
>  
>      bool growBy(size_t num) {
> +        size_t newlength = length_ + num;

Can this overflow also?

Comment 9

[Dan Gohman [:sunfish]]

Attachment: ds-fixedlist.patch

(In reply to Luke Wagner [:luke] from comment #8)
> >      bool growBy(size_t num) {
> > +        size_t newlength = length_ + num;
> 
> Can this overflow also?

Yes, good catch.

Comment 10

[Dan Gohman [:sunfish]]

Attachment: ds-asmjscall.patch

This patch converts a few things in MAsmJSCall to use FixedList. The old code lacked overflow checks, and FixedList is being fixed to do overflow checks.

Comment 11

[Dan Gohman [:sunfish]]

Attachment: ds-mirgenerator-allocate.patch

This patch deletes an unused allocate function which lacked overflow checks, and adds an overflow check to MIRGenerator's allocate function.

Comment 12

[Dan Gohman [:sunfish]]

Attachment: ds-typeset.patch

This patch adds overflow checking to some custom hashtable code in jsinferinlines.h.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8341867 [details] [diff] [review]
ds-asmjscall.patch

Nice

Comment 14

[Dan Gohman [:sunfish]]

Upgrading to sec-critical, because there are several fixes here now, and if any are exploitable, they'd be sec-critical. I'll prepare patches for release branches as desired.

Comment 15

[Dan Gohman [:sunfish]]

Comment on attachment 830194 [details] [diff] [review]
ds-overflow-check.patch

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

It'd be challenging. It would first require finding a way to create an overflow in the affected code. It'd also require crafting an actual exploit around the subsequent memory clobber.

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

They paint a bulls-eye on the code that has the overflow problem, but not on how to actually get it to overflow.

Which older supported branches are affected by this flaw?

Most branches are.

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

Bug 684039.

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

I don't have backports handy, but I can easily create them on request. They ought to be easy and low-risk.

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

It is very unlikely to cause regressions. I've done extensive local testing of it.

Comment 16

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

We're shipping in seven days so this can't go into this cycle. I'm going to give it sec-approval+ for checkin on 12/23 or later.

Comment 17

[Dan Gohman [:sunfish]]

Attachment: ds-overflow-all.patch

Since there's a relatively high per-patch overhead here, I've coalesced the patches above into a single patch. They each have regular review approval, and I'm submitting the combined patch for sec-approval herewith.

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

It'd be challenging. It would first require finding a way to create an overflow in the affected code. It'd also require crafting an actual exploit around the subsequent memory clobber.

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

They paint a bulls-eye on the code that has the overflow problem, but not on how to actually get it to overflow.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?

Probably all branches.

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

I don't have backports made yet, but they'd be easy to create, and low-risk.

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

It's very unlikely to cause regressions. I've done extensive local testing of it.

Comment 18

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

Comment on attachment 8345022 [details] [diff] [review]
ds-overflow-all.patch

sec-approval+ with the same caveat (checkin on 12/23 or laster).

Comment 19

[Dan Gohman [:sunfish]]

Comment on attachment 8341867 [details] [diff] [review]
ds-asmjscall.patch

I resubmitted this asmjscall patch in bug 948241, as it wasn't actually security-relevant.

For the main patches on this bug, I will wait until 12/23 as directed.

Comment 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8345022 [details] [diff] [review]
ds-overflow-all.patch

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

::: js/src/jit/FixedList.h
@@ +61,1 @@
>          T *list = (T *)alloc.allocate((length_ + num) * sizeof(T));

nit: replace (length_ + num) by newLength in the above expression, to ensure that one is not mutated without the other.

Comment 21

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68f065d772e8

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/68f065d772e8

Comment 23

[bhavana bajaj [:bajaj]]

NI on :sunfish to help with uplift here so this can get into our next beta.

Comment 24

[Dan Gohman [:sunfish]]

What information do you need from me here?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Approval requests would be nice :)

Comment 26

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

Does the patch apply to branches as is?

Comment 27

[Dan Gohman [:sunfish]]

Comment on attachment 8345022 [details] [diff] [review]
ds-overflow-all.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 684039
User impact if declined: Potential security bug
Testing completed (on m-c, etc.): On mozilla-central
Risk to taking this patch (and alternatives if risky): Quite low; the changes are simple and localized.
String or IDL/UUID changes made by this patch: None.

Comment 28

[Dan Gohman [:sunfish]]

Attachment: ds-overflow-all-beta.patch

Attached is the patch ported to the mozilla-beta branch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 684039
User impact if declined: Potential security bug
Testing completed (on m-c, etc.): On mozilla-central
Risk to taking this patch (and alternatives if risky): Quite low; the changes are simple and localized.
String or IDL/UUID changes made by this patch: None.

Comment 29

[chris hofmann]

Unsettting bounty nomination. Dan has passed on receiving a bounty since google is paying him to work on Mozilla code.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/58f46efec92e
https://hg.mozilla.org/releases/mozilla-beta/rev/12b7cb9e2054
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/66c0f155776b

Please post esr24/b2g18 patches as well.

Comment 31

[bhavana bajaj [:bajaj]]

Comment on attachment 8356266 [details] [diff] [review]
ds-overflow-all-beta.patch



[Triage Comment]
Spoke on irc with Ryan as the same pacth applies to b2g/esr, approving for uplift.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/41411b6b5e15

Comment 33

[Dan Gohman [:sunfish]]

Attachment: ds-overflow-all-b2g18.patch

Attached is the patch ported to mozilla-b2g18. I removed the Ion parts, since Ion is disabled in b2g18.

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 684039
User impact if declined: Potential security bug
Testing completed: On mozilla-central
Risk to taking this patch (and alternatives if risky): Quite low; the changes are simple and localized.
String or UUID changes made by this patch: None.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8356774 [details] [diff] [review]
ds-overflow-all-b2g18.patch

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e8c68b175abb

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e8c68b175abb

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] unless someone provides a runnable test case.