This patch introduces ASLR in FreeBSD.
Details
Details
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
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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. | |
| 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. | |
Comment Actions
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
Comment Actions
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. | |
Comment Actions
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.
Comment Actions
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. | |
Comment Actions
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 | I think there is a new convergence in the kernel: | |
| 776 | Not yet, but after the latest sys_mmap cleanup from jhb@ I changed and cleaned up the MAP_32BIT part. | |
| 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. | |
Comment Actions
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. | |
Comment Actions
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
Comment Actions
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?
Comment Actions
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.
Comment Actions
$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.
Comment Actions
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