GCC recommendation: -fstack-protector

[bcearl]

I recommend to include the option -fstack-protector to the UNIX makefile. Many distributions (including Ubuntu) use it by default, but some others may not.

Why does it make sense?
On the one hand the Bitcoin client is supposed to be online and connected with many peers. On the other hand it handles data that must be kept secret at all costs.

Thus the client processes messages from unknown peers all the time. If there is a bug in processing, there could be buffer overflows. Those could be exploited to take over the client.

There are three common measurements at the moment against such attacks:

- NX bit: a CPU feature that prevents data from being interpreted as code
- address randomization: the Linux kernel gives each process different stack addresses every time
- GCC stack protector: buffers on stack are surrounded by test data which makes it hard to overflow a buffer without being detected

While the first two are configured by hardware and OS, the third one is configured at compile time.

Code:

-fstack-protector
           Emit extra code to check for buffer overflows, such as stack
           smashing attacks.  This is done by adding a guard variable to
           functions with vulnerable objects.  This includes functions that
           call alloca, and functions with buffers larger than 8 bytes.  The
           guards are initialized when a function is entered and then checked
           when the function exits.  If a guard check fails, an error message
           is printed and the program exits.

           NOTE: In Ubuntu 6.10 and later versions this option is enabled by
           default for C, C++, ObjC, ObjC++, if none of -fno-stack-protector,
           -nostdlib, nor -ffreestanding are found.

Any disadvantages?

Of course every measurement of this kind affects Performance. But this affects only functions that have buffers of more than 8 bytes.

And if you have built it on Ubuntu until now, you have had it activated anyway without knowing.

[1715286410]

1715286410

[1715286410]

1715286410

[1715286410]

1715286410

[wumpus]

Does it affect performance much?

If not, please submit a pull request, everything that makes bitcoind more safe against future exploits is good.

[bcearl]

Does it affect performance much?

If not, please submit a pull request, everything that makes bitcoind more safe against future exploits is good.

I could not see any performance differences, but I wanted to hear some more opinions.

I didn't commit that yet because I don't know how such a change would affect people who want to use a compiler other than GCC. Will there be something like a ./configure script in the future? I think the flag must be set at the point where you know what compiler is used.

[dikidera]

<jtaylor> ubuntu enables -fstack-protector in hardening-wrapper

Some random guy on IRC ^

[Matt Corallo]

In Ubuntu 6.10 and later versions this option is enabled by default

Bitcoin is build on 10.04 LTS, so it looks like we are using it.

[wumpus]

I've added the option to the build script of my client, seems to work OK in Linux and Mingw builds.

I did notice that it is important to provide the option to CXXFLAGS as well as LDFLAGS, otherwise it will give errors when linking.

[bcearl]

In Ubuntu 6.10 and later versions this option is enabled by default

Bitcoin is build on 10.04 LTS, so it looks like we are using it.

That's great, so everybody who uses the binary from bitcoin.org is already having it.

[ampkZjWDQcqT]

The root of this problem resides in Bitcoin not having a configure script.

[Matt Corallo]

The root of this problem resides in Bitcoin not having a configure script.

That has absolutely nothing to do with this problem.

[bcearl]

The root of this problem resides in Bitcoin not having a configure script.

That has absolutely nothing to do with this problem.

It has a lot. The configure script's job is to find out what's available. It would activate that option depending on the compiler that is used.

[Matt Corallo]

It has a lot. The configure script's job is to find out what's available. It would activate that option depending on the compiler that is used.

Since Bitcoin only supports gcc, its not a problem.

[twobits]

The root of this problem resides in Bitcoin not having a configure script.

That has absolutely nothing to do with this problem.

It has a lot. The configure script's job is to find out what's available. It would activate that option depending on the compiler that is used.

It uses makefiles?  Could just have it be a line in the makefile that someone can comment in or out for now.

[bcearl]

The root of this problem resides in Bitcoin not having a configure script.

That has absolutely nothing to do with this problem.

It has a lot. The configure script's job is to find out what's available. It would activate that option depending on the compiler that is used.

It uses makefiles?  Could just have it be a line in the makefile that someone can comment in or out for now.

That sucks. The upnp stuff in the makefile makes it a pain already.

[kjj]

As much as I despise working with autoconf, it seriously rocks.

[bcearl]

I don't care what tool you want to use, but use one, please ...

[error]

Oh, and it's not "makefile.unix", it's "Makefile"!

[wumpus]

As much as I hate autoconf, it is going to be integrated in 0.4.x

[Matt Corallo]

Oh, and it's not "makefile.unix", it's "Makefile"!

Not if you support three different OSes and more build environments.

I don't care what tool you want to use, but use one, please ...

We do, we use make.  For our purposes, it works fine if you have a sane build environment.  That said, it would be great to switch to something more flexible.  Though its not particularly high on the priorities list, if someone has time to do it, by all means please do, until then...make it is.

[Matt Corallo]

As much as I hate autoconf, it is going to be integrated in 0.4.x

If someone gets around to writing it, yes.  But as it stands, no one is really working on it, it might end up not making it for quite some time, and definitely won't be in in 0.4.

[wumpus]

If someone gets around to writing it, yes.  But as it stands, no one is really working on it, it might end up not making it for quite some time, and definitely won't be in in 0.4.

Huh, I understood that there is already a pull request for it, and it was planned to be integrated.

