Page MenuHomeFreeBSD

Base ASLR
AbandonedPublic

Authored by lattera-gmail.com on Jul 23 2014, 9:38 PM.

Details

Summary

This patch introduces ASLR in FreeBSD.

Test Plan

Add PAX_ASLR and PAX_SYSCTLS options to your kernel config. Recompile and install world and kernel. Reboot and execute a shell, running procstat -v <pid> inside of it to see that the process' memory is randomized.

Or execute paxtest from this link: https://github.com/HardenedBSD/tools/raw/master/tests/paxtest-freebsd/paxtest-0.9.11-fbsd64-Hunger.tgz

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rwatson added inline comments.Mar 26 2015, 6:01 PM
share/man/man4/aslr.4
3

Sequential years in copyright strings should use "-" rather than ",".

43

I'm not sure this introductory sentence is necessary.

49

This is likely invalid nroff -- "and the" should be on the next line.

52

"-based ..." should be on the next line.

54

"Randomizes" should not be capitalised.

55

Explain, briefly, why it makes exploitation more difficult for the attacker.

56

This sentence is not necessary. We wouldn't include a feature if t weren't these things.

64

s/force/configure/?

72

Use suitable markup for "sysctl" and also for the sysctl name.

74

Use markup for the option name.

75

Use markup for the sysctl name.

78

Xref jail here instead?

86

Perhaps these details belong in comments in the kernel?

87

Use markup for both sysinit and SI_SUB_PAX.

89

Use markup for the path name. Xref a suitable man page.

100

Line break after comma.

103

Line break after comma.

118

Mark up sysctl name.
Mark up "et_dyn_addr".

119

Mark up ET_DYN_LOAD_ADDR.

123

Mark up sysctl name.

149

There is probably more suitable markup for this -- e.g., a definition list sort of thing?

169

Hard sentence break required.

186

Hard sentence break required.

189

Hard sentence break required.

192

Markup sysctl name.

204

Hard sentence break required.

207

This list of xrefs doesn't match the ones in the document -- synchronise.

217

It's not clear why this should appear in the man page.

232

I wonder if the "and" should go on the next line -- I never remember with .An.

235

Explain why.

239

This comma belongs on the previous line ... but "will..." shouldn't be.

244

I naively believe that many newer programs will use the sysctl instead of the hard-coded C constant.

245

Explain why.

253

I don't think this applies to the commit candidate, so likely this sentence should be removed.

sys/conf/NOTES
2981

Should this heading be PAX-related rather than ASLR-related.

sys/conf/options
934

Sometimes you spell PAX PaX, and sometimes, PaX PAX -- but sometimes also pax. Pick one for comments.

sys/kern/imgact_elf.c
823

The original version of this code avoided assigning more than once, placing this assignment in two 'else' clauses. I'd rather you maintained the original structure, minimising differences.

Robert,

Thanks for your thorough review! I hope to take a look at this over the weekend. I hope to have a new patch that addresses your concerns on Monday.

Thanks,

Shawn

Just a little update. I worked on a new patch over the weekend and am almost ready with a new patch. I need to research how to create SDT probes and a few other tasks. I'm hoping to have an updated patch this weekend.

sys/kern/kern_pax.c
180

With KTR(9), if a kernel panic happens, we can inspect ASLR post-mortem. As DTrace is time-of-use, that's not possible with SDT probes. Would losing the ability to do post-mortem analysis/inspection be preferred?

sys/kern/imgact_elf.c
823

Though this adds to the difference, it simplifies the logic. Previously, you had et_dyn_addr being set in multiple conditionals. This now only has a single conditional for modifying it. I'd rather keep it this way for simplicity sake.

sys/kern/kern_exec.c
1416

The next patch will include MAC integration into this logic. However, the spot where I got the credential changing logic from needs additional details. There's no way to remove code duplication. If you'd like to see a different approach than the one I take with the next patch, please provide instruction on how to do so and I'd be happy to make the change.

sys/sys/jail.h
193

Some of the hardening/exploit mitigation features we hope to upstream from HardenedBSD have nothing to do with PaX. pr_hardening makes the most sense when that is taken into account.

sys/sys/kernel.h
105

I would have thought that since these kinds of things are in all caps, that SI_SUB_PAX ought to be as well in order to keep with current code style.

sys/sys/ktr_class.h
74–75

See my previous comment about losing the ability to do post-mortem analysis of ASLR in the case of kernel panic.

This update fixes all the concerns Robert Watson brought up, with a few outstanding fixes that need to be addressed by Robert Watson (due to my comments the last 24 hours). If I have missed anything, please let me know and I'd be happy to take a look ASAP.

jch added a subscriber: jch.Apr 19 2015, 8:13 AM

Are there any updates since I last updated the patch nearly a month ago?

