Closed Bug 1200951 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: njn, Assigned: RyanVM)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main53-] (stepping-stone for exploits))

Attachments

(1 file, 1 obsolete file)

In bug 1044077 we did this for mozjemalloc. We should do it for jemalloc4 as well, if it gets enabled. And possibly upstream the change as well.
I opened https://github.com/jemalloc/jemalloc/issues/272 to discuss upstream. I went with a public issue because it's not exactly an unknown problem.
Group: core-security → core-security-release
Looks like this was fixed upstream.
https://github.com/jemalloc/jemalloc/commit/a82070ef5fc3aa81fda43086cdcc22bfa826b894

Given that we just recently updated our in-tree copy of jemalloc4 to version 4.3.1, we should be able to do this now by setting JEMALLOC_ALLOC_JUNK to 0xe4 and JEMALLOC_FREE_JUNK to 0xe5. Any objections, Mike?
Flags: needinfo?(mh+mozilla)
No objection.
Flags: needinfo?(mh+mozilla)
Not sure how to verify that this is actually working as intended (possible to cause an intentional crash somewhere that'd show the poison pattern?). I'm a bit worried that internal/util.h seems to define these unconditionally and don't know how that'll interact with defining them on the command line as well.

Green on Try, for whatever that's worth, though.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d57e2fe335ab99704b5008d2264ab500b0330079
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #8812648 - Flags: review?(mh+mozilla)
Comment on attachment 8812648 [details] [diff] [review]
Use the same poison patterns for jemalloc4 as mozjemalloc

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

This won't work because of the unconditional #ifdefs in util.h. They'd need to be wrapped with #ifndefs, or for those to become something that can be set through a configure flag.
Attachment #8812648 - Flags: review?(mh+mozilla) → review-
Thanks for confirming. I filed an upstream issue for it.
https://github.com/jemalloc/jemalloc/issues/509
Looks like we can proceed here once we update to version 4.4.0 after it's released.
https://github.com/jemalloc/jemalloc/commit/fc11f3cb8443c029f54bf9ba21574b0f61996dd2
Confirmed with dveditz that this doesn't need to be closed. It's a known issue and jemalloc4 isn't enabled by default anywhere anyway.
Group: core-security-release
As far as I can tell, this works correctly on top of the tip revision of the upstream rc-4.4.0 branch. When I used intentionally-bogus values, the builds all failed as expected. Everything builds successfully with this patch as-is.
Attachment #8812648 - Attachment is obsolete: true
Attachment #8815095 - Flags: review?(mh+mozilla)
Comment on attachment 8815095 [details] [diff] [review]
Use the same poison patterns for jemalloc4 as mozjemalloc

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

::: memory/jemalloc/moz.build
@@ +68,5 @@
>  if CONFIG['GNU_CC']:
>      CFLAGS += ['-std=gnu99']
>  
>  DEFINES['abort'] = 'moz_abort'
> +DEFINES['JEMALLOC_ALLOC_JUNK'] = '((uint8_t)0xe4)'

Kind of a bummer that one needs to put the case in the define... Something else to report upstream I guess...
Attachment #8815095 - Flags: review?(mh+mozilla) → review+
Depends on: 1322027
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3ed8b79fa7
Use the same poison patterns for jemalloc4 as mozjemalloc. r=glandium
https://hg.mozilla.org/mozilla-central/rev/ae3ed8b79fa7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Whiteboard: (stepping-stone for exploits) → [adv-main53-] (stepping-stone for exploits)
You need to log in before you can comment on or make changes to this bug.