Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn on memory corruption hardening where available #53

Merged
merged 7 commits into from Jan 2, 2015

Conversation

jbarlow83
Copy link
Contributor

Many compilers enable some kind of hardening by default, but this is not universally the case, and some more aggressive options are not always enabled.

There may be a performance impact to -fstack-protector-all by default, but Chromium uses this option when -fstack-protector-strong is not available, and I think that is appropriate for a critical security component.

Jim Barlow added 4 commits December 23, 2014 04:38
...and leave a note that -Qunused-arguments is being applied to CFLAGS
not LDFLAGS, probably in error.
Where available, enable stack smashing protection, fortify source,
no-strict-overflow, and read only relocations.

Many Linux distributions automatically enable most of these options.
They are no brainers. The difference introduced here is in asking for a
few more aggressive options. An option to disable the more aggressive
options is provided (--disable-hardening). When set, configure will fall
back to the default CFLAGS on the system - in many cases that will still
be hardened. There is no point in going further than that.

Options enabled are:

-fstack-protector-strong is a relatively new GCC-4.9 feature that is
supposed to give a better balance between performance and protection.
-all is considered too aggressive, but was used in Chromium and other
security critical systems until -strong became available. Follow their
lead and use -strong when possible. clang 6.0 supports -all but not
-strong.

_FORTIFY_SOURCE replaces certain unsafe C str* and mem* functions with
more robust equivalents when the compiler can determine the length of
the buffers involved.

-fno-strict-overflow instructs GCC to not make optimizations based on
the assumption that signed arithmetic will wrap around on overflow (e.g.
(short)0x7FFF + 1 == 0). This prevents the optimizer from doing some
unexpected things. Further improvements should trap signed overflows and
reduce the use of signed to refer to naturally unsigned quantities.

I did not set -fPIE (position independent executables). The critical
function of Open/LibreSSL is as a library, not an executable.

Tested on Ubuntu Linux 14.04.1 LTS, OS X 10.10.1 with "make check".

Signed-off-by: Jim Barlow <jim@purplerock.ca>
Where available, enable stack smashing protection, fortify source,
no-strict-overflow, and read only relocations.

Many Linux distributions automatically enable most of these options.
They are no brainers. The difference introduced here is in asking for a
few more aggressive options. An option to disable the more aggressive
options is provided (--disable-hardening). When set, configure will fall
back to the default CFLAGS on the system - in many cases that will still
be hardened. There is no point in going further than that.

Options enabled are:

-fstack-protector-strong is a relatively new GCC-4.9 feature that is
supposed to give a better balance between performance and protection.
-all is considered too aggressive, but was used in Chromium and other
security critical systems until -strong became available. Follow their
lead and use -strong when possible. clang 6.0 supports -all but not
-strong.

_FORTIFY_SOURCE replaces certain unsafe C str* and mem* functions with
more robust equivalents when the compiler can determine the length of
the buffers involved.

-fno-strict-overflow instructs GCC to not make optimizations based on
the assumption that signed arithmetic will wrap around on overflow (e.g.
(short)0x7FFF + 1 == 0). This prevents the optimizer from doing some
unexpected things. Further improvements should trap signed overflows and
reduce the use of signed to refer to naturally unsigned quantities.

I did not set -fPIE (position independent executables). The critical
function of Open/LibreSSL is as a library, not an executable.

Tested on Ubuntu Linux 14.04.1 LTS, OS X 10.10.1 with "make check".

The code added to m4/ is GPLv3 but con

Signed-off-by: Jim Barlow <jim@purplerock.ca>
@busterb
Copy link
Contributor

busterb commented Dec 31, 2014

This failed on every Travis build. That's not very useful behavior. Is the claim here Ubuntu 12.04 (which Linux Travis builders are based on) and OS X do not have stack protector support? I don't think that's true.

I'm not certain if this check is doing the right thing, or if the absence or presence of the compiler flag really means that stack hardening is not enabled. It's actually more ideal if the system compiler just enables it without any special flags, no?

I think a lot of distributions already enable stack protector by default anyway (and you might want --fstack-protector-strong, or --fstack-protector-all)

http://en.wikipedia.org/wiki/Buffer_overflow_protection#GNU_Compiler_Collection_.28GCC.29

So, who's the target for automatically setting these flags? I'm not certain its something we can judge very accurately from a compiler test. Maybe we could just update the README with some examples?

@busterb
Copy link
Contributor

busterb commented Dec 31, 2014

Ah - all of the gcc builds got detected as clang for some reason too.

In the actual clang case, wouldn't it make sense to try some of its -fsanitize options in that case as well?

@jbarlow83
Copy link
Contributor Author

Some distributions don't enable stack protector. Embedded distributions are
particularly lazy about this sort of thing and also more likely to use
older GCC versions.

