maria-developers team mailing list archive

[Reindl Harald <h.reindl@xxxxxxxxxxxxx>]

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@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

Attachment: signature.asc
Description: OpenPGP digital signature