Re: [CFT] ASLR, PIE, and segvguard on 11-current and 10-stable

On 5/24/14, Shawn Webb <lattera_at_gmail.com> wrote:
> On May 23, 2014 07:53 PM +0000, Wojciech A. Koszek wrote:
>> On Wed, May 14, 2014 at 09:58:52AM -0400, Shawn Webb wrote:
>> > Hey All,
>> >
>> > [NOTE: crossposting between freebsd-current_at_, freebsd-security_at_, and
>> > freebsd-stable_at_. Please forgive me if crossposting is frowned upon.]
>> >
>> > Address Space Layout Randomization, or ASLR for short, is an exploit
>> > mitigation technology. It helps secure applications against low-level
>> > exploits. A popular secure implementation is known as PaX ASLR, which
>> > is
>> > a third-party patch for Linux. Our implementation is based off of
>> > PaX's.
>> >
>> > Oliver Pinter, Danilo Egea, and I have been working hard to bring more
>> > features and robust stability to our ASLR patches. We've done extensive
>> > testing on amd64. We'd like to get as many people testing these
>> > patches.
>> > Given the nature of them, we'd also like as many eyeballs reviewing the
>> > code as well.
>> >
>> > I have a Raspberry Pi and have noticed a few bugs. On ARM (at least, on
>> > the RPI), when a parent forks a child, and the child gracefully exits,
>> > the parent segfaults with the pc register pointing to 0xc0000000. That
>> > address is always the same, no matter the application. If anyone knows
>> > the ARM architecture well, and how FreeBSD ties into it, I'd like a
>> > little guidance.
>> >
>> > I also have a sparc64 box, but I'm having trouble getting a vanilla
>> > 11-current system to be stable on it. I ought to file a few PRs.
>> >
>> > You can find links to the patches below.
>> >
>> > Patch for 11-current:
>> > http://www.crysys.hu/~op/freebsd/patches/20140514091132-freebsd-current-aslr-segvguard-SNAPSHOT.diff
>> >
>> > Patch for 10-stable:
>> > http://www.crysys.hu/~op/freebsd/patches/20140514091132-freebsd-stable-10-aslr-segvguard-SNAPSHOT.diff
>> >
>>
>> Shawn
>>
>> I appreciate you working on this. We must have this in FreeBSD.
>>
>> I looked at the patch and I read, but not run it.  Comments below.
>>
>> My personal opinion is that kern_pax.c should be compiled in by default.
>> If
>> it adds a lot of size, it'd be better to provide empty stub calls instead
>> of
>> #ifdef'ing everything. But security is very important especially in
>> embeddded systems, so you can imagine you're writing the code that
>> everybody
>> wants and must have enabled for decent level of security.
>>
>> All modern systems run with ASLR turned on.
>>
>> I skipped user-space stuff. I don't think it's necessary in this commit
>> and
>> should be separated.
>>
>> There's a lot of lines of code for status showing. Not sure if we care
>> that
>> much: ASLR is either on or off. Not sure about more granularity. More
>> below.
>
> We provide the level of granularity because there are a lot of
> applications that might exhibit weird behaviors or even crash if we
> randomize too many bits. We provide sane defaults, but allow each user
> to choose the level of security versus the level of stability they
> desire.

Two idea here:
a) create a tunable security.pax.expert_mode, and create sysctls at
boot time depending from expert mode
  ( https://github.com/HardenedBSD/hardenedBSD/blob/hardened/current/aslr/sys/kern/kern_sysctl.c#L460
)
b) just add CTLFLAG_SKIP and hide the sysctl from normal user
  ( https://github.com/HardenedBSD/hardenedBSD/blob/hardened/current/aslr/sys/kern/kern_sysctl.c#L739
)