koobs added a subscriber: koobs.
koobs added a comment.May 28 2015, 2:32 AM

Add @rwatson to reviewers given the thorough review provided

gjb removed a subscriber: gjb.May 28 2015, 1:50 PM
sbruno added a subscriber: sbruno.Jun 6 2015, 4:13 PM
rwatson edited edge metadata.Jun 6 2015, 4:55 PM

Some previous comments appear to have been missed in updating the patch, and should be addressed. A few minor cosmetic tweaks incremental to the most recent revision.

share/man/man4/aslr.4
43

Don't use contractions in formal written text ("won't" -> "will not").

46

Should PAX_ASLR be marked up as a kernel option?

124

Possibly, in this context, it should be marked up as a mmap(2) Xref not sys_mmap()? The latter is part of the implementation, but the former is about the API/service.

215

This list now seems incomplete -- jail, etc?

sys/amd64/include/vmparam.h
181–182

Is the comment explaining (4) now elsewhere? I've not spotted it yet.

sys/conf/NOTES
2983

I would like to see the change to frob read-write vs. read-only sysctls made before commit. Compiling the sysctls out seems unhelpful. That said, I'm not convinced by this argument regarding Bastion systems. Should the sysctls instead be controlled by securelevel?

sys/kern/imgact_elf.c
827

Is referring to the binary as a 'dso' really accurate here? I wonder if 'binary' would be more appropriate? Also, 'for some reason' might just be because it needs to be mapped there due to its linkage -- so possibly we should just drop 'for some reason' as there are entirely legitimate reasons reasons, not vague 'some reasons'?

sys/kern/kern_exec.c
1417

You continue to ignore my request that this logic should *not* be copied and pasted, but instead be centralised -- ideally with the decision made once, atomically, and cached, rather than in multiple places, non-atomically. This is for both functionality and code maintenance reasons.

sys/kern/kern_pax.c
19

I would also like to see this change.

92

This comment appears not to have been responded to / addressed.

sys/kern/kern_pax_aslr.c
260

Not a fan -- with many tens of thousands of sysctls, CTLFLAG_SKIP is not really a cosmetic thing about hiding advanced settings, it's about skipping nodes that have a functional impact if viewed in 'sysctl -a'.

776

Was this comment addressed?

sys/kern/sys_process.c
744

This appears not to have been addressed.

sys/sys/kernel.h
105

I was referring to the comment, not in the constant name.

Robert,

Thank you for taking the time to review our patch. I'll work to address your comments while I'm at BSDCan.

Thanks,

Shawn

Sounds good -- unfortunately, I won't be there myself this year, but will watch for an update. As I mentioned to Adrian earlier, I'm hopeful that this is the last 'incremental' tidy-up and then we can do a 'from scratch' review of the patch stand-alone rather than as the product of a long series of iterations.

I'm going to try to get some help with the credential changing stuff. That's an area that I'm not well versed in. If you have time to suggest how the credential change detection logic should like in code, I'd be happy to make that change. Ideally, we'd want all places that check for credential changing to be switched to that, but I'm not sure that's the job of an ASLR patch.

op added a comment.Jun 6 2015, 10:26 PM

I opened issues which should fix before the next upload: https://github.com/HardenedBSD/hardenedBSD-upstreaming/issues .
Feel free to open new issues here.

And we move the upstreamed/upstreamable codes to this repo: https://github.com/HardenedBSD/hardenedBSD-upstreaming

sys/arm/arm/machdep.c
296
commit bebda004cd4e9ea1e0ae6c1bd2d78a2a81cc09ec
Author: Ilya Bakulin <Ilya@Bakulin.de>
Date:   Mon Sep 29 02:46:16 2014 +0200

    PAX ASLR: Fix signal delivery on ARM when ASLR is enabled
    
    The LR register on ARM contains the return address. The way its value
    is calculated must match the corresponding code in kern/kern_exec.c,
    so call pax_aslr_stack() to adjust the LR.
    
    github-issue: #46
    Submitted-by: Ilya Bakulin <Ilya@Bakulin.de>
    Signed-off-by: Oliver Pinter <oliver.pntr@gmail.com>
sys/kern/imgact_elf.c
827

This comment was come from kib@:

op@opn hardenedBSD.git> git blame freebsd/master -- sys/kern/kern_exec.c

[...]
edf781a81 (kib        2009-10-10 15:33:01 +0000  801)           if ((brand_info->flags & BI_CAN_EXEC_DYN) == 0)
edf781a81 (kib        2009-10-10 15:33:01 +0000  802)                   return (ENOEXEC);
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  803)           /*
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  804)            * Honour the base load address from the dso if it is
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  805)            * non-zero for some reason.
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  806)            */
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  807)           if (baddr == 0)
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  808)                   et_dyn_addr = ET_DYN_LOAD_ADDR;
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  809)           else
2eb5677d2 (kib        2009-10-18 12:57:48 +0000  810)                   et_dyn_addr = 0;
[...]
933

