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 <s...@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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
