maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

> commit feab48657528b9bb40fb035d51bee28d93710c1e
> Author: Sergei Golubchik <serg@xxxxxxxxxxx>
> Date:   Mon Jun 16 21:39:09 2014 +0200
> 
>     MDEV-5730 enhance security using special compilation options
>     
>     -Wl,-z,relro,-z,now
>     -pie
>     -fstack-protector --param=ssp-buffer-size=4
>     -D_FORTIFY_SOURCE=2

The patch is ok to push with a clarified comment as requested below.

I have checked the compiler/linker options added and commented some on them
below.

> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index a24f000..5b0d348 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -201,6 +201,20 @@ IF (WITH_ASAN)
>    ENDIF()
>  ENDIF()
>  
> +OPTION(SECURITY_HARDENED "Use security-enhancing compiler features (stack protector, relro, etc)" ON)

So these are on by default.
This means that this patch will cause some performance impact.

So having them on by default is a compromise between performance and security.
You should probably include in the commit comment a discussion of why the
security viewpoint was prefered over the performance viewpoint in this case
(or a comment in the cmake file if you prefer).

The proper way to handle a patch like this would be to measure the performance
impact, eg. for single-threaded performance with some various simple sysbench
loads. However, it is quite hard to do such measurements, as you need a setup
that has low enough noise in results to detect sub-percentage differences of
performance. I have had some success by locking processes to a single core and
using `perf` measurements of cycles spent (rather than wall-clock time), but
even so the results can be tricky to interpret. So it may be too much work for
a small patch like this.

(And even if the performance improvement can be measured, how can we measure
the security impact to weight them against one another?)

My personal opinion is that I do not buy into this "security" mindset much. I
do not think these options improve security significantly in practise for most
users. So while the performance impact is probably also not very significant,
why accept it at all (by default)? But the opposite viewpoint exists and is
valid as well. So I think this patch is ok if you add a discussion of why it
was chosen to do it this way.

(Even if the reason is "Most distros do it" - at least then it is documented
what the reason is).

Also, the option should mention the possible performance impact. Suggestion:

"Use compiler options (stack protector, relro, etc) that may slightly enhance
security of the resulting binary, at the possible cost of some small
performance impact."

> +IF(SECURITY_HARDENED)
> +  # security-enhancing flags
> +  MY_CHECK_AND_SET_COMPILER_FLAG("-pie -fPIC")

-fPIC can have a significant impact on performance on some platforms. For
example on 32-bit x86, it requires extra instructions at the head of every
function, and it makes one less register available for code generation on a
CPU already starved for registers. On 64-bit x86, the impact is much smaller,
as the architecture was enhanced to better support -fPIC.

However, I seem to recall that the server is anyway built with -fPIC, for
libmysqld or something like that? In which case this has no extra impact.

> +  MY_CHECK_AND_SET_COMPILER_FLAG("-Wl,-z,relro,-z,now")

This option seems to cause all relocations to happen at load time, rather than
lazily on-demand. The impact should be higher memory usage and some extra CPU
time for these relocations, many of which may never be needed.

This option will have the biggest effect on the client programs probably, as
server startup is already fairly heavy. Probably even for client programs the
added cost will be not very significant, as even the client programs are
usually called to do significant processing (eg. connect to a database and so
on).

I guess these options would mostly hurt small utility programs for shell
scripts, like `mv` or so.

> +  MY_CHECK_AND_SET_COMPILER_FLAG("-fstack-protector --param=ssp-buffer-size=4")

This also adds extra code and memory loads to each function, so it will have
some impact to performance. It is probably the most significant impact of all
the added options, though without detailed analysis this is only guesswork, of
course.

> +  # sometimes _FORTIFY_SOURCE is predefined
> +  INCLUDE(CheckSymbolExists)
> +  CHECK_SYMBOL_EXISTS(_FORTIFY_SOURCE "" HAVE_FORTIFY_SOURCE)
> +  IF(NOT HAVE_FORTIFY_SOURCE)
> +    ADD_DEFINITIONS(-D_FORTIFY_SOURCE=2)
> +  ENDIF()
> +ENDIF()

FORTIFY_SOURCE also adds some runtime overhead, but it is not very much, it is
an extra argument to memcpy() and friends about destination buffer size.
Probably the way or code is written, there will be few places where this
option makes any difference, so the performance impact does not seem likely to
be very significant.

Hope this helps,

 - Kristian.