Message ID | 1457445064-7107-2-git-send-email-nix@esperi.org.uk |
---|---|
State | New |
Headers | show |
On 08 Mar 2016 13:50, Nix wrote: > +AC_ARG_ENABLE([stack-protector], > + AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@], 8 spaces -> tabs, and re-align the 2nd arg > + [Detect stack overflows in glibc functions, either with local buffers (yes), or with those plus arrays (strong), or all functions (all)]), is this really necessary ? can't we just say something like "pass -fstack-protector[-all|-strong]" and leave the rest to the gcc manual ? > + [enable_stack_protector=no]) should we default this to auto-detecting strong ? or at least make that a follow up patch in this series ? > +case x"$enable_stack_protector" in > + xall|xyes|xno|xstrong) ;; don't need the x prefix. that historical wart is related to empty string compares with the test command, not the case command. > + *) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector]);; don't indent case statements > +if test x$libc_cv_ssp = xyes; then quote the LHS -- cache vars can be set via the env or config.site files > + no_stack_protector=-fno-stack-protector indent with two spaces > +if test x$enable_stack_protector = xyes && test $libc_cv_ssp = yes; then > + stack_protector=-fstack-protector > +elif test x$enable_stack_protector = xall && test $libc_cv_ssp_all = yes; then > + stack_protector=-fstack-protector-all > +elif test x$enable_stack_protector = xstrong && test $libc_cv_ssp_strong = yes; then > + stack_protector=-fstack-protector-strong > +fi all of these strings should be quoted imo. they were before you changed things too. -mike
On 9 Mar 2016, Mike Frysinger said: > On 08 Mar 2016 13:50, Nix wrote: >> +AC_ARG_ENABLE([stack-protector], >> + AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@], > > 8 spaces -> tabs, and re-align the 2nd arg Fixed. Second arg looks perfectly alright to me, though: it's aligned in my tree, and in the patch I emailed out. Looks like your MUA has done something to it. (Though it was aligned with spaces.) >> + [Detect stack overflows in glibc functions, either with local buffers (yes), or with those plus arrays (strong), or all functions (all)]), > > is this really necessary ? can't we just say something like "pass > -fstack-protector[-all|-strong]" and leave the rest to the gcc manual ? Er, yes, and Andreas even asked me to do that, and I thought oh yes that's a good idea and then didn't do it. Um. It's done now. How about [Use -fstack-protector[-all|-strong] to detect glibc stack overflows] so that at least it becomes clear what it means to 'protect' the stack without having to dive into the manual? I'd like to say a tiny bit more than just say "pass": to what? why? (but maybe the last five words are redundant after all. I dunno. Still, even if you take them off it doesn't fit on one line in configure --help output in my tests, so the extra words don't actually *cost* anything.) >> + [enable_stack_protector=no]) > > should we default this to auto-detecting strong ? > or at least make that a follow up patch in this series ? I thought that might be too contentious! but if people want to flip the default, it's fine by me. (Just don't flip it to -all: that's probably best left to hardened distros, since it probably does have a real performance cost...) >> +case x"$enable_stack_protector" in >> + xall|xyes|xno|xstrong) ;; > > don't need the x prefix. that historical wart is related to empty > string compares with the test command, not the case command. It's historical terror. (And I have distinct memories of seeing problems with case on ancient HP-UX boxes, HP-UX 9 or thereabouts. This is obviously relevant since people build glibc on HP-UX 9 boxes *all the time*.) Fixed. :) >> + *) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector]);; > > don't indent case statements Ew, really? ... gods, you're right, none of them are indented. I never noticed that. Fixed. >> +if test x$libc_cv_ssp = xyes; then > > quote the LHS -- cache vars can be set via the env or config.site files Fixed. (And if I'm quoting it I don't need to x it: fixed that, too.) >> + no_stack_protector=-fno-stack-protector > > indent with two spaces Oops, yes. (Also fixed a couple of places in patch 9 that made the same mistake.) >> +if test x$enable_stack_protector = xyes && test $libc_cv_ssp = yes; then >> + stack_protector=-fstack-protector >> +elif test x$enable_stack_protector = xall && test $libc_cv_ssp_all = yes; then >> + stack_protector=-fstack-protector-all >> +elif test x$enable_stack_protector = xstrong && test $libc_cv_ssp_strong = yes; then >> + stack_protector=-fstack-protector-strong >> +fi > > all of these strings should be quoted imo. they were before you changed > things too. Yeah, I removed the quotation because it was useless -- the value of $enable_stack_protector is constrained by the check further up, and you don't need to quote a single word with no shell metacharacters in it (and - is not a shell metacharacter). The former argument is debatable -- someone might remove the check further up in later maintenance without even noticing this code exists -- but the latter is not, really: any shell in which the string '-fstack-protector' needed quoting would not be a shell that could run configure at all. So, quoted all the $libc_cv_ssps and $enable_stack_protectors, de-x'ed the latter, but not yet quoted the assignments, because... what for? Also quoted and de-x'ed the stuff in the checks added in patch 9, for the same reason.
On 9 Mar 2016, nix@esperi.org.uk uttered the following:
> [Use -fstack-protector[-all|-strong] to detect glibc stack overflows]
Obviously this was stupid. That's not what -fstack-protector does!
What was I thinking...
This is better:
[Use -fstack-protector[-all|-strong] to detect glibc buffer overflows]
diff --git a/INSTALL b/INSTALL index a0d0c05..3d55585 100644 --- a/INSTALL +++ b/INSTALL @@ -141,20 +141,31 @@ will be used, and CFLAGS sets optimization options for the compiler. '--enable-lock-elision=yes' Enable lock elision for pthread mutexes by default. -'--enable-pt_chown' - The file 'pt_chown' is a helper binary for 'grantpt' (*note - Pseudo-Terminals: Allocation.) that is installed setuid root to fix - up pseudo-terminal ownership. It is not built by default because - systems using the Linux kernel are commonly built with the 'devpts' - filesystem enabled and mounted at '/dev/pts', which manages - pseudo-terminal ownership automatically. By using - '--enable-pt_chown', you may build 'pt_chown' and install it setuid - and owned by 'root'. The use of 'pt_chown' introduces additional - security risks to the system and you should enable it only if you - understand and accept those risks. - -'--disable-werror' - By default, the GNU C Library is built with '-Werror'. If you wish +`--enable-stack-protector' +`--enable-stack-protector=strong' +`--enable-stack-protector=all' + Compile the C library and all other parts of the glibc package + (including the threading and math libraries, NSS modules, and + transliteration modules) using the GCC `-fstack-protector', + `-fstack-protector-strong' or `-fstack-protector-all' options to + detect stack overruns. Only the dynamic linker and a small number + of routines called directly from assembler are excluded from this + protection. + +`--enable-pt_chown' + The file `pt_chown' is a helper binary for `grantpt' (*note + Pseudo-Terminals: Allocation.) that is installed setuid root to + fix up pseudo-terminal ownership. It is not built by default + because systems using the Linux kernel are commonly built with the + `devpts' filesystem enabled and mounted at `/dev/pts', which + manages pseudo-terminal ownership automatically. By using + `--enable-pt_chown', you may build `pt_chown' and install it + setuid and owned by `root'. The use of `pt_chown' introduces + additional security risks to the system and you should enable it + only if you understand and accept those risks. + +`--disable-werror' + By default, the GNU C Library is built with `-Werror'. If you wish to build without this option (for example, if building with a newer version of GCC than this version of the GNU C Library was tested with, so new warnings cause the build with '-Werror' to fail), you diff --git a/configure.ac b/configure.ac index 3c766b7..7b81049 100644 --- a/configure.ac +++ b/configure.ac @@ -232,6 +232,18 @@ AC_ARG_ENABLE([bind-now], [bindnow=no]) AC_SUBST(bindnow) +dnl Build glibc with -fstack-protector, -fstack-protector-all, or +dnl -fstack-protector-strong. +AC_ARG_ENABLE([stack-protector], + AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@], + [Detect stack overflows in glibc functions, either with local buffers (yes), or with those plus arrays (strong), or all functions (all)]), + [enable_stack_protector=$enableval], + [enable_stack_protector=no]) +case x"$enable_stack_protector" in + xall|xyes|xno|xstrong) ;; + *) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector]);; +esac + dnl On some platforms we cannot use dynamic loading. We must provide dnl static NSS modules. AC_ARG_ENABLE([static-nss], @@ -602,6 +614,41 @@ fi test -n "$base_machine" || base_machine=$machine AC_SUBST(base_machine) +AC_CACHE_CHECK(for -fstack-protector, libc_cv_ssp, [dnl +LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector], + [libc_cv_ssp=yes], + [libc_cv_ssp=no]) +]) + +AC_CACHE_CHECK(for -fstack-protector-strong, libc_cv_ssp_strong, [dnl +LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-strong], + [libc_cv_ssp_strong=yes], + [libc_cv_ssp_strong=no]) +]) + +AC_CACHE_CHECK(for -fstack-protector-all, libc_cv_ssp_all, [dnl +LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all], + [libc_cv_ssp_all=yes], + [libc_cv_ssp_all=no]) +]) + +stack_protector= +no_stack_protector= +if test x$libc_cv_ssp = xyes; then + no_stack_protector=-fno-stack-protector +fi + +if test x$enable_stack_protector = xyes && test $libc_cv_ssp = yes; then + stack_protector=-fstack-protector +elif test x$enable_stack_protector = xall && test $libc_cv_ssp_all = yes; then + stack_protector=-fstack-protector-all +elif test x$enable_stack_protector = xstrong && test $libc_cv_ssp_strong = yes; then + stack_protector=-fstack-protector-strong +fi +AC_SUBST(libc_cv_ssp) +AC_SUBST(stack_protector) +AC_SUBST(no_stack_protector) + # For the multi-arch option we need support in the assembler & linker. AC_CACHE_CHECK([for assembler and linker STT_GNU_IFUNC support], libc_cv_ld_gnu_indirect_function, [dnl @@ -1389,26 +1436,6 @@ else fi AC_SUBST(fno_unit_at_a_time) -AC_CACHE_CHECK(for -fstack-protector, libc_cv_ssp, [dnl -LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector], - [libc_cv_ssp=yes], - [libc_cv_ssp=no]) -]) - -AC_CACHE_CHECK(for -fstack-protector-strong, libc_cv_ssp_strong, [dnl -LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-strong], - [libc_cv_ssp_strong=yes], - [libc_cv_ssp_strong=no]) -]) - -stack_protector= -if test "$libc_cv_ssp_strong" = "yes"; then - stack_protector="-fstack-protector-strong" -elif test "$libc_cv_ssp" = "yes"; then - stack_protector="-fstack-protector" -fi -AC_SUBST(stack_protector) - AC_CACHE_CHECK(whether cc puts quotes around section names, libc_cv_have_section_quotes, [cat > conftest.c <<EOF diff --git a/manual/install.texi b/manual/install.texi index b329950..e05424a 100644 --- a/manual/install.texi +++ b/manual/install.texi @@ -170,6 +170,18 @@ time. Consult the @file{timezone} subdirectory for more details. @item --enable-lock-elision=yes Enable lock elision for pthread mutexes by default. +@item --enable-stack-protector +@itemx --enable-stack-protector=strong +@itemx --enable-stack-protector=all +Compile the C library and all other parts of the glibc package +(including the threading and math libraries, NSS modules, and +transliteration modules) using the GCC @option{-fstack-protector}, +@option{-fstack-protector-strong} or @option{-fstack-protector-all} +options to detect stack overruns. Only the dynamic linker and a small +number of routines called directly from assembler are excluded from this +protection. + + @pindex pt_chown @findex grantpt @item --enable-pt_chown
From: Nick Alcock <nick.alcock@oracle.com> This adds =all and =strong, with obvious semantics, and with a rather arbitrarily-chosen default off, which we might well want to change to something stronger once this patch has been tested by people other than me. We don't validate the value of the option yet: that's in a later patch. Nor do we use it for anything at this stage. We differentiate between 'the compiler understands -fstack-protector' and 'the user wanted -fstack-protector' so that we can pass -fno-stack-protector in appropriate places even if the user didn't want to turn on -fstack-protector for other parts. (This helps us overcome another existing limitation, that glibc doesn't work with GCCs hacked to pass in -fstack-protector by default.) We might want to add another configuration option to turn on -fstack-protector for nscd and other network-facing operations by default, but for now I've stuck with one option to control everything. v2: documentation in install.texi; better description of the option. INSTALL regenerated. v3: Substitute in no_stack_protector. [BZ #7065] * configure.ac (libc_cv_ssp): Move up. (libc_cv_ssp_strong): Likewise. (libc_cv_ssp_all): New. (stack_protector): Augment, adding -fstack-protector-all. (no_stack_protector): New. (AC_ARG_ENABLE(stack-protector)): New configure flag. * manual/install.texi (--enable-stack-protector): Document it. * INSTALL: Regenerate. --- INSTALL | 39 ++++++++++++++++++++----------- configure.ac | 67 +++++++++++++++++++++++++++++++++++++---------------- manual/install.texi | 12 ++++++++++ 3 files changed, 84 insertions(+), 34 deletions(-)