Skip to content
This repository has been archived by the owner on Sep 17, 2018. It is now read-only.

Heap Buffer Overflow on bchunk v1.2.1 and v1.2.0 #2

Closed
kongwenbin opened this issue Aug 31, 2017 · 30 comments
Closed

Heap Buffer Overflow on bchunk v1.2.1 and v1.2.0 #2

kongwenbin opened this issue Aug 31, 2017 · 30 comments

Comments

@kongwenbin
Copy link

I discovered an instance of heap buffer overflow bug on bchunk v1.2.1 and v1.2.0. This issue was discovered and can be replicated on a 32-bit Ubuntu machine, for instance I discovered the issue on Linux ubuntu 4.10.0-32-generic #36~16.04.1-Ubuntu SMP Wed Aug 9 09:18:53 UTC 2017 i686 i686 i686 GNU/Linux

The following is some stack trace information, please kindly advise how and where can I share the full output with more details and also the POC files to replicate the issue:

# bchunk any.bin $FILE /dev/null
ERROR: AddressSanitizer: heap-buffer-overflow on address 0xb5f00b91 at pc 0x080a717e bp 0xbf8b8f18 sp 0xbf8b8af0
WRITE of size 24 at 0xb5f00b91 thread T0
    #0 0x80a717d  (/opt/targetApps/asan/bchunk-1.2.1/bchunk+0x80a717d)
    #1 0x80a724b  (/opt/targetApps/asan/bchunk-1.2.1/bchunk+0x80a724b)
    #2 0x81337e9  (/opt/targetApps/asan/bchunk-1.2.1/bchunk+0x81337e9)
    #3 0x8135b2a  (/opt/targetApps/asan/bchunk-1.2.1/bchunk+0x8135b2a)
    #4 0xb748c636  (/lib/i386-linux-gnu/libc.so.6+0x18636)
    #5 0x805ecd7  (/opt/targetApps/asan/bchunk-1.2.1/bchunk+0x805ecd7)

I have emailed the author a few days ago but I don't think he still maintain the code, since v1.2.0 was published in 2004. However, this project's v1.2.1 was published in 2016 so I believe that this is still maintained. This seems to be the only active upstream for bchunk. If this is not the right place to report the issue, please kindly point me to the right direction, thanks a lot!

@kongwenbin
Copy link
Author

kongwenbin commented Sep 9, 2017

Below is the output from gdb exploitable having the following crash hash value for v1.2.0 and v1.2.1 respectively and determining that the issue is exploitable as well.

  • v1.2.0: ff124d9a4469aa25d74fb3dce4d74b59.e950ae68ac3f7d8cb9227eb083a20054
(gdb) exploitable
__main__:99: UserWarning: GDB v7.11 may not support required Python API
Description: Heap error
Short description: HeapError (10/22)
Hash: ff124d9a4469aa25d74fb3dce4d74b59.e950ae68ac3f7d8cb9227eb083a20054
Exploitability Classification: EXPLOITABLE
Explanation: The target's backtrace indicates that libc has detected a heap error or that the target was executing a heap function when it stopped. This could be due to heap corruption, passing a bad pointer to a heap function such as free(), etc. Since heap errors might include buffer overflows, use-after-free situations, etc. they are generally considered exploitable.
Other tags: AbortSignal (20/22)
  • v1.2.1: b26ce06e43455b1895a6768234d44cae.c787edf5a5b647c4cea8a01f7978a316
(gdb) exploitable
__main__:99: UserWarning: GDB v7.11 may not support required Python API
Description: Heap error
Short description: HeapError (10/22)
Hash: b26ce06e43455b1895a6768234d44cae.c787edf5a5b647c4cea8a01f7978a316
Exploitability Classification: EXPLOITABLE
Explanation: The target's backtrace indicates that libc has detected a heap error or that the target was executing a heap function when it stopped. This could be due to heap corruption, passing a bad pointer to a heap function such as free(), etc. Since heap errors might include buffer overflows, use-after-free situations, etc. they are generally considered exploitable.
Other tags: AbortSignal (20/22)

@lukateras
Copy link

lukateras commented Oct 30, 2017

This patch should fix both this issue and #3, but I should test this with your payload.

--- bchunk-1.2.0.orig/bchunk.c	2017-10-30 18:03:58.658741629 +0000
+++ bchunk-1.2.0/bchunk.c	2017-10-30 19:40:25.558131619 +0000
@@ -18,6 +18,7 @@
   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
   */
 
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -271,11 +272,10 @@
 	int16_t i;
 	float fl;
 	
