[Maria-developers] Review of MDEV-5730: enhance security using special compilation options
commit feab48657528b9bb40fb035d51bee28d93710c1e Author: Sergei Golubchik <serg@mariadb.org> 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.
please make sure that explicit set options are not overriden in case of GCC the last option wins so if you set "-fstack-protector" by add it to the flags you disable "-fstack-protector-strong" from below which depends on the GCC version and is now default in Fedora as example export CFLAGS="%{optflags} -O3 -fstack-protector-strong --param=ssp-buffer-size=4 -fPIC -fomit-frame-pointer -fno-exceptions -ffixed-ebp -fwrapv -fno-strict-aliasing -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE" export CXXFLAGS="$CFLAGS" export FFLAGS="$CFLAGS" export FCFLAGS="$CFLAGS" export LDFLAGS="-Wl,-z,now -Wl,-z,relro,-z,noexecstack -pie" export SH_LDFLAGS="$LDFLAGS" Am 24.06.2014 10:02, schrieb Kristian Nielsen:
commit feab48657528b9bb40fb035d51bee28d93710c1e Author: Sergei Golubchik <serg@mariadb.org> 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
Hi, Reindl! On Jun 24, Reindl Harald wrote:
please make sure that explicit set options are not overriden
in case of GCC the last option wins
so if you set "-fstack-protector" by add it to the flags you disable "-fstack-protector-strong" from below which depends on the GCC version and is now default in Fedora as example
export CFLAGS="%{optflags} -O3 -fstack-protector-strong --param=ssp-buffer-size=4 -fPIC -fomit-frame-pointer -fno-exceptions -ffixed-ebp -fwrapv -fno-strict-aliasing -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE" export CXXFLAGS="$CFLAGS" export FFLAGS="$CFLAGS" export FCFLAGS="$CFLAGS" export LDFLAGS="-Wl,-z,now -Wl,-z,relro,-z,noexecstack -pie" export SH_LDFLAGS="$LDFLAGS"
Yes, I believe explicitly specified options are added last, so they'll override whatever is set in cmakefiles. Either way, our CMakeLists.txt won't try to figure out what options (from the explicitly specified set) are incompatible or conflicting with automatically added. But you can always configure with cmake -DSECURITY_HARDENED=OFF and cmake won't add any of these options automatically. Regards, Sergei
participants (3)
-
Kristian Nielsen -
Reindl Harald -
Sergei Golubchik