Created attachment 8465 [details] Buffer overflow reproducer Joshua Rogers reported a stack buffer overflow in ihex.c (ihex_bad_byte): http://www.openwall.com/lists/oss-security/2014/11/03/16 It still affects HEAD, as of: 22d31b1 Automatic date update in version.in It was reported to Ubuntu with a reliable reproducer: https://bugs.launchpad.net/bugs/1476014 I've attached the reproducer file. Running size (or gdb and probably others) on the reproducer results in a buffer stack overflow: $ ./binutils/size size-SBBOF *** buffer overflow detected ***: ./binutils/size terminated ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x78c4e)[0x7f457d1c9c4e] /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7f457d269e8c] /lib/x86_64-linux-gnu/libc.so.6(+0x116e80)[0x7f457d267e80] /lib/x86_64-linux-gnu/libc.so.6(+0x1163d9)[0x7f457d2673d9] /lib/x86_64-linux-gnu/libc.so.6(_IO_default_xsputn+0x80)[0x7f457d1cd3a0] /lib/x86_64-linux-gnu/libc.so.6(_IO_vfprintf+0x3e42)[0x7f457d19ea62] /lib/x86_64-linux-gnu/libc.so.6(__vsprintf_chk+0x84)[0x7f457d267464] /lib/x86_64-linux-gnu/libc.so.6(__sprintf_chk+0x7d)[0x7f457d2673bd] ./binutils/size[0x40fb5f] ./binutils/size[0x40ff81] ./binutils/size[0x40ac35] ./binutils/size[0x4035d0] ./binutils/size[0x403780] ./binutils/size[0x402bfe] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f457d171a40] ./binutils/size[0x402d39] ======= Memory map: ======== 00400000-004fc000 r-xp 00000000 08:11 462525 /var/scm/binutils-gdb/binutils/size 006fb000-006fc000 r--p 000fb000 08:11 462525 /var/scm/binutils-gdb/binutils/size 006fc000-00701000 rw-p 000fc000 08:11 462525 /var/scm/binutils-gdb/binutils/size 00701000-00706000 rw-p 00000000 00:00 0 00c91000-00cb2000 rw-p 00000000 00:00 0 [heap] 7f457cc36000-7f457cc4c000 r-xp 00000000 08:11 3408637 /lib/x86_64-linux-gnu/libgcc_s.so.1 7f457cc4c000-7f457ce4b000 ---p 00016000 08:11 3408637 /lib/x86_64-linux-gnu/libgcc_s.so.1 7f457ce4b000-7f457ce4c000 rw-p 00015000 08:11 3408637 /lib/x86_64-linux-gnu/libgcc_s.so.1 7f457ce4c000-7f457d151000 r--p 00000000 08:11 3279935 /usr/lib/locale/locale-archive 7f457d151000-7f457d311000 r-xp 00000000 08:11 3411884 /lib/x86_64-linux-gnu/libc-2.21.so 7f457d311000-7f457d511000 ---p 001c0000 08:11 3411884 /lib/x86_64-linux-gnu/libc-2.21.so 7f457d511000-7f457d515000 r--p 001c0000 08:11 3411884 /lib/x86_64-linux-gnu/libc-2.21.so 7f457d515000-7f457d517000 rw-p 001c4000 08:11 3411884 /lib/x86_64-linux-gnu/libc-2.21.so 7f457d517000-7f457d51b000 rw-p 00000000 00:00 0 7f457d51b000-7f457d51e000 r-xp 00000000 08:11 3409823 /lib/x86_64-linux-gnu/libdl-2.21.so 7f457d51e000-7f457d71d000 ---p 00003000 08:11 3409823 /lib/x86_64-linux-gnu/libdl-2.21.so 7f457d71d000-7f457d71e000 r--p 00002000 08:11 3409823 /lib/x86_64-linux-gnu/libdl-2.21.so 7f457d71e000-7f457d71f000 rw-p 00003000 08:11 3409823 /lib/x86_64-linux-gnu/libdl-2.21.so 7f457d71f000-7f457d743000 r-xp 00000000 08:11 3410094 /lib/x86_64-linux-gnu/ld-2.21.so 7f457d914000-7f457d917000 rw-p 00000000 00:00 0 7f457d937000-7f457d939000 rw-p 00000000 00:00 0 7f457d939000-7f457d940000 r--s 00000000 08:11 3820440 /usr/lib/x86_64-linux-gnu/gconv/gconv-modules.cache 7f457d940000-7f457d942000 rw-p 00000000 00:00 0 7f457d942000-7f457d943000 r--p 00023000 08:11 3410094 /lib/x86_64-linux-gnu/ld-2.21.so 7f457d943000-7f457d944000 rw-p 00024000 08:11 3410094 /lib/x86_64-linux-gnu/ld-2.21.so 7f457d944000-7f457d945000 rw-p 00000000 00:00 0 7fffedd60000-7fffedd81000 rw-p 00000000 00:00 0 [stack] 7fffeddc1000-7fffeddc3000 r--p 00000000 00:00 0 [vvar] 7fffeddc3000-7fffeddc5000 r-xp 00000000 00:00 0 [vdso] ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] Aborted (core dumped)
Created attachment 8466 [details] Fix stack buffer overflow when printing bad bytes in Intel Hex objects Here's a proposed patch to fix this bug.
I've requested that a CVE be assigned for this issue: http://openwall.com/lists/oss-security/2015/07/31/6
Created attachment 8468 [details] Fix incorrect escape sequence on platform with signed char '\377' should not be ever printed as \37777777777 to begin with. With this bug fixed, snprintf and other overcomplications are unnecessary.
Created attachment 8469 [details] Fix incorrect escape sequence on platform with signed char (v2) ... and there are same bug in srec.c.
Yes, but srec.c gets it right by storing the bytes in an unsigned char instead of a signed one. I now think that ihex.c should also have this change, as well: diff --git a/bfd/ihex.c b/bfd/ihex.c index 8e66372..31d1cc7 100644 --- a/bfd/ihex.c +++ b/bfd/ihex.c @@ -276,7 +276,7 @@ ihex_scan (bfd *abfd) else { file_ptr pos; - char hdr[8]; + unsigned char hdr[8]; unsigned int i; unsigned int len; bfd_vma addr;
Created attachment 8470 [details] Fix incorrect escape sequence on platform with signed char (v3) ... and similar bug (but just incorrect output, without stack overflow) in binutils/readelf.c (process_mips_specific). (all other `git grep '%03o'` instances looks safe). Re: tyhicks (#5): wrong, look carefully at srec.c: L299 int c; ... L457 char hdr[3]; ... L473 c = hdr[1]; ... L476 srec_bad_byte (abfd, lineno, c, error); *BOOM*
(In reply to Yuriy Kaminskiy from comment #6) > Created attachment 8470 [details] > Fix incorrect escape sequence on platform with signed char (v3) > > ... and similar bug (but just incorrect output, without stack overflow) in > binutils/readelf.c (process_mips_specific). > (all other `git grep '%03o'` instances looks safe). > > Re: tyhicks (#5): wrong, look carefully at srec.c: > > L299 int c; > ... > L457 char hdr[3]; > ... > L473 c = hdr[1]; > ... > L476 srec_bad_byte (abfd, lineno, c, error); > *BOOM* Hrm? You must be looking at an old version of the code. It was changed in the last hunk of this patch: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=708d7d0d
Created attachment 8471 [details] Fix incorrect escape sequence on platform with signed char (v4) Re: tyhicks (#7): Oops. Indeed, wrong clone :-| Sorry. srec.c, indeed, impossible to trigger in current version. Only exploitable code is in ihex.c. Rebased against correct repo (and similar code re-verified, nothing new found).
Comment on attachment 8471 [details] Fix incorrect escape sequence on platform with signed char (v4) No problem! This patch looks good but I wonder why you still didn't change hdr, ihex_read_section() and ihex_scan(), from an array of char to an array unsigned char?
Created attachment 8472 [details] Fix incorrect escape sequence on platform with signed char (v5) (In reply to Tyler Hicks from comment #9) > This patch looks good but I wonder why you still didn't change hdr, > ihex_read_section() and ihex_scan(), from an array of char to an array > unsigned char? Hmm... it would, indeed, fix this issue as well (and needed anyway, due to "\377 as EOF bug").
Comment on attachment 8472 [details] Fix incorrect escape sequence on platform with signed char (v5) This patch looks great. Thanks!
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7e27a9d5f22f9f7ead11738b1546d0b5c737266b commit 7e27a9d5f22f9f7ead11738b1546d0b5c737266b Author: Yuriy M. Kaminskiy <yumkam@gmail.com> Date: Tue Aug 4 16:51:53 2015 +0100 Fix stack buffer overflows when parsing corrupt ihex files. PR binutils/18750 * ihex.c (ihex_scan): Fixes incorrect escape sequence in error message and stack overflow when char is signed and \200-\376 was in place of hex digit; also fixes \377 was handled as EOF instead of "incorrect character". (ihex_read_section): Changed for consistency. (ihex_bad_byte): Prevent (now impossible to trigger) stack overflow and incorrect escape sequence handling. * srec.c (srec_bad_byte): Likewise. * readelf.c (process_mips_specific): Fix incorrect escape sequence handling.
Hi Tyler, Hi Yuriy, Thanks for the bug report and patches. I have now applied the final version of the patch. Cheers Nick