-	if (!(fname = malloc(strlen(bname) + 8))) {
-		fprintf(stderr, "main(): malloc() failed, out of memory\n");
+	if (asprintf(&fname, "%s%2.2d.%s", bname, track->num, track->extension) == -1) {
+		fprintf(stderr, "writetrack(): asprintf() failed, out of memory\n");
 		exit(4);
 	}
-	sprintf(fname, "%s%2.2d.%s", bname, track->num, track->extension);
 	
 	printf("%2d: %s ", track->num, fname);
 	

Can be reproduced if TRACK number is long enough, for example:

FILE "Shpongle - Are You Shpongled.wav" WAVE
  TRACK 011123456789 AUDIO
    TITLE "Divine Moments of Truth"
    INDEX 01 00:00:00

There are only 2 heap allocations, and the other one seems to be fine. So this and #4 (comment) should do.

@kongwenbin
Copy link
Author

Very nice fix there @yegortimoshenko ! I find it really amazing how you fixed the issue so quickly before I can react and provide you with more information. 👍

The issue should be fixed, but just to be sure, please try the attached payload.
poc.zip

@lukateras
Copy link

This payload results in error code 3, as expected:

$ valgrind result/bin/bchunk poc18.bin poc18.cue /tmp/zzz
==14075== Memcheck, a memory error detector
==14075== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==14075== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==14075== Command: result/bin/bchunk poc18.bin poc18.cue /tmp/zzz
==14075== 
binchunker for Unix, version 1.2.0 by Heikki Hannikainen <hessu@hes.iki.fi>
	Created with the kind help of Bob Marietta <marietrg@SLU.EDU>,
	partly based on his Pascal (Delphi) implementation.
	Support for MODE2/2352 ISO tracks thanks to input from
	Godmar Back <gback@cs.utah.edu>, Colas Nahaboo <Colas@Nahaboo.com>
	and Matthew Green <mrg@eterna.com.au>.
	Released under the GNU GPL, version 2 or later (at your option).

Reading the CUE file:

Track -1:  14444444444 (?) 
Track  1: �YY          (?) 
... ouch, no space after TRACK.
Track ==14075== 
==14075== HEAP SUMMARY:
==14075==     in use at exit: 1,889 bytes in 9 blocks
==14075==   total heap usage: 11 allocs, 2 frees, 7,009 bytes allocated
==14075== 
==14075== LEAK SUMMARY:
==14075==    definitely lost: 0 bytes in 0 blocks
==14075==    indirectly lost: 0 bytes in 0 blocks
==14075==      possibly lost: 0 bytes in 0 blocks
==14075==    still reachable: 1,889 bytes in 9 blocks
==14075==         suppressed: 0 bytes in 0 blocks
==14075== Rerun with --leak-check=full to see details of leaked memory
==14075== 
==14075== For counts of detected and suppressed errors, rerun with: -v
==14075== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

$ echo $?
3

So yes, this should be fixed. Thanks for reporting and figuring this out @kongwenbin!

Once I test #4 I'll post patches to Red Hat mailing list (https://bugzilla.redhat.com/show_bug.cgi?id=1507576), hopefully it will propagate to other distros from here. NixOS already has these patches in master, Gentoo should merge them soon.

@kongwenbin
Copy link
Author

kongwenbin commented Oct 31, 2017

No worries! The plan sounds good, many thanks to you too @yegortimoshenko !

By the way, since the issues are fixed already, do I proceed to close the issues now? Or do I need to wait until all the distros has finish merging? Thanks.

@kongwenbin
Copy link
Author

Closing all the 3 issues since they are fixed!

@ilovezfs
Copy link

@hessu would it be possible to get these fixes upstreamed in a new release of bchunk?

@lukateras
Copy link

lukateras commented Nov 12, 2017

@hessu: if you are not interested i maintaining bchunk anymore, I could fork it, apply these patches and clean it up further, without changing any of the behavior, of course. Would need some sort of approval from you in order to get my fork packaged by distributions.

@ilovezfs
Copy link

@yegortimoshenko right. The question is whether to revert Homebrew/homebrew-core#20471 which is contingent on whether this will ever actually be fixed upstream.

@lukateras
Copy link

lukateras commented Nov 13, 2017

@ilovezfs Now I remember why I've stopped using Homebrew even when I was still a Mac user, it's because you break users' computers all the time.

Just so that you know, some virtual machines and game console emulators only read ISO, and don't support BIN+CUE, including VMWare Fusion and VirtualBox. Some people have album rips in BIN+CUE and need to convert it to actually play it. bchunk is literally the only program that can do that.

These vulnerabilities are not exploitable on Darwin, as Darwin heap is not executable, and the only way to make it executable is via an explicit mprotect syscall.

If patched version doesn't build on Darwin, I'd be happy to help, if you show me logs.

@ilovezfs
Copy link

@yegortimoshenko deleting the bchunk formula doesn't break anyone's existing installations and it's trivial to add the formula to your own tap. Please don't spread misinformation.

@lukateras
Copy link

lukateras commented Nov 13, 2017

@ilovezfs Whenever someone removes something that is publicly facing, it's breakage. In this case you might delay it until someone buys a new Mac, reinstalls macOS, or just wipes all Homebrew packages (something I've personally done multiple times). People rely on it being there, and will be taken by suprise.

