Bug#563637: improvements from Ubuntu to handle compiler hardening better
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? -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net -- To UNSUBSCRIBE, email to debian-glibc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20140506205604.ga8...@volta.rr44.fr
Bug#563637: improvements from Ubuntu to handle compiler hardening better
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
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