[Matt Corallo]

Huh, I understood that there is already a pull request for it, and it was planned to be integrated.

That was the plan, but the guy who wrote it has given up communicating and updating it with various suggestions.  Its also (from what I hear from people with more autotools experience) very poorly coded. So it sits in a "there is no way we are going to pull this, please update it" state.

[gmaxwell]

It's probably worth mentioning that the default static linking defeats ASLR, ... which is probably a more important security enhancement than -fstack-protector.

The default build should probably not statically link— if you've got the libraries then you can dynamically link them.  It should also be compiled -pie -fPIE.

There appears to be good general hardening advice here: http://wiki.debian.org/Hardening

[Matt Corallo]

The default build should probably not statically link

Oh god how I'd love to not have to static link anything or build anything for Linux anymore.  But as it stands now, thats not really an option.  First, we use a development release of wx, meaning it is in no package repos anywhere so you cant require people to install that.  Secondly, we use ECC which is stripped out of openssl on all redhat-based distros due to copyright concerns.  Lastly, we don't have distro-packages so there is no way for us to create package dependencies for bitcoin aside from instillation instructions which gets very, very user unfriendly.  When we make a switch to QT all that can go away and we can get into distro packages for everything (except redhat-based distros...).  If you are talking about on windows, well, what other option is there? Either you ship your own libraries, in which case the argument is invalid, or you ship static, like every other sane program on Windows.  Windows just doesn't have a way to get dependencies like Linux (really a shame, you'd think after this many years Microsoft and Apple could have caught up with Linux on this one).

It should also be compiled -pie -fPIE.

There appears to be good general hardening advice here: http://wiki.debian.org/Hardening

By all means, pull request away

[wumpus]

It's indeed a good thing to static-link the distributed binaries. Nearly all projects that distribute distribution-agnostic binaries for Linux distribute a statically-linked version. The remaining ones package the .so's, as a poor-mans static linking

On the other hand, when building from source (and building distribution-specific) it doesn't have to default to static linking (and can include all the PIE you want :p). That's currently just an artifact of the inflexibility of "make". As mentioned, a configurable build system like autotools would address this by providing options such as --with-static.

BTW here some links about the PIE thing, I found it pretty interesting:

https://secure.wikimedia.org/wikipedia/en/wiki/Position-independent_code
https://wiki.ubuntu.com/Security/Features#Built%20as%20PIE

Most notably:
"PIE has a large (5-10%) performance penalty on architectures with small numbers of general registers (e.g. x86), ... PIE on x86_64 does not have the same penalties, and will eventually be made the default, but more testing is required"

So it should probably be enabled by default for x86_64 but not x86_32.

[gmaxwell]

On the other hand, when building from source (and building distribution-specific) it doesn't have to default to static linking (and can include all the PIE you want :p). That's currently just an artifact of the inflexibility of "make". As mentioned, a configurable build system like autotools would address this by providing options such as --with-static.

Right, this was what I meant.  Obviously the shipped binaries will need to be statically linked, alas.   Thats still no reason to leave everyone else statically linked.

You don't need autotools for this: just add another target "make static-dist", or whatever.

For me, on Fedora, the static builds fail because the distribution doesn't ship with static versions of pretty much _anything_ because the overuse of static libraries has been a reoccurring security nightmare.  So, really, for some people the failure to separate static and not static builds means that building simply doesn't work.

Oh, while we're talking about this— it was claimed upthread that bitcoin should already have -fstack-protector — I don't know about Ubuntu, but in fedora -fstack-protector is set via the default RPM cflags, and _not_ by modifying GCC.  If the same is true on ubuntu, then bitcoind doesn't have it.  Someone with ubuntu handy ought to compile the examples from that debian page and see if gcc in ubuntu is really providing the protection by default.

Most notably:
"PIE has a large (5-10%) performance penalty on architectures with small numbers of general registers (e.g. x86), ... PIE on x86_64 does not have the same penalties, and will eventually be made the default, but more testing is required"
So it should probably be enabled by default for x86_64 but not x86_32.

This is only the case for tight loop register starved cpu bound code.  Bitcoin is usually I/O bound.   I've been running bitcoin in _valgrind_  (which run most things things at 1/10th to 1/20th speed) and hardly notice any difference except while syncing up the blockchain.

I'm seriously doubtful that PIE is going to make a noticeable performance difference.  Also, the libraries on any system are all already fpic, and all the crypto stuff is already in libraries,  Moreover, there is a pretty easy answer to someone who wants more performance: use x86_64, which is generally true (esp for C++ code) with or without PIE.

[wumpus]

Moreover, there is a pretty easy answer to someone who wants more performance: use x86_64, which is generally true (esp for C++ code) with or without PIE.

One would think that it is mainly the 32-bit systems that need every bit of extra performance

The only place in which performance matters, indeed, is the initial sync-up with the block chain. After that, traffic is so low that crypto speed doesn't matter much.

And I wouldn't be surprised either if much of the slowless on initial block chain download was caused by disk access, not CPU load. I once tried to bootstrap bitcoin on an USB stick. Bad idea  

You don't need autotools for this: just add another target "make static-dist", or whatever.

Feel free to send a pull request.

[bcearl]

We do, we use make.  For our purposes, it works fine if you have a sane build environment.

An environment with uupnp is not sane.