Closed Bug 1044077 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox40 --- wontfix
firefox41 + fixed
firefox42 + fixed
firefox43 + fixed
firefox-esr31 --- wontfix
firefox-esr38 41+ fixed

People

(Reporter: mccr8, Assigned: njn)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+] (stepping-stone for exploits))

Attachments

(1 file, 1 obsolete file)

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.)
(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.
We should solve this with bug 1108693.
(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.
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?
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.
See Also: → 1182002
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).
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.
Might I point out that my exploit for bug 1179484 would probably not be possible and certainly not as reliable without poisoning?
Attached patch Tweak some jemalloc constants (obsolete) — Splinter Review
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.
Attachment #8633872 - Flags: feedback?(mh+mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Wait, 0xe3 is JCXZ which is a valid user-space instruction. So I changed the
alloc fill value to 0xe4.
Attachment #8633876 - Flags: feedback?(mh+mozilla)
Attachment #8633872 - Attachment is obsolete: true
Attachment #8633872 - Flags: feedback?(mh+mozilla)
Attachment #8633876 - Flags: feedback?(mh+mozilla) → feedback+
(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.
> > 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
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.
Nicolas, do you recall how jemalloc poisoned memory ended up being executed in bug 1036779? Thanks.
Flags: needinfo?(nicolas.b.pierron)
(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
Flags: needinfo?(nicolas.b.pierron)
Nick, is this ready to move forward? Thanks.
Flags: needinfo?(n.nethercote)
(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.
Flags: needinfo?(n.nethercote)
Whiteboard: [waiting for jemalloc4]
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.
Flags: needinfo?(n.nethercote)
Keywords: sec-wantsec-moderate
Whiteboard: [waiting for jemalloc4] → [waiting for jemalloc4] (stepping-stone for exploits)
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.
(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.
Flags: needinfo?(n.nethercote)
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?
Attachment #8633876 - Flags: review?(mh+mozilla)
Attachment #8633876 - Flags: review?(mh+mozilla) → review+
> > 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.
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.
> 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.
(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
(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
(they are all valid thumb instructions too)
Attachment #8633876 - Flags: feedback+
Attachment #8633876 - Flags: sec-approval?
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.
Attachment #8633876 - Flags: approval-mozilla-aurora?
(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 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.
Attachment #8633876 - Flags: sec-approval?
Whiteboard: [waiting for jemalloc4] (stepping-stone for exploits) → (stepping-stone for exploits)
I created bug 1200951 for doing the same thing with jemalloc4.
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?
Flags: needinfo?(n.nethercote)
(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.
Flags: needinfo?(n.nethercote)
(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?
Flags: needinfo?(n.nethercote)
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.
Attachment #8633876 - Flags: approval-mozilla-esr38?
Attachment #8633876 - Flags: approval-mozilla-beta?
Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

Let's take this sec fix that in Beta41 and Aurora42
Attachment #8633876 - Flags: approval-mozilla-beta?
Attachment #8633876 - Flags: approval-mozilla-beta+
Attachment #8633876 - Flags: approval-mozilla-aurora?
Attachment #8633876 - Flags: approval-mozilla-aurora+
Comment on attachment 8633876 [details] [diff] [review]
Tweak some jemalloc constants

Sec fix, let's uplift this to ESR as well.
Attachment #8633876 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Flags: needinfo?(n.nethercote)
Whiteboard: (stepping-stone for exploits) → [post-critsmash-triage](stepping-stone for exploits)
Group: core-security → core-security-release
This patch doesn't appear to apply at all to esr38.
Flags: needinfo?(n.nethercote)
Sorry, somehow ended up on the wrong tab.
Flags: needinfo?(n.nethercote)
Whiteboard: [post-critsmash-triage](stepping-stone for exploits) → [post-critsmash-triage][adv-main41+][adv-esr38.3+] (stepping-stone for exploits)
See Also: → 1267557
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.