>
>>
>> Lots of files:
>>
>> You conditionally make .sv_pax_aslr_init method point to something else.
>> I'd
>> assume PAX function _pax_aslr_init32() always gets called and based on
>> whether ASLR is on or not, it does something or not. This will simplify
>> the
>> code a lot, and the difference probably won't be measurable.
>>
>> You have:
>>
>> 	int	a;
>> 	int	b;
>>
>> instead of:
>>
>> 	int	a, b;
>>
>> And you miss spaces around "=" sometimes.
>
> Cleaning up the code and make style changes are a high priority on my
> list. Once I get a few more pieces of code locked down, I'm going to go
> over every line with a comb to make sure I'm adhering to the FreeBSD
> coding style. des_at_ has made a lot of suggestions in that regard and has
> even provided me with a sample vimrc. Prior to talking with des_at_, I was
> re-using the same vimrc that I use for ClamAV (which, admittedly, has a
> much different coding style than FreeBSD).
>
>>
>> kern_jail.c:
>>
>> something looks wrong here. Sounds like you need "pr->pax".  But I don't
>> understand why you need to have these pr_* values here. It seems
>> unnecessary.
>
> I've made it possible to have per-jail ASLR settings. If you have an
> application that misbehaves, you can jail it with ASLR turned off just
> for that jail. My BSDCan presentation talks about this. The recording
> isn't up, yet, though.
>
>>
>> kern_pax.c:
>>
>> I can't quickly tell what locking is using. Some ASSERTS() in pax_
>> function
>> would help.
>>
>> pax_aslr_active():
>>
>> I don't see why you need to pass "td" and "proc" (I looked at usage: you
>> pass proc only once). I think you could always pass proc to it, with
>> td->td_proc passed typically.
>> kern_pax_*:
>>
>> There's so many SYSCTLs I think people will have problem configuring it.
>> Pick reasonable value for all values and let users change them via
>> SYSCTL_INT (static sysctls) only for debugging.
>
> There are quite a few SYSCTLs, I agree. I'll talk with Oliver Pinter,
> one of the developers that is working with me on this ASLR
> implementation, to see if we can simplify this.
>
>>
>> I can imagine we won't want ASLR only temporarily, for ports which break
>> and
>> must be fixed. So we probably just need per-process ASLR on/off switch and
>> a
>> wrapper which could be used like:
>>
>> 	aslr off program ....
>
> So we have right now an addition to mac_bsdextended(4)/ugidfw(8) that
> does this exact thing. We also plan on adding FS extended attribute
> support soon, too. Also, per-jail ASLR settings.
>
>>
>> The debug stuff I'd remove too. We could have additional CTR stubs used
>> there, if necessary.
>
> Oliver just released a new patchset today with new debugging
> functionality. I'd love to hear your commments on it.
>
>>
>> segvguard part I didn't understand. Why do you keep a list of programs
>> that
>> failed? There was no ASSERTs, thus it was hard to understand the locking
>> too.
>
> We've semi-paused development of segvguard for the moment to focus on
> ASLR. Though the features are related and segvguard is recommended for a
> proper ASLR implementation, it is not required. Danilo Egea Gondolfo is
> the principal engineer behind our segvguard implementation. We're still
> working out the kinks and the underlying design and architecture of this
> feature.
>
>>
>> I'm trying to understand if randomization is done correctly. Do you think
>> you could post the results?
>>
>> Program:
>>
>> 	http://pastebin.com/XTRHLhMg
>
> My results on an amd64 VM are pasted here: http://ix.io/cD5
>
> We're in talks with des_at_, kib_at_, and Alan Cox regarding how our
> implementation could affect the VM system, with special consideration to
> superpages.
>
> Thanks for taking the time to read through the code and offer insight.
> One of the things we need to do is write documentation regarding our
> implementation. Both Oliver and I have wiki accounts and we've created
> the start of the documentation there. I think if we had better
> documented our implementation, there would've been less confusion on the
> part of those reading/analysing our code.
>
> wiki page: https://wiki.freebsd.org/Hardening
>
> Thanks,
>
> Shawn
>