It's an essential piece of software that is required to handle a very popular format for use in some (also popular) programs. And users are not supposed to know how to write package declarations in order to install software.

On to the patches: I've looked up Homebrew/homebrew-core@61ca0be and these patches are meant to apply on top of upstream's tarball, not Debian's. It is located at http://he.fi/bchunk/bchunk-1.2.0.tar.gz.

@ilovezfs
Copy link

Whenever someone removes something that is publicly facing, it's breakage.

which is not the same thing as "you break users' computers all the time."

@ilovezfs
Copy link

@yegortimoshenko hmm I'm getting

bash-3.2$ patch -p1 < CVE-2017-15955.patch 
patching file bchunk.c
Hunk #1 succeeded at 426 with fuzz 1.
missing header for unified diff at line 19 of patch
can't find file to patch at input line 19
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|
--------------------------
File to patch: ^C

@lukateras
Copy link

lukateras commented Nov 13, 2017

Patches for me:

[chronos@yegatool:/tmp]$ curl -LO http://he.fi/bchunk/bchunk-1.2.0.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 15078  100 15078    0     0  15078      0  0:00:01 --:--:--  0:00:01  144k

[chronos@yegatool:/tmp]$ tar zxf bchunk-1.2.0.tar.gz 

[chronos@yegatool:/tmp]$ cd bchunk-1.2.0

[chronos@yegatool:/tmp/bchunk-1.2.0]$ curl -LO https://raw.githubusercontent.com/NixOS/nixpkgs/7d04f9f8fdf22071f422ba8563d47b9ca04c518c/pkgs/tools/cd-dvd/bchunk/CVE-2017-15953.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   738  100   738    0     0    738      0  0:00:01 --:--:--  0:00:01  2055

[chronos@yegatool:/tmp/bchunk-1.2.0]$ curl -LO https://raw.githubusercontent.com/NixOS/nixpkgs/7d04f9f8fdf22071f422ba8563d47b9ca04c518c/pkgs/tools/cd-dvd/bchunk/CVE-2017-15955.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   833  100   833    0     0    833      0  0:00:01  0:00:01 --:--:--   745

[chronos@yegatool:/tmp/bchunk-1.2.0]$ patch -p1 < CVE-2017-15953.patch 
patching file bchunk.c

[chronos@yegatool:/tmp/bchunk-1.2.0]$ patch -p1 < CVE-2017-15955.patch 
patching file bchunk.c
Hunk #1 succeeded at 426 with fuzz 1.
patch unexpectedly ends in middle of line

[chronos@yegatool:/tmp/bchunk-1.2.0]$ echo $?
0

I'll make a cleaner version of the second patch, maybe that will fix it? Hold on.

@lukateras
Copy link

@ilovezfs Here's a new version of CVE-2017-15955 patch that should apply without any warnings:
CVE-2017-15955.patch.gz

@ilovezfs
Copy link

ilovezfs commented Nov 13, 2017

@yegortimoshenko yep that's the output I get from GNU patch 2.7.5. However, not from 2.5.8. Also note that the patch doesn't actually fully apply.

bash-3.2$ /usr/local/Cellar/gpatch/2.7.5/bin/patch -p1 < CVE-2017-15955.patch 
patching file bchunk.c
Hunk #1 succeeded at 426 with fuzz 1.
patch unexpectedly ends in middle of line
bash-3.2$ git diff
diff --git a/bchunk.c b/bchunk.c
index 619aa43..04d57e8 100644
--- a/bchunk.c
+++ b/bchunk.c
@@ -18,6 +18,7 @@
   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
   */
 
+#define _GNU_SOURCE
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -271,11 +272,10 @@ int writetrack(FILE *bf, struct track_t *track, char *bname)
        int16_t i;
        float fl;
        
