Patchwork [01/18] Configury support for --enable-stack-protector.

login
register
mail settings
Submitter Nix
Date March 8, 2016, 1:50 p.m.
Message ID <1457445064-7107-2-git-send-email-nix@esperi.org.uk>
Download mbox | patch
Permalink /patch/11252/
State New
Headers show

Comments

Nix - March 8, 2016, 1:50 p.m.
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(-)
Mike Frysinger - March 9, 2016, 10:36 p.m.
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
Nix - March 9, 2016, 11:29 p.m.
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.
Nix - March 10, 2016, 1:02 a.m.
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]

Patch

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