Page MenuHomeFreeBSD

Adopt higher level of stack protection.
AbandonedPublic

Authored by koobs on Aug 23 2015, 3:21 AM.

Details

Reviewers
jlh
pfg
imp
op
Group Reviewers
secteam
Summary

It is well known that the normal stack protection supported from
the early gcc days is very weak. Google engineers have developed
a new ssp-protector-strong option that provides much better
protection without hurting performance.

This has been adopted by most linux distributions[2]
and OpenBSD. Apparently clang in FreeBSD 11 supports it and
we recently brought support for that option in our base gcc
so it seems like a good moment to adopt such option.

This is complementary but unrelated to the GSoC 2015
to bring the FORTIFY_SOURCE functionality to libc.

[1] https://lwn.net/Articles/584225/
[2] https://securityblog.redhat.com/tag/stack-protector/

Test Plan

Tested for performance with dbench (pho).
Passed port exp-run with stricter --stack-protector-all.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 194
Build 194: arc lint + arc unit

Event Timeline

pfg updated this revision to Diff 8137.Aug 23 2015, 3:21 AM
pfg retitled this revision from to Adopt higher level of stack protection..
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
pfg added a reviewer: op.
pfg added a reviewer: secteam.Aug 23 2015, 3:22 AM
pfg added a project: security.
imp requested changes to this revision.Aug 23 2015, 3:25 AM
imp added a reviewer: imp.

What's the performance impact?

This revision now requires changes to proceed.Aug 23 2015, 3:25 AM
op edited edge metadata.Aug 23 2015, 12:03 PM
This comment was removed by op.
op added a comment.Aug 23 2015, 12:07 PM

We should investigate that the -fstack-protector-strong triggers the same as -fstack-protector-all:

https://github.com/freebsd/freebsd/blob/master/lib/libc/Makefile#L187

I could add this patch to HardenedBSD 11-CURRENT-unstable, and see what's happen.

pfg added a comment.Aug 23 2015, 3:26 PM
In D3463#70669, @imp wrote:

What's the performance impact?

According to redhat/fedora the performance impact is negligible[1] and all linux distributions have been passing the word without any hard numbers.

As I see it, the FreeBSD project trusted this technology but it has a completely inadequate coverage (about 2%), this bumps the coverage to about 20% without adding significant overhead. The alternative should just be disabling completely the stack protection for production builds, since 2% is just useless and AFAICT has provided no real protection.

I also see this a temporary step while we figure out the safestack bits and as an alternative for builds that still depend on GCC.

[1] https://fedorahosted.org/fesco/ticket/1128

pfg added a reviewer: jlh.Aug 27 2015, 4:07 PM
pfg added a comment.Aug 28 2015, 2:57 PM
This comment was removed by pfg.
pfg added a comment.Aug 29 2015, 7:53 PM
This comment was removed by pfg.

I'll patch my laptop and have started a clean build so I'll see if there would be visible performance/other impact after running with it for a few days.

Will it be possible that you can provide some basic test data, for instance, a few world stone runs on memory disk, and see how exactly the impact would be? It would be easier to convince people when you have first hand data, and although we know others have already do some testing, it's important to know if the same applies on FreeBSD because it's likely that the experiments were not on FreeBSD, and the result may be different.

I'd recommend requesting an exp-run in the meantime, by the way. In general I'm for this be enabled by default, as long as the performance change is within in an reasonable range and thanks for working on it!

pfg added a comment.Sep 26 2015, 3:17 AM

...

Will it be possible that you can provide some basic test data, for instance, a few world stone runs on memory disk, and see how exactly the impact would be? It would be easier to convince people when you have first hand data, and although we know others have already do some testing, it's important to know if the same applies on FreeBSD because it's likely that the experiments were not on FreeBSD, and the result may be different.

OK, yes I see. Also the clang implementation may present important differences with the GCC implementation. I will running some other tests.

I'd recommend requesting an exp-run in the meantime, by the way. In general I'm for this be enabled by default, as long as the performance change is within in an reasonable range and thanks for working on it!

An exp-run is unlikely to show anything but I think I will add in a patch to activate relro (which is also supposed to have no performance impact) so we can test both changes simultaneously over the ports tree.

pfg added a comment.Sep 28 2015, 2:37 PM

Peter Holm has kindly provided some testing:

"... There seems to be no difference between a pristine build and one with your patch.
Your patch only seems to add 128 bytes to the kernel size, so if that
is true that could explain the findings. If I had more time I would
probably investigate the consequence of the flag compared the object
file size of a few files. ..."

This mostly matches the results from linux but I still have to look at the details.

For the exp-run I opened PR 203394 (it includes another change as well which shouldn't have any consequence)..

pfg added a comment.Oct 3 2015, 5:56 PM

Results from exp-run (PR 2013394) are out:


Exp-run results on head i386:
http://package23.nyi.freebsd.org/jail.html?mastername=headi386PR203394-default

Exp-run results on head amd64:
http://package22.nyi.freebsd.org/jail.html?mastername=headamd64PR203394-default

0 new failure


Huge thanks to Peter Holm and Antoine Brodin for the extensive testing.

Given that we now have hard evidence from dbench that there is no performance hit from the patch and that the change doesn't cause any breakage in regular operation, I intend to commit the change between today and tomorrow (I'll give some time for people to express approval for the change).

pfg edited the test plan for this revision. (Show Details)Oct 4 2015, 7:10 PM

Committed as r288669.

pfg abandoned this revision.Oct 8 2015, 3:12 PM

This is already in the tree (apparently no one approved it though).

koobs added a subscriber: koobs.Oct 22 2015, 9:49 AM
koobs commandeered this revision.Oct 22 2015, 9:56 AM
koobs added a reviewer: pfg.

Apologies, I intended to Commandeer -> Close.