Why do this? While most distributions now enable stack protection by
default, many don't especially in the embedded space where some rather
important devices like routers tend to be developed, usually with older
versions of everything. They usually set -fstack-protector, but not -strong
or -all, which are appropriate for code facing the public internet. It make
sense to complain if necessary hardening is not available. I also believe
that read only relocations are not generally enabled (and in some cases,
they can't be), similarly no-strict-overflow.

This works for me on Ubuntu 14.04 (GCC) and OS X 10.10.1 (Clang). The
compiler check works, it just incorrectly reports the text string "CLANG"
instead of the variable $CLANG.

It seems that ./configure is running twice. Note that in the first pass, on
each test, all of the tests I added work as expected. See lines 433-440 on
job 10.1 for example. It then fails on the apparent second run of
./configure around line 666 of the output. Why does this occur? I'm not
quite sure what the reasoning is for this second pass of configure. It
seems to be related to "make distcheck" and not a part of "make check" or
"make".

On Tue, Dec 30, 2014 at 7:47 PM, Brent Cook notifications@github.com
wrote:

Ah - all of the gcc builds got detected as clang for some reason too.

In the actual clang case, wouldn't it make sense to try some of its
-fsanitize options in that case as well?


Reply to this email directly or view it on GitHub
#53 (comment)
.

@jbarlow83
Copy link
Contributor Author

I see it now - my addition of the scripts/ folder wasn't properly added to Makefile.am, so it didn't get through distcheck.

Regarding -fsanitize, the performance penalty for some of these options is quite stiff (50% or more). As an extended test mode these options would make sense, but they may be too expensive for a release build.

@busterb
Copy link
Contributor

busterb commented Jan 1, 2015

Thanks, I do like the results here!

A few more comments:

I think we need something in the scripts/wrap-compiler-for-flag-check file that explicitly says something like 'This file is in the public domain.' right at the top, based on the README here: https://github.com/kmcallister/autoharden

Have you looked at what libsndfile does? https://github.com/erikd/libsndfile/tree/master/Scripts

I'm not sure if some of the comments from @ToddMiller1 / @millert at http://mainisusuallyafunction.blogspot.com/2012/05/automatic-binary-hardening-with.html have been addressed. There don't seem to have been any commits to the repository since that blog post.

@jbarlow83
Copy link
Contributor Author

On Thu, Jan 1, 2015 at 10:33 AM, Brent Cook notifications@github.com
wrote:

Thanks, I do like the results here!

A few more comments:

I think we need something in the scripts/wrap-compiler-for-flag-check file
that explicitly says something like 'This file is in the public domain.'
right at the top, based on the README here:
https://github.com/kmcallister/autoharden

Have you looked at what libsndfile does?
https://github.com/erikd/libsndfile/tree/master/Scripts

Do you mean their "sanitize" script for clang? That looks like a good way
to make sanitize more available for testing.

I'm not sure if some of the comments from @ToddMiller1
https://github.com/ToddMiller1 / @millert https://github.com/millert
at
http://mainisusuallyafunction.blogspot.com/2012/05/automatic-binary-hardening-with.html
have been addressed. There don't seem to have been any commits to the
repository since that blog post.

Re @ToddMiller1: His first comment was about getting -fPIE to work on
GCC/OS X 10.6. Since libressl is mainly a library I don't think -fPIE will
add much protection unless the openssl binary is statically linked.

Second comment is on improving the wrap-compiler-for-flag-check situation
using autoconf intrinsics.
However, it appears that the AC_LANG_WERROR([on/off/pop]) was proposed but
not implemented, so you have to manually implement the stack in a hacky way
that depends on internal variables in the AC_LANG_WERROR macro. It also
looks like OS X is not completely warning free so just setting
AC_LANG_WERROR in generally is not an option yet.

Another finding was that wrap-compiler-for-flag-check is mainly dealing
with a pre-clang 5.1 problem (about 1.5 years old). I think libsndfile does
not check for this case and probably gets the flags wrong on clang. I
thought the best thing to do here was document this fact so this workaround
can be reevaluated and removed when clang 5.1 is too old to worry about.

Reply to this email directly or view it on GitHub
#53 (comment)
.

@busterb busterb merged commit 813e7bd into libressl:master Jan 2, 2015
@busterb
Copy link
Contributor

busterb commented Jan 2, 2015

Thanks for the explanation and for working on this.

@busterb
Copy link
Contributor

busterb commented Jan 7, 2015

I made a couple of fixes changing the == compares to = to support systems running posix shells f2d68c7

I also found some issues building with GCC 4.2. Given the potential for other unexpected things to go haywire, I disabled the ERROR message for now in ec81c28 . We can re-enable that part later after the hardening flags have had a release for getting some field testing/feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants