[01/18] Configury support for --enable-stack-protector.
Commit Message
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(-)
Comments
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]
@@ -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
@@ -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
@@ -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