1044077 - Change jemalloc poison address to something that is not a nop-slide

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Andrew McCreight [:mccr8]]

In bug 1036779 comment 4, nbp pointed out that the 0x5a5a5a5a poison value actually turns into valid instructions, so if the JIT ends up running over freed memory, it doesn't crash.  Somebody should figure out what kind of value we can use for poisoning that will fail when directly executed.

(I'm marking this s-s for now, but maybe it is obvious to anybody who would be writing an exploit, and so it doesn't need to be hidden.)

Comment 1

[Mike Hommey [:glandium]]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> In bug 1036779 comment 4, nbp pointed out that the 0x5a5a5a5a poison value
> actually turns into valid instructions, so if the JIT ends up running over
> freed memory, it doesn't crash.  Somebody should figure out what kind of
> value we can use for poisoning that will fail when directly executed.
> 
> (I'm marking this s-s for now, but maybe it is obvious to anybody who would
> be writing an exploit, and so it doesn't need to be hidden.)

It probably will have to be per-architecture.

Comment 2

[Benjamin Smedberg]

We should solve this with bug 1108693.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> We should solve this with bug 1108693.

That's unrelated. Bug 1108693 is about 0x5a5a5a5a as an address being handled appropriately. This bug is about 0x5a5a5a5a as an instruction (or set of instructions) doing something sensitive.

Comment 4

[(Away)]

Possible candidates:
0xCC (int3, maybe not distinctive enough?)
0xE4-0xE7, 0xEC-0xEF (various flavors of in/out instructions, will fail in user mode)
other suggestions?

Comment 5

[(Away)]

A nice thing about the in/out series is that we could use different codes to mean different things (like a5 versus 5a in current mozjemalloc) or as fallbacks if our first choice is already allocated.

Comment 6

[SkyLined]

0xCC would lead to the address 0xCCCCCCCC, which is used by Microsoft compilers to mark uninitialized local variables (https://msdn.microsoft.com/en-us/library/aa260966%28v=vs.60%29.aspx). This might cause confusion when analyzing crashes.

Values in the range 0xE0-0xEF do not lead to any addresses that are used as markers AFAIK (see bug 1182002 for a list).

Comment 7

[(Away)]

FWIW the standard Microsoft fill patterns don't really appear in Firefox crashes, since we use a custom allocator and since we don't have the stack debugging stuff in shipping builds. So confusion from conflicts isn't really an issue.

Comment 8

[Nicolas B. Pierron [:nbp]]

FYI, fact has been remarked by some attackers:
  https://wikileaks.org/hackingteam/emails/emailid/510364
  https://wikileaks.org/hackingteam/emails/emailid/472286 (Bug 719114)

Comment 9

[SkyLined]

Might I point out that my exploit for bug 1179484 would probably not be possible and certainly not as reliable without poisoning?

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Tweak some jemalloc constants

I chose 0xe5 for the free fill value, and 0xe3 for the less important alloc
fill value.

I've only done mozjemalloc so far, because I can't work out how modifications
to jemalloc3 should work... I see the update.sh script but I don't know how
those email+patch files are generated, and if they should be sent to the
jemalloc developer list or something.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Tweak some jemalloc constants

Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
alloc fill value to 0xe4.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Created attachment 8633876 [details] [diff] [review]
> Tweak some jemalloc constants
> 
> Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
> alloc fill value to 0xe4.

Note that 0xe4 and 0xe5 are too, as are 0x5a and 0xa5. 0xe3 is a jump, though. Anyways, it should be hard to get poisoned memory to be executed because they shouldn't be in executable memory in the first place, and we shouldn't turn them into executable memory afterwards.

(In reply to Nicholas Nethercote [:njn] from comment #10)
> I've only done mozjemalloc so far, because I can't work out how modifications
> to jemalloc3 should work... I see the update.sh script but I don't know how
> those email+patch files are generated, and if they should be sent to the
> jemalloc developer list or something.

the email+patch files are just created by git format-patch. They don't need to be that way, but I've always generated them in an upstream jemalloc clone, so it was easier to use git format-patch. That should be upstreamed, but maybe for upstreaming it would be better to use constants and allow them to be overridden (something like #ifndef POISON_VALUE/#define POISON_VALUE). That allows to keep the current values upstream (if desirable) and allows us to override them at build time with DEFINES in memory/jemalloc/moz.build.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

> > Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
> > alloc fill value to 0xe4.
> 
> Note that 0xe4 and 0xe5 are too, as are 0x5a and 0xa5. 0xe3 is a jump,
> though. Anyways, it should be hard to get poisoned memory to be executed
> because they shouldn't be in executable memory in the first place, and we
> shouldn't turn them into executable memory afterwards.

What about freed JIT memory? (and allocated-but-not-written-to JIT memory, less critically) Does that get the jemalloc poison value?  See comment 0

Comment 14

[Mike Hommey [:glandium]]

bug 1036779 comment 4 is a big wtf. I'd like to hear how that actually happened, because afaik, JIT memory is mmap/munmapped, not malloc/freeed. So the only way I can see those poisoned values ending up in JIT memory is if they are _copied_ from poisoned memory. IOW, if there is a use-after-free in the code preparing the JIT code.

Now, I won't claim to know how the JIT actually works, but if it's using something else than js::jit::AllocateExecutableMemory/js::jit::DeallocateExecutableMemory then there's a problem imho.

Comment 15

[Andrew McCreight [:mccr8]]

Nicolas, do you recall how jemalloc poisoned memory ended up being executed in bug 1036779? Thanks.

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Nicolas, do you recall how jemalloc poisoned memory ended up being executed
> in bug 1036779? Thanks.

I don't even know how.  I just noticed that this is a bad thing to have in general.  I would prefer to have a jump statement which cause an infinite loop, or a xor & load than a nop-slide.

(In reply to Mike Hommey [:glandium] from comment #14)
> […]. So
> the only way I can see those poisoned values ending up in JIT memory is if
> they are _copied_ from poisoned memory. IOW, if there is a use-after-free in
> the code preparing the JIT code.

The Jit is using a LifoAlloc for a big parts of its allocations and the SystemAllocPolicy for vectors which are resized as we append more data.  I do not expect us to have such patterns at compile time. I guess we could use the DEBUG mode to double-check that we generate no such patterns.

Then none of the instructions visible in Bug 1036779 comment 3 are looking like any meaningful instruction that we produce.  These look more like data than assembly code in which we interleave some pointers.

Among the data that we have in the executable part of the code, we have the structures used by the inline caches[1].  Still, none of these structures, which are managed by jemalloc, thus there is no reasons to have any free-patterns.

By the way, the current patterns are not in Jit code, as the error code is ACCESS_VIOLATION_EXEC.  So if your concern is about Jit memory pages, then we should at least do that for the GC poison values, as we do have GC pointers baked in Jit code.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCaches.h#204-221,378-382,527-535,612-622,723-733,807-809

Comment 17

[Andrew McCreight [:mccr8]]

Nick, is this ready to move forward? Thanks.

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #17)
> Nick, is this ready to move forward? Thanks.

glandium is very close to getting jemalloc4 enabled. There's not much point in doing the changes twice (once for mozjemalloc, once for jemalloc4) so I'm waiting on that.

Comment 19

[Daniel Veditz [:dveditz]]

How close is jemalloc4? It seems like jemalloc3 was just around the corner until eventually it wasn't -- how do we know jemalloc4 won't end up the same way?

It seems like a pretty straightforward patch, would it be all that hard to apply it twice? The reporter of bug 1179484 wants to blog about his exploit and that will reveal bug 1182002. If others have figured it out (and SkyLined wasn't the first to comment on it, e.g. this bug) they could be using it to turn vulnerabilities into exploits.

Comment 20

[Daniel Veditz [:dveditz]]

In any case, soon or not we certainly won't be putting jemalloc4 into Firefox 41 or the ESR-38 branch. We'll still need this mozjemalloc patch.

Comment 21

[Nicholas Nethercote [inactive]]

(In reply to Daniel Veditz [:dveditz] from comment #19)
> How close is jemalloc4? It seems like jemalloc3 was just around the corner
> until eventually it wasn't -- how do we know jemalloc4 won't end up the same
> way?

jemalloc3 and jemalloc4 are conceptually the same, i.e. "the latest version of jemalloc". It's just that jemalloc 3.whatever became jemalloc 4.0.0 just recently.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

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

glandium, any reason we shouldn't land this as-is?

Comment 23

[Nicholas Nethercote [inactive]]

> > Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
> > alloc fill value to 0xe4.
> 
> Note that 0xe4 and 0xe5 are too

They are IN instructions, as far as I can tell, e.g. from http://www.sandpile.org/x86/opc_1.htm, and comment 4.

Comment 24

[(Away)]

What should we do about communicating this change? A bunch of people have come to know what the poison value entails for debugging and bug filing, and they'll need to be on the lookout for the updated value.

Comment 25

[Nicholas Nethercote [inactive]]

> What should we do about communicating this change? A bunch of people have
> come to know what the poison value entails for debugging and bug filing, and
> they'll need to be on the lookout for the updated value.

I too was wondering that. It would be good to announce it (e.g. on dev-platform) so Mozilla devs know about it, but that would broadcast it to blackhats as well. I'll defer to what the experts (e.g. dveditz) think.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> > > Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
> > > alloc fill value to 0xe4.
> > 
> > Note that 0xe4 and 0xe5 are too
> 
> They are IN instructions, as far as I can tell, e.g. from
> http://www.sandpile.org/x86/opc_1.htm, and comment 4.

They are valid arm instructions.
   0:	e4e4e4e4 	strbt	lr, [r4], #1252	; 0x4e4
   4:	e5e5e5e5 	strb	lr, [r5, #1509]!	; 0x5e5

Comment 27

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #26)
> (In reply to Nicholas Nethercote [:njn] from comment #23)
> > > > Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
> > > > alloc fill value to 0xe4.
> > > 
> > > Note that 0xe4 and 0xe5 are too
> > 
> > They are IN instructions, as far as I can tell, e.g. from
> > http://www.sandpile.org/x86/opc_1.htm, and comment 4.
> 
> They are valid arm instructions.
>    0:	e4e4e4e4 	strbt	lr, [r4], #1252	; 0x4e4
>    4:	e5e5e5e5 	strb	lr, [r5, #1509]!	; 0x5e5

But so are 5a and a5:
   0:	5a5a5a5a 	bpl	1696970
   4:	a5a5a5a5 	strge	sl, [r5, #1445]!	; 0x5a5

Comment 28

[Mike Hommey [:glandium]]

(they are all valid thumb instructions too)

Comment 29

[Nicholas Nethercote [inactive]]

Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

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

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

Not sure. We already know that exploits exist.

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?

All of them.

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

N/A

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

This patch should backport to all patches trivially.

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

Unlikely. It's writing values that should never be read by valid code.

Comment 30

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c485dd582d1d

Comment 31

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Mike Hommey [:glandium] from comment #27)
> (In reply to Mike Hommey [:glandium] from comment #26)
> > They are valid arm instructions.
> >    0:	e4e4e4e4 	strbt	lr, [r4], #1252	; 0x4e4
> >    4:	e5e5e5e5 	strb	lr, [r5, #1509]!	; 0x5e5
> 
> But so are 5a and a5:
>    0:	5a5a5a5a 	bpl	1696970
>    4:	a5a5a5a5 	strge	sl, [r5, #1445]!	; 0x5a5

Anything that starts with 0 through D is predicated in ARM mode, so might be a no-op depending on flags.  Of the remaining 32, many are well-defined non-privileged instructions in at least one mode; exceptions follow:

0xeeeeeeee is an attempt to access the debug unit (same meaning in ARM and Thumb), which I'd expect should reliably fault in user mode, and on my Nexus 5 it results in SIGILL in both ARM and Thumb modes.  0xfefefefe is similar.

The four bytes matching 0x[ef][cd] are operations on cp12/cp13 (respectively), but it might be possible for a system to map those to coprocessors that user mode would be allowed to access?

0xf8f8f8f8, 0xf9f9f9f9, and 0xffffffff are currently undefined, but future architecture revisions could make them mean something.

So, if we wanted to use a different value on ARM, these are some possibilities.

Comment 32

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

Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

A sec-moderate doesn't need sec-approval to check in, just highs and criticals. So you are good to go.

Comment 33

[Nicholas Nethercote [inactive]]

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

Comment 34

[Nicholas Nethercote [inactive]]

I created bug 1200951 for doing the same thing with jemalloc4.

Comment 35

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

Nicholas, do you think we should also uplift this patch to Beta? Or do you prefer stabilizing this in Aurora for a few days before requesting Beta uplift?

Comment 36

[Nicholas Nethercote [inactive]]

(In reply to Ritu Kothari (:ritu) from comment #35)
> Nicholas, do you think we should also uplift this patch to Beta? Or do you
> prefer stabilizing this in Aurora for a few days before requesting Beta
> uplift?

I think it's fine to uplift now. It's a very simple patch. It could even be uplifted to ESR.

Comment 37

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

(In reply to Nicholas Nethercote [:njn] from comment #36)
> (In reply to Ritu Kothari (:ritu) from comment #35)
> > Nicholas, do you think we should also uplift this patch to Beta? Or do you
> > prefer stabilizing this in Aurora for a few days before requesting Beta
> > uplift?
> 
> I think it's fine to uplift now. It's a very simple patch. It could even be
> uplifted to ESR.

Nicholas, in that case, can you please request Beta and ESR uplift for the patch then?

Comment 38

[Nicholas Nethercote [inactive]]

Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

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

See comment 29 for the uplift request details.

Comment 39

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

Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

Let's take this sec fix that in Beta41 and Aurora42

Comment 40

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a8459b40a444

Not sure why the merge to m-c wasn't marked:
https://hg.mozilla.org/mozilla-central/rev/dbf06fdc8ecc

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

Sec fix, let's uplift this to ESR as well.

Comment 42

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

https://hg.mozilla.org/releases/mozilla-beta/rev/8008235a2429

Comment 43

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

This patch doesn't appear to apply at all to esr38.

Comment 44

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

Sorry, somehow ended up on the wrong tab.

Comment 45

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

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