Bug 18750 - Stack buffer overflow when printing bad bytes in Intel Hex objects
Summary: Stack buffer overflow when printing bad bytes in Intel Hex objects
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-31 17:05 IST by Tyler Hicks
Modified: 2015-08-04 15:54 IST (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Buffer overflow reproducer (15 bytes, application/octet-stream)
2015-07-31 17:05 IST, Tyler Hicks
Details
Fix stack buffer overflow when printing bad bytes in Intel Hex objects (688 bytes, patch)
2015-07-31 17:08 IST, Tyler Hicks
Details | Diff
Fix incorrect escape sequence on platform with signed char (208 bytes, patch)
2015-07-31 19:43 IST, Yuriy Kaminskiy
Details | Diff
Fix incorrect escape sequence on platform with signed char (v2) (578 bytes, patch)
2015-07-31 20:03 IST, Yuriy Kaminskiy
Details | Diff
Fix incorrect escape sequence on platform with signed char (v3) (756 bytes, patch)
2015-07-31 20:47 IST, Yuriy Kaminskiy
Details | Diff
Fix incorrect escape sequence on platform with signed char (v4) (802 bytes, patch)
2015-07-31 22:04 IST, Yuriy Kaminskiy
Details | Diff
Fix incorrect escape sequence on platform with signed char (v5) (1.08 KB, patch)
2015-08-01 00:27 IST, Yuriy Kaminskiy
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Hicks 2015-07-31 17:05:22 IST
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)
Comment 1 Tyler Hicks 2015-07-31 17:08:59 IST
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.
Comment 2 Tyler Hicks 2015-07-31 17:27:43 IST
I've requested that a CVE be assigned for this issue:

  http://openwall.com/lists/oss-security/2015/07/31/6
Comment 3 Yuriy Kaminskiy 2015-07-31 19:43:54 IST
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.
Comment 4 Yuriy Kaminskiy 2015-07-31 20:03:02 IST
Created attachment 8469 [details]
Fix incorrect escape sequence on platform with signed char (v2)

... and there are same bug in srec.c.
Comment 5 Tyler Hicks 2015-07-31 20:37:20 IST
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;
Comment 6 Yuriy Kaminskiy 2015-07-31 20:47:46 IST
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*
Comment 7 Tyler Hicks 2015-07-31 21:06:15 IST
(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
Comment 8 Yuriy Kaminskiy 2015-07-31 22:04:15 IST
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 9 Tyler Hicks 2015-07-31 22:16:23 IST
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?
Comment 10 Yuriy Kaminskiy 2015-08-01 00:27:04 IST
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 11 Tyler Hicks 2015-08-01 16:45:17 IST
Comment on attachment 8472 [details]
Fix incorrect escape sequence on platform with signed char (v5)

This patch looks great. Thanks!
Comment 12 cvs-commit@gcc.gnu.org 2015-08-04 15:53:31 IST
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.
Comment 13 Nick Clifton 2015-08-04 15:54:54 IST
Hi Tyler, Hi Yuriy,

  Thanks for the bug report and patches.  I have now applied the final version of the patch.

Cheers
  Nick