-       if (!(fname = malloc(strlen(bname) + 8))) {
-               fprintf(stderr, "main(): malloc() failed, out of memory\n");
+       if (asprintf(&fname, "%s%2.2d.%s", bname, track->num, track->extension) == -1) {
+               fprintf(stderr, "writetrack(): asprintf() failed, out of memory\n");
                exit(4);
        }
-       sprintf(fname, "%s%2.2d.%s", bname, track->num, track->extension);
        
        printf("%2d: %s ", track->num, fname);
        
@@ -426,12 +426,12 @@ int main(int argc, char **argv)
                        printf("\nTrack ");
                        if (!(p = strchr(p, ' '))) {
                                fprintf(stderr, "... ouch, no space after TRACK.\n");
-                               continue;
+                               exit(3);
                        }
                        p++;
                        if (!(t = strchr(p, ' '))) {
                                fprintf(stderr, "... ouch, no space after track number.\n");
-                               continue;
+                               exit(3);
                        }
                        *t = '\0';
                        
bash-3.2$ 

So I think the nix package may still be vulnerable.

@ilovezfs
Copy link

i.e. the diff ends up missing the part at the bottom

@@ -460,12 +460,12 @@
 		} else if ((p = strstr(s, "INDEX"))) {
 			if (!(p = strchr(p, ' '))) {
 				printf("... ouch, no space after INDEX.\n");
-				continue;
+				exit(3);
 			}
 			p++;
 			if (!(t = strchr(p, ' '))) {
 				printf("... ouch, no space after index number.\n");
-				continue;
+				exit(3);
 			}
 			*t = '\0';
 			t++;

@lukateras
Copy link

@ilovezfs Thank you a lot, good catch! I'll submit the fixed patch in Gentoo and Nix.

@ilovezfs
Copy link

@ilovezfs
Copy link

ilovezfs commented Nov 13, 2017

@yegortimoshenko no problem. BTW, there's probably some switch to pass to 2.7.5 to make that a hard error instead of exiting 0.

Another question: any reason you're not applying the Debian 01-track-size.patch patch?

bash-3.2$ cat debian-d/patches/01-track-size.patch 
Author: Piotr Kaczuba <pepe@attika.ath.cx>
Description:
 Fix wrong track size calculation when having multiple tracks in one image
 (Closes: #261274).

diff -Naurp bchunk.orig/bchunk.c bchunk/bchunk.c
--- bchunk.orig/bchunk.c	2009-07-16 22:13:58.000000000 +0000
+++ bchunk/bchunk.c	2009-07-16 22:16:33.000000000 +0000
@@ -476,7 +476,7 @@ int main(int argc, char **argv)
 			if (verbose)
 				printf(" (startsect %ld ofs %ld)", track->startsect, track->start);
 			if ((prevtrack) && (prevtrack->stopsect < 0)) {
-				prevtrack->stopsect = track->startsect;
+				prevtrack->stopsect = track->startsect - 1;
 				prevtrack->stop = track->start - 1;
 			}
 		}
@@ -484,7 +484,7 @@ int main(int argc, char **argv)
 	
 	if (track) {
 		fseek(binf, 0, SEEK_END);
-		track->stop = ftell(binf);
+		track->stop = ftell(binf) - 1;
 		track->stopsect = track->stop / SECTLEN;
 	}
 	

@lukateras
Copy link

@ilovezfs We should patch that too, thank you! This fixes an off-by-one error (i.e. in case with two-track bin/cue, one of the tracks gets an extra sector while the other gets truncated).

@ilovezfs
Copy link

Debian has one more patch, but it appears to just be cosmetics https://gist.github.com/ilovezfs/d15f03df971edee317a24132d212aead

@hessu
Copy link

hessu commented Nov 13, 2017

Hi guys! Thanks for the patches. I'll merge the security fixes and release upstream version 1.2.1 in the near future; cannot commit to a date though right now.

@hessu
Copy link

hessu commented Nov 13, 2017

Oh well, just went and did it now. Thank you for the patches, again. 1.2.2 is now available in upstream (https://github.com/hessu/bchunk and the crummy old http://he.fi/bchunk/). Also merged the two debian patches which had been lurking around.

@lukateras
Copy link

@hessu Thank you for a prompt release!

@ilovezfs
Copy link

Thanks @hessu! You're awesome :)

@ilovezfs
Copy link

@pravi you'll probably want to upgrade Debian to 1.2.2. :)

@hessu
Copy link

hessu commented Nov 14, 2017

I had not really realised that bchunk is actually still being actively used by someone. Should probably throw it at coverity and see what it finds, and do a bit of a code review; maybe I've learned something about writing software in the past 19 years and could improve it a bit by now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants