Page MenuHomeFreeBSD

Per-Process PS_STRINGS, USRSTACK, shared_object_base, and sigcode_base
AbandonedPublic

Authored by lattera-gmail.com on Sep 4 2015, 12:05 AM.

Details

Summary

This patch introduces per-process PS_STRINGS, USRSTACK, shared_object_base, and sigcode_base as a prerequisite to Address Space Layout Randomization (ASLR). This is needed in order to achieve full randomization of the shared object (aka, the VDSO) and the stack.

Test Plan
  1. Apply patch to HEAD
  2. Rebuild world+kernel
  3. Install new world+kernel
  4. Reboot
  5. Run ATF tests
  6. If you want to verify your application is working as it should: procstat -v $$

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lattera-gmail.com retitled this revision from to Per-Process PS_STRINGS, USRSTACK, shared_object_base, and sigcode_base.
lattera-gmail.com updated this object.
lattera-gmail.com edited the test plan for this revision. (Show Details)
lattera-gmail.com added reviewers: rwatson, kib, alc.
lattera-gmail.com set the repository for this revision to rS FreeBSD src repository.
lattera-gmail.com added a project: HardenedBSD.
kib edited edge metadata.Sep 8 2015, 10:36 AM

Anything like this patch can only be considered if your 'aslr' implementation is ever found acceptable. Until it is not, the patch only adds bloat.

That said, and being whatever trivial, the patch has technical issues. The first and most obvious one is the fact that struct proc is the wrong location for the data you put into it. I argue that the locations of objects in the user address space is the property of the vmspace and not the process.

Second issue is less trivial. Right now, the data which you moved is located in the static (as in, not changing) struct sysentvec. In other words, after you have dereferenced sv = p->p_sysent, you know that you have a pointer to the stable and _consistent_ set of values like address of psstrings, address of signal trampoline, base of the shared page etc. With your patch, this property is lost. Now, you must guarantee that the operations which modify the user address space layout, like execve(), do not happen in parallel. I did not inspected your patch closely for the issue, but neither did you.

And, of course, the patch changes the ABI by making e.g. psstrings/sigtrampoline process-private, instead of all ABI-sharing processes sharing the psstrings/sigtramp value. I am certain that this e.g. breaks core dump analysis due to signal trampoline address change.

In D3565#74326, @kib wrote:

Anything like this patch can only be considered if your 'aslr' implementation is ever found acceptable. Until it is not, the patch only adds bloat.

That said, and being whatever trivial, the patch has technical issues. The first and most obvious one is the fact that struct proc is the wrong location for the data you put into it. I argue that the locations of objects in the user address space is the property of the vmspace and not the process.

Second issue is less trivial. Right now, the data which you moved is located in the static (as in, not changing) struct sysentvec. In other words, after you have dereferenced sv = p->p_sysent, you know that you have a pointer to the stable and _consistent_ set of values like address of psstrings, address of signal trampoline, base of the shared page etc. With your patch, this property is lost. Now, you must guarantee that the operations which modify the user address space layout, like execve(), do not happen in parallel. I did not inspected your patch closely for the issue, but neither did you.

And, of course, the patch changes the ABI by making e.g. psstrings/sigtrampoline process-private, instead of all ABI-sharing processes sharing the psstrings/sigtramp value. I am certain that this e.g. breaks core dump analysis due to signal trampoline address change.

Hey Kostik,

Remember that we're not actually applying ASLR in this patch as this is simply a prerequisite patch, so the values copied from the sysentvec will remain constant, as you pointed out above.

You bring up some technical points that I do need to research further. Before making any more changes or comments, I'm going to wait for Robert Watson's review of this patch.

Thank you for your constructive and productive comments. I look forward to working with you in a positive manner as this patch progresses.

Thanks,

Shawn

op added a reviewer: adrian.Sep 21 2015, 11:26 AM
op removed a subscriber: adrian.
op added a subscriber: op.Sep 21 2015, 12:15 PM
In D3565#74326, @kib wrote:

Anything like this patch can only be considered if your 'aslr' implementation is ever found acceptable. Until it is not, the patch only adds bloat.

Yes, I'm agreed with this.

That said, and being whatever trivial, the patch has technical issues. The first and most obvious one is the fact that struct proc is the wrong location for the data you put into it. I argue that the locations of objects in the user address space is the property of the vmspace and not the process.

Second issue is less trivial. Right now, the data which you moved is located in the static (as in, not changing) struct sysentvec. In other words, after you have dereferenced sv = p->p_sysent, you know that you have a pointer to the stable and _consistent_ set of values like address of psstrings, address of signal trampoline, base of the shared page etc. With your patch, this property is lost. Now, you must guarantee that the operations which modify the user address space layout, like execve(), do not happen in parallel. I did not inspected your patch closely for the issue, but neither did you.

And, of course, the patch changes the ABI by making e.g. psstrings/sigtrampoline process-private, instead of all ABI-sharing processes sharing the psstrings/sigtramp value. I am certain that this e.g. breaks core dump analysis due to signal trampoline address change.

The signal handling and debugging is fully working after this patch, and even working, when you applied our whole patch with full randomization.

See this small test case:

op@opn /tmp> sysctl hardening.pax.aslr
hardening.pax.aslr.vdso_len: 28
hardening.pax.aslr.exec_len: 30
hardening.pax.aslr.stack_len: 42
hardening.pax.aslr.mmap_len: 30
hardening.pax.aslr.status: 2
op@opn /tmp> cat test.c
#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <signal.h>

void *
mthr(void *arg)
{
        raise(4);

        return (NULL);
}

int
main(int argc, char **argv)
{
        pthread_t       thread_0;

        pthread_create(&thread_0, NULL, mthr, NULL);

        printf(".\n");

        pthread_join(thread_0, NULL);

        return (0);
}
op@opn /tmp> rm test
op@opn /tmp> make test
cc -O2 -pipe -pipe   -DHARDENEDBSD -lpthread -ggdb  test.c  -o test
op@opn /tmp> gdb ./test 
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "amd64-marcel-freebsd"...
(gdb) r
Starting program: /tmp/test 
[New LWP 100548]
[New Thread 14385215400 (LWP 100551/test)]

Program received signal SIGILL, Illegal instruction.
[Switching to Thread 14385215400 (LWP 100551/test)]
0x0000014384c2bcba in thr_kill () from /lib/libc.so.7
(gdb) thread signal
signal mask:

signal pending:

si_signo 4 si_errno 0
si_code UNKNOWN (65543) si_pid 8375 si_uid 1001 si_status 0 si_addr 0x0
(gdb)
swills added a subscriber: swills.Oct 1 2015, 1:55 PM

This update brings the patch current with latest FreeBSD HEAD. No other changes.

brooks added a subscriber: brooks.Nov 5 2015, 12:56 AM

Looking at the members of struct proc and struct vmspace I do think I agree that vmspace is a better place for these things.

I'll or someone else on our team be exploring what happens to coredumps with an alternative layout of sigcode and ps_strings on CHERI so hopefully we'll have some useful insights there.

One thing I have been wondering is if it makes sense to add a syscall to retrieve a ps_strings pointer rather than relying on constructing a pointer from a long from sysctl.

sys/sys/proc.h
601

I don't understand why the latter two aren't marked (b). The p_shared_page_base ends up in Auxargs so it must be set before the program or rtld is started. I suppose one could defer setting up sigcode, but that seems unlikely to be a worthwhile optimization.

kib added inline comments.Nov 5 2015, 1:53 PM
sys/sys/proc.h
601

I do not understand neither the patch, nor you comment, in this regard. The ABI base object pointers are only constant between execs, not forks.

lattera-gmail.com abandoned this revision.Dec 19 2015, 6:49 PM

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