--- Begin Message ---
- To: Debian Bugs <submit@bugs.debian.org>
- Subject: improvements from Ubuntu to handle compiler hardening better
- From: Kees Cook <kees@debian.org>
- Date: Mon, 4 Jan 2010 01:20:33 -0800
- Message-id: <20100104092033.GI16532@outflux.net>
Package: eglibc
Version: 2.10.2-3
Severity: normal
Tags: patch
User: ubuntu-devel@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.cook@canonical.com>
Index: glibc-2.9/debug/vsprintf_chk.c
===================================================================
--- glibc-2.9.orig/debug/vsprintf_chk.c 2008-12-23 21:30:07.000000000 -0800
+++ glibc-2.9/debug/vsprintf_chk.c 2008-12-23 21:30:19.000000000 -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 <doko@ubuntu.com>
--- ./libio/stdio.h~ 2008-05-24 20:14:36.000000000 +0200
+++ ./libio/stdio.h 2009-03-27 20:59:20.000000000 +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 MIN(a,b) (((a)<(b))?(a):(b))
@@ -80,6 +83,32 @@
unsigned char *p = (unsigned char *) &ret;
p[sizeof (ret) - 1] = 255;
p[sizeof (ret) - 2] = '\n';
+#ifdef HP_TIMING_NOW
+ hp_timing_t hpt;
+ HP_TIMING_NOW (hpt);
+ hpt = (hpt & 0xffff) << 8;
+ ret ^= hpt;
+#endif
+ uintptr_t stk;
+ /* Avoid GCC being too smart. */
+ asm ("" : "=r" (stk) : "r" (p));
+ stk &= 0x7ffff0;
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+ stk <<= (__WORDSIZE - 23);
+#elif __WORDSIZE == 64
+ stk <<= 31;
+#endif
+ ret ^= stk;
+ /* Avoid GCC being too smart. */
+ p = (unsigned char *) &errno;
+ asm ("" : "=r" (stk) : "r" (p));
+ stk &= 0x7fff00;
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+ stk <<= (__WORDSIZE - 29);
+#else
+ stk >>= 8;
+#endif
+ ret ^= stk;
}
else
#endif
Index: b/elf/tst-stackguard1.c
===================================================================
--- a/elf/tst-stackguard1.c
+++ b/elf/tst-stackguard1.c
@@ -160,17 +160,21 @@
the 16 runs, something is very wrong. */
int ndifferences = 0;
int ndefaults = 0;
+ int npartlyrandomized = 0;
for (i = 0; i < N; ++i)
{
if (child_stack_chk_guards[i] != child_stack_chk_guards[i+1])
ndifferences++;
else if (child_stack_chk_guards[i] == default_guard)
ndefaults++;
+ else if (*(char *) &child_stack_chk_guards[i] == 0)
+ npartlyrandomized++;
}
- printf ("differences %d defaults %d\n", ndifferences, ndefaults);
+ printf ("differences %d defaults %d partly randomized %d\n",
+ ndifferences, ndefaults, npartlyrandomized);
- if (ndifferences < N / 2 && ndefaults < N / 2)
+ if ((ndifferences + ndefaults + npartlyrandomized) < 3 * N / 4)
{
puts ("stack guard canaries are not randomized enough");
puts ("nor equal to the default canary value");
Index: b/nptl/tst-stackguard1.c
===================================================================
--- a/nptl/tst-stackguard1.c
+++ b/nptl/tst-stackguard1.c
@@ -190,17 +190,21 @@
the 16 runs, something is very wrong. */
int ndifferences = 0;
int ndefaults = 0;
+ int npartlyrandomized = 0;
for (i = 0; i < N; ++i)
{
if (child_stack_chk_guards[i] != child_stack_chk_guards[i+1])
ndifferences++;
else if (child_stack_chk_guards[i] == default_guard)
ndefaults++;
+ else if (*(char *) &child_stack_chk_guards[i] == 0)
+ npartlyrandomized++;
}
- printf ("differences %d defaults %d\n", ndifferences, ndefaults);
+ printf ("differences %d defaults %d partly randomized %d\n",
+ ndifferences, ndefaults, npartlyrandomized);
- if (ndifferences < N / 2 && ndefaults < N / 2)
+ if ((ndifferences + ndefaults + npartlyrandomized) < 3 * N / 4)
{
puts ("stack guard canaries are not randomized enough");
puts ("nor equal to the default canary value");
Description: require that the first byte in the stack guard in a NULL byte,
to improve mitigation of NULL-terminated string overflows.
Bug: http://sourceware.org/bugzilla/show_bug.cgi?id=10149
Bug-Ubuntu: https://bugs.launchpad.net/bugs/413278
Author: Kees Cook <kees.cook@canonical.com>
Index: eglibc-2.10.1/sysdeps/unix/sysv/linux/dl-osinfo.h
===================================================================
--- eglibc-2.10.1.orig/sysdeps/unix/sysv/linux/dl-osinfo.h 2009-08-12 16:30:26.000000000 -0700
+++ eglibc-2.10.1/sysdeps/unix/sysv/linux/dl-osinfo.h 2009-08-12 16:32:55.000000000 -0700
@@ -65,7 +65,12 @@
static inline uintptr_t __attribute__ ((always_inline))
_dl_setup_stack_chk_guard (void *dl_random)
{
- uintptr_t ret;
+ uintptr_t ret = 0;
+ /* Having a leading zero byte protects the stack guard from being
+ overwritten with str* write operations or exposed by an
+ unterminated str* read operation. */
+ unsigned char *p = ((unsigned char *) &ret) + 1;
+ int size = sizeof (ret) - 1;
#ifndef __ASSUME_AT_RANDOM
if (__builtin_expect (dl_random == NULL, 0))
{
@@ -73,16 +78,16 @@
int fd = __open ("/dev/urandom", O_RDONLY);
if (fd >= 0)
{
- ssize_t reslen = __read (fd, &ret, sizeof (ret));
+ ssize_t reslen = __read (fd, p, size);
__close (fd);
- if (reslen == (ssize_t) sizeof (ret))
+ if (reslen == (ssize_t) size)
return ret;
}
# endif
- ret = 0;
- unsigned char *p = (unsigned char *) &ret;
- p[sizeof (ret) - 1] = 255;
- p[sizeof (ret) - 2] = '\n';
+ /* Lacking any other form of randomized stack guard, add other
+ terminators in an attempt to block things like fgets, etc. */
+ p[size - 1] = 255;
+ p[size - 2] = '\n';
#ifdef HP_TIMING_NOW
hp_timing_t hpt;
HP_TIMING_NOW (hpt);
@@ -115,7 +120,7 @@
/* We need in the moment only 8 bytes on 32-bit platforms and 16
bytes on 64-bit platforms. Therefore we can use the data
directly and not use the kernel-provided data to seed a PRNG. */
- memcpy (&ret, dl_random, sizeof (ret));
+ memcpy (p, dl_random, size);
return ret;
}
--- End Message ---
--- Begin Message ---
- To: Kees Cook <kees@debian.org>
- Cc: 563637-done@bugs.debian.org
- Subject: Re: improvements from Ubuntu to handle compiler hardening better
- From: Aurelien Jarno <aurelien@aurel32.net>
- Date: Wed, 21 Oct 2015 00:19:39 +0200
- Message-id: <20151020221939.GA10319@aurel32.net>
- In-reply-to: <20140506205604.GA8736@volta.rr44.fr>
- References: <20100104092033.GI16532@outflux.net> <20100207131308.GA20616@volta.aurel32.net> <20100211193710.GK18073@outflux.net> <20140506205604.GA8736@volta.rr44.fr>
On 2014-05-06 22:56, Aurelien Jarno wrote:
> On Thu, Feb 11, 2010 at 11:37:10AM -0800, Kees Cook wrote:
> > 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.
>
> We are now more than 5 years since the mail with the list of affected
> packages, and -D_FORTIFY_SOURCE=2 is now the default, so I guess all
> affected packages have been fixed.
>
> > > > 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. :(
>
> This has been done by upstream in glibc 2.16.
>
> So I guess we could now close this bug. Do you agree?
I am now closing the bug. Please feel free to reopen it if something is
still missing.
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
--- End Message ---