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 

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 

--- ./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 
 #include 
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #ifndef MIN
 # define MIN(a,b) (((a)&l