Bug#563637: improvements from Ubuntu to handle compiler hardening better

2010-02-11 Thread Kees Cook
Hi Aurelien,

On Sun, Feb 07, 2010 at 02:13:08PM +0100, Aurelien Jarno wrote:
  I would like to include the following patches that Ubuntu has carried for
  several releases now.  (Note that submitted-leading-zero-stack-guard.diff
  will need to be adjusted slightly if stack-guard-quick-randomization.diff
  is not applied.)
 
 I have applied the two stack protection patches in the Debian package,
 but not the two other ones. See my comments below.

Excellent, thanks!

  no-sprintf-pre-truncate.diff
  The sprintf function used when -D_FORTIFY_SOURCE=2 is used incorrectly
  pre-truncates the destination buffer; this changes the long-standing
  expectation of sprintf(foo,%sbaz,foo) to work.  See the patch for
  further discussion.
 
 As explained in the bug report, this code is not valid anyway. If we
 want people to fix their code, we should not workaround the issue. Also
 I am not able to evaluate the impact on the fix, and don't know if it
 may introduce a security bug.

Right, it's incorrect, but around 200 packages[1] use it and expect the
prior behavior.  I don't feel there is a security issue here, but I can
respect not wanting to change it.  200 is a pretty small number of
packages compared to the overall size of the archive.

Perhaps I can re-scan the archive and actually do the mass bug filing.

  local-fwrite-no-attr-unused.diff
  Again, patch contains discussion, but basically, this disables a
  useless and noisy warning that -D_FORTIFY_SOURCE=2 triggers.
 
 I think people should either not use -D_FORTIFY_SOURCE=2 or fix their
 code. This is a warning anyway. I agree an error can happens up to the
 fclose() call, but it's not an excuse to not check possible errors at
 the fwrite() level. The real bug is actually that fclose() is not marked
 __wur, and that's probably what has to be fixed.

Yeah, I would tend to agree.  The main glitch was that there is no
compiler option to turn off the warning.  :(

Thanks for reviewing the patches!

-Kees

[1] http://lists.debian.org/debian-devel/2008/12/msg01079.html

-- 
Kees Cook@debian.org



-- 
To UNSUBSCRIBE, email to debian-glibc-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org



Bug#563637: improvements from Ubuntu to handle compiler hardening better

2010-01-04 Thread Kees Cook
Package: eglibc
Version: 2.10.2-3
Severity: normal
Tags: patch
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu lucid ubuntu-patch

Hello!

As more packages (perhaps all!) start using either hardening-wrapper
or the hardening-includes packages to gain the -D_FORTIFY_SOURCE=2 and
-fstack-protector compiler flags, it starts becoming important to handle
a number of special cases that upstream glibc either hasn't acted on or
has inappropriately rejected.

I would like to include the following patches that Ubuntu has carried for
several releases now.  (Note that submitted-leading-zero-stack-guard.diff
will need to be adjusted slightly if stack-guard-quick-randomization.diff
is not applied.)

no-sprintf-pre-truncate.diff
The sprintf function used when -D_FORTIFY_SOURCE=2 is used incorrectly
pre-truncates the destination buffer; this changes the long-standing
expectation of sprintf(foo,%sbaz,foo) to work.  See the patch for
further discussion.

local-fwrite-no-attr-unused.diff
Again, patch contains discussion, but basically, this disables a
useless and noisy warning that -D_FORTIFY_SOURCE=2 triggers.

stack-guard-quick-randomization.diff
For applications built with -fstack-protector, this adds the
poor man's randomization for when AT_RANDOM is not available.
Since AT_RANDOM is in 2.6.31 and later, it may not be useful to
carry any longer and may not be kfreebsd friendly.

submitted-leading-zero-stack-guard.diff
This is important as the recent glibc fails to keep the first byte
of the stack guard a NULL when constructing the stack guard for use
with -fstack-protector.  Without this, it is possible to potentially
read and write beyond the stack guard value using NULL-terminated
string overflows.

Thanks!

-Kees

-- 
Kees Cook@debian.org
Description: when a program is compiled with -D_FORTIFY_SOURCE=2, the
 vsprintf_chk function is called to handle sprintf/snprintf, but it
 needlessly pretruncates the destination which changes the results of
 sprintf(foo, %sbar, baz).
Bug: http://sourceware.org/bugzilla/show_bug.cgi?id=7075
Bug-Ubuntu: https://launchpad.net/bugs/305901
Author: Kees Cook kees.c...@canonical.com

Index: glibc-2.9/debug/vsprintf_chk.c
===
--- glibc-2.9.orig/debug/vsprintf_chk.c	2008-12-23 21:30:07.0 -0800
+++ glibc-2.9/debug/vsprintf_chk.c	2008-12-23 21:30:19.0 -0800
@@ -76,7 +76,6 @@
 
   _IO_no_init (f._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);
   _IO_JUMPS (f._sbf) = _IO_str_chk_jumps;
-  s[0] = '\0';
   _IO_str_init_static_internal (f, s, slen - 1, s);
 
   /* For flags  0 (i.e. __USE_FORTIFY_LEVEL  1) request that %n
Description: when compiling with -D_FORTIFY_SOURCE=2, the compiler will
 generate warn-unused-results notifications for several functions.  It
 is not sensible to do this for fwrite() since it is frequently unchecked
 and may not fail until fclose() which is not marked with __wur, making
 the fwrite() check noisy and pointless.
Author: Matthias Klose d...@ubuntu.com

--- ./libio/stdio.h~	2008-05-24 20:14:36.0 +0200
+++ ./libio/stdio.h	2009-03-27 20:59:20.0 +0100
@@ -682,7 +682,7 @@
This function is a possible cancellation points and therefore not
marked with __THROW.  */
 extern size_t fwrite (__const void *__restrict __ptr, size_t __size,
-		  size_t __n, FILE *__restrict __s) __wur;
+		  size_t __n, FILE *__restrict __s);
 __END_NAMESPACE_STD
 
 #ifdef __USE_GNU
@@ -706,7 +706,7 @@
 extern size_t fread_unlocked (void *__restrict __ptr, size_t __size,
 			  size_t __n, FILE *__restrict __stream) __wur;
 extern size_t fwrite_unlocked (__const void *__restrict __ptr, size_t __size,
-			   size_t __n, FILE *__restrict __stream) __wur;
+			   size_t __n, FILE *__restrict __stream);
 #endif
 
 
Description: when AT_RANDOM is not available, attempt to build randomization
 of stack guard value from the ASLR of stack and heap locations, and finally
 the hp_timing_t value.  Upstream glibc does not want this patch, as they
 feel AT_RANDOM is sufficient.
Author: Jakub Jelinek
Origin: http://cvs.fedora.redhat.com/viewvc/devel/glibc/
Forwarded: not-needed

---
 elf/tst-stackguard1.c   |8 ++--
 nptl/tst-stackguard1.c  |8 ++--
 sysdeps/unix/sysv/linux/dl-osinfo.h |   29 +
 3 files changed, 41 insertions(+), 4 deletions(-)

Index: b/sysdeps/unix/sysv/linux/dl-osinfo.h
===
--- a/sysdeps/unix/sysv/linux/dl-osinfo.h
+++ b/sysdeps/unix/sysv/linux/dl-osinfo.h
@@ -17,10 +17,13 @@
Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
02111-1307 USA.  */
 
+#include errno.h
 #include kernel-features.h
 #include dl-sysdep.h
 #include fcntl.h
 #include stdint.h
+#include hp-timing.h
+#include endian.h
 
 #ifndef MIN
 # define