The next update will issue the missing rtld randomization here.

sys/kern/kern_pax_aslr.c
260

Agreed.

572
776

Not yet, but after the latest sys_mmap cleanup from jhb@ I changed and cleaned up the MAP_32BIT part.
I can solve this, if I add 3 new fields to vmspace, which could contain the randoms for compat / MAP_32BIT cases. I not really like to truncate the original randoms to fit to MAP_32BIT mappings.

sys/sys/ktr_class.h
74–75

This issue has "fixed" by jhb. They extended the 32 bit ktr_class to 64 bit.

sys/sys/pax.h
86

In general I like variable names in headers, because they helps the development when you use an IDE or vim with clang_complete.

I'm going to try to get some help with the credential changing stuff. That's an area that I'm not well versed in. If you have time to suggest how the credential change detection logic should like in code, I'd be happy to make that change. Ideally, we'd want all places that check for credential changing to be switched to that, but I'm not sure that's the job of an ASLR patch.

Not entirely sure if this refers to credential_changing, but assuming it does:

Credential transition decisions were previously made in exactly one place in the kernel source code: do_execve(), while holding all of the suitable locks guaranteeing required atomicity between 'will_transition' and 'transition' for MAC, as well as with respect to the vnode, etc. By copying and pasting the code (incorrectly, no less!), the ASLR change replicated that logic, which increases overhead, makes a previously atomic operation non-atomic, and creates a real risk of future maintenance problems (vis the incorrect copy-and-paste in the earlier ASLR version!).

You need to do what the PMC code already does: cache the credential-changing choice after it has been made so that it is exposed where you later require it. One place that I think you could do that is in imgp, which is retained throughout execve(), avoiding the need to make the choice a second time.

FWIW, I'm not actually sure the logic here is really right either -- but it's probably as close as it will get for now. A comment should be left in use of the cached credential_changing flag that this may experience false positives for non-access-control credential changes at execve()-time.

sys/sys/pax.h
86

I like them too -- but in large, complex programs made up of many parts maintained by different people and organisations -- and worse, as the OS vendor where your headers are included by many third parties -- the risk of a namespace collision is simply too great. For example, if future kernel or user code or another header #defines a colliding name.

Hey Robert,

I just wanted to give you an update. I got really sick at BSDCan, so I wasn't able to work on the patch. I'll hopefully have an update for you within the coming weeks.

Thanks,

Shawn

Hi Shawn. Hope you are feeling better post-travel. Just wanted to check on the status of this patch -- will there be an updated version fixing the credential transition issue soon?

In D473#59409, @rwatson wrote:

Hi Shawn. Hope you are feeling better post-travel. Just wanted to check on the status of this patch -- will there be an updated version fixing the credential transition issue soon?

Hey Robert. Sorry I haven't updated you on this, yet. After three weeks of drugged-up heck, I'm finally back to development. We've made a ton of progress on our ASLR implementation in HardenedBSD. I'm going to backport all of that to vanilla FreeBSD HEAD and rebase the patch against that. I'll split the patch into separate, smaller patches for review. Once I'm ready to open up those smaller reviews, I'll close this one out. Unless you feel it'd be better to go a different direction.

Time-wise, I'm hoping to backport our ASLR work to our 10-STABLE branch so I can do a new OPNSense build and help those guys out. I should have that finished by the end of this week. Then I'll get to forward-porting our latest ASLR work to FreeBSD HEAD (not too hard since all our development is already based on HEAD). I should hopefully have that done by the end of next week.

That seems a sensible strategy.

In D473#59442, @rwatson wrote:

That seems a sensible strategy.

Any luck?

In D473#64973, @rwatson wrote:
In D473#59442, @rwatson wrote:

That seems a sensible strategy.

Any luck?

$WORK has been tough lately. I've been working 110 hours a week for around a month now. Zero time for the patch until around October. Sorry for the delays.

In D473#64973, @rwatson wrote:
In D473#59442, @rwatson wrote:

That seems a sensible strategy.

Any luck?

$WORK has been tough lately. I've been working 110 hours a week for around a month now.
Zero time for the patch until around October. Sorry for the delays.

No problem -- know the feeling...

New update: I've found an issue with stack randomization on ARM64. We're working on fixing it right now. You can track the bug here: https://github.com/HardenedBSD/hardenedBSD/issues/154

op added a comment.Aug 5 2015, 11:45 PM

And I found a bug with ASLR in pkg's libpkg: https://reviews.freebsd.org/D3311 .

swills added a subscriber: swills.Oct 1 2015, 1:54 PM
lattera-gmail.com abandoned this revision.Dec 19 2015, 6:49 PM

Closing this revision. FreeBSD is free to pull from HardenedBSD.