Opened 2 years ago

Closed 23 months ago

Last modified 19 months ago

#5210 closed defect (fixed)

Enable gcc and ld hardening by default in 0.2.3.x

Reported by: ioerror Owned by: ioerror
Priority: major Milestone: Tor: 0.2.3.x-final
Component: Tor Version:
Keywords: security tor-client Cc: arma, nickm, nextgens@…, tails@…, asn
Actual Points: Parent ID:
Points:

Description

Per a discussion with Roger at the Seattle hackfest I propose we enable gcc and ld hardening by default in 0.2.3.x when the ./configure is built with no options.

Shall I submit a patch that flips the options? So rather than --enable-gcc-hardening, we say --disable-gcc-hardening?

Or shall we just load the options and call it a day?

If someone else wants to bell the cat, I'm happy to defer to them.

Child Tickets

Attachments (1)

patchv2.diff (1.4 KB) - added by nextgens 2 years ago.
patch against current master - 4ade55ecb9e6c3144dc6ed192fae0f613acfb467

Download all attachments as: .zip

Change History (27)

comment:2 Changed 2 years ago by Sebastian

Implementing this would make a small part of #5024 implemented once we switch to tor 0.2.3.x for TBB, but not #5024 means all other stuff we bundle I think.

comment:3 Changed 2 years ago by nickm

ioerror: I would totally welcome a switch to --enable-gcc-hardening becoming the new default.

comment:4 Changed 2 years ago by cypherpunks

The tor package from http://deb.torproject.org/torproject.org is already compiled using hardening options:

"Full RELRO Canary found NX enabled PIE enabled"

comment:5 Changed 2 years ago by Sebastian

Yes, the Debian packaging sets the appropriate configure commands by default. In theory all other packagers should do so too, but because they don't, choosing this default for them when possible makes sense.

comment:6 Changed 2 years ago by arma

nextgens gives us this patch:

diff --git a/changes/bug5210 b/changes/bug5210
new file mode 100644
index 0000000..b07e7f1
--- /dev/null
+++ b/changes/bug5210
@@ -0,0 +1,2 @@
+  o Security fixes:
+    - Enable gcc and ld hardening by default. Fixes bug 5210.
diff --git a/configure.in b/configure.in
index 7415ce8..23dcc07 100644
--- a/configure.in
+++ b/configure.in
@@ -122,19 +122,23 @@ dnl -D_FORTIFY_SOURCE=2 -fstack-protector-all
 dnl Others suggest '/gs /safeseh /nxcompat /dynamicbase' for non-gcc on Windows
 dnl This requires that we use gcc and that we add -O2 to the CFLAGS.
 AC_ARG_ENABLE(gcc-hardening,
-     AS_HELP_STRING(--enable-gcc-hardening, enable compiler security checks),
+    AS_HELP_STRING(--disable-gcc-hardening, disable compiler security checks),
+    [enableval=no;],
+    [enableval=yes;])
 [if test x$enableval = xyes; then
     CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2 -fstack-protector-all"
     CFLAGS="$CFLAGS -fwrapv -fPIE -Wstack-protector"
     CFLAGS="$CFLAGS --param ssp-buffer-size=1"
     LDFLAGS="$LDFLAGS -pie"
-fi])
+fi]
 
 dnl Linker hardening options
 dnl Currently these options are ELF specific - you can't use this with MacOSX
 AC_ARG_ENABLE(linker-hardening,
-        AS_HELP_STRING(--enable-linker-hardening, enable linker security fixups
-[if test x$enableval = xyes; then
+    AS_HELP_STRING(--disable-linker-hardening, disable linker security fixups),
+    [enableval=no;],
+    [enableval=yes;])
+AC_CHECK_HEADER([elf.h], [if test x$enableval = xyes; then
     LDFLAGS="$LDFLAGS -z relro -z now"
 fi])

Changed 2 years ago by nextgens

patch against current master - 4ade55ecb9e6c3144dc6ed192fae0f613acfb467

comment:7 Changed 2 years ago by Sebastian

The patch breaks currently existing setups in a subtle way, so we can't apply it as is. If you call configure with --enable-gcc-hardening, the hardening gets turned off silently - obviously that's bad.

comment:8 Changed 2 years ago by Sebastian

  • Status changed from new to needs_revision

comment:9 Changed 2 years ago by nextgens

  • Cc nextgens@… added

This is the working version of the above:
https://github.com/nextgens/Tor/commit/62f3121a3d209fb4f826988d53b1aac93842502c

now both --enable and --disable should now work as intended.

There's two caveats with this patch still:

  • it doesn't detect whether gcc is in use and assumes that the compiler will ignore the flags it doesn't 'support'
  • it checks for the presence of the header 'elf.h' rather than the format of the executable produced (ie: the auto-detection magic won't work if you cross-compile or happen to have the elf headers installed)

Detecting whether a particular flag is supported by the compiler could be done with a macro such as:
http://ac-archive.sourceforge.net/ac-archive/ax_cflags_gcc_option.html

comment:10 Changed 2 years ago by Sebastian

That does indeed seem to work better, but I can confirm that building on clang, which doesn't support --param ssp-buffer-size=1 and -pie, gives a ton of warnings.

comment:11 Changed 2 years ago by nickm

Okay, so what would be a way to detect this situation? We could just say, "If clang, don't use these options," but that wouldn't be so nice if some future version of clang starts supporting them. Other ideas?

comment:12 Changed 2 years ago by T(A)ILS developers

  • Cc tails@… added

comment:13 Changed 2 years ago by nickm

  • Status changed from needs_revision to needs_review

I tried building something with AX_CFLAGS_GCC_OPTION, but it was too buggy; it didn't support --param ssp-buffer-size=1. So instead, I've hacked together a "bug5210" in my public repo, based on the one from nextgetns. Tested on linux with gcc and clang; then tried on OSX, where I had to add some more options to make clang shut up.

Please test and review and think about this stuff.

comment:14 Changed 2 years ago by Sebastian

  • Cc asn added

asn probably has something to add here, based on experience with obfsproxy

comment:15 follow-up: Changed 2 years ago by kmcallister

Hi, you may be interested in my recent article about automatic binary hardening with Autoconf:

http://mainisusuallyafunction.blogspot.com/2012/05/automatic-binary-hardening-with.html

comment:16 Changed 2 years ago by Sebastian

Quite interesting, this certainly gets around the clang annoyances. How portable do you expect your approach to be?

comment:17 Changed 2 years ago by Sebastian

(What I find interesting are the performance implications. 30% slowdown on Debian does seem somewhat drastic)

comment:18 in reply to: ↑ 15 Changed 2 years ago by nickm

Replying to kmcallister:

Hi, you may be interested in my recent article about automatic binary hardening with Autoconf:

http://mainisusuallyafunction.blogspot.com/2012/05/automatic-binary-hardening-with.html

Looks like they've converged on the same options we have. That much is good.

I'm not convinced that explicitly grepping for a warning from clang is such a good idea: warnings change in the presence of localization.

The slowdown business is something we'll need to deal with in practice as we go. If stack-protector is hideously slow in some configurations, we might need to turn it off. If -fPIE is a big deal, we may need to add in a -fomit-frame-pointer for production builds of critical-path pieces of the code.

Incidentally, I don't think we really get protection from -fPIE unless any static library we link against is also built with -fPIE, right?

Some of this won't work on windows unless we do yet more magic; said magic is however a thing for a separate ticket.

comment:19 Changed 23 months ago by nickm

  • Status changed from needs_review to needs_revision

Yuck. For me, this breaks on mingw+windows for some reason. Investigating... it looks like the -pie linker option breaks stuff there, but not immediately in a way that the configure script notices. (Stuff goes okay, and then when it wants to link libevent, it dies in a fire) I wonder if this is just my junky mingw setup, or if others have this issue with this branch too?

comment:20 Changed 23 months ago by nickm

oh hey. The obvious fix for that would be to do these tests _after_ we know what the libraries are.

comment:21 Changed 23 months ago by nickm

  • Status changed from needs_revision to needs_review

Okay, I think I have something that works added to my branch "bug5210" in my public repo. I'll try it in a few more places before I merge it.

comment:22 Changed 23 months ago by nickm

Ugh. The "-pie" part still makes the windows build crap out -- it doesn't detect that on windows, '-pie' is hopeless.. I have just gone and hard-disabled it when we're building for windows. Branch "bug5210" in my public repo is still the one to look at.

comment:23 Changed 23 months ago by Sebastian

Tested this on osx 10.7 with llvm-gcc 4.2 and clang 3.1, seems to work fine on both. I'll start a compile of clang 3.0 when I have power again and try compiling tor with clang 3.0 with that, too

comment:24 Changed 23 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

I've tested it on all my VMs, and with clang 3. I am calling this ready to go. Getting more of it working on windows is a task for #5400.

Merging into 0.2.3. Thanks, everyone!

comment:25 Changed 19 months ago by nickm

  • Keywords tor-client added

comment:26 Changed 19 months ago by nickm

  • Component changed from Tor Client to Tor
Note: See TracTickets for help on using tickets.