Fixed SIGSEGV, fixed heap buffer overflow, heap use after free #6925

wargio · 2017-03-06T01:22:04Z

Here is the list of the binaries tested:

heap-buffer-overflow-2ce-030-5a6
heap-buffer-overflow-657-030-5a6
heap-buffer-overflow-657-26e-dc4
heap-buffer-overflow-ad2-d00-020
heap-buffer-overflow-fc3-0b5-0a6
heap-buffer-overflow-fc3-183-0a6
heap-buffer-overflow-fc3-220-6d2
heap-use-after-free-c05-e7c-e7c
SEGV-341-892-d00
SEGV-8f6-98a-f0e
SEGV-989-98a-f0e
SEGV-a94-0b5-0a6
SEGV-a94-220-6d2
SEGV-b7b-c17-f10
SEGV-b7b-c17-f10
SEGV-d26-f76-98a
SEGV-d8f-f76-98a

alvarofe · 2017-03-06T08:10:30Z

libr/util/r_asn1.c

@@ -386,16 +392,14 @@ ut32 r_asn1_count_objects (const ut8 *buffer, ut32 length) {
 	while (next >= buffer && next < end) {
 		object = asn1_parse_header (next, end - next);
 		if (!object || next == object->sector) {
-			//			if (object->tag != TAG_NULL)
+			if (object) {


there is not need to check if (object) free, free already takes care of that

alvarofe · 2017-03-06T08:10:46Z

libr/util/r_asn1.c

 				inner = r_asn1_create_object (next, end - next);
 				if (!inner || next == inner->sector) {
-					//if(inner->tag != TAG_NULL)
+					if (inner) {


alvarofe · 2017-03-06T08:11:56Z

Create a PR in r2r uploading all those files

wargio · 2017-03-07T12:27:16Z

@alvarofe @radare
Merge, please!

radare · 2017-03-07T19:13:03Z

i'll merge after having those changes fixed. thanks

wargio · 2017-03-07T19:31:04Z

what changes?
if you are talking about alvarofes ones, they are already solved.

radare · 2017-03-07T18:21:19Z

libr/util/r_asn1.c

@@ -311,14 +325,11 @@ RASN1Object *asn1_parse_header (const ut8 *buffer, ut32 length) {
 	if (!object) {
 		return NULL;
 	}
+	memset (object, 0, sizeof(RASN1Object));


Missing soace before (

radare · 2017-03-07T18:21:30Z

libr/util/r_asn1.c

@@ -63,8 +76,9 @@ RASN1String *r_asn1_stringify_string (const ut8 *buffer, ut32 length) {
 		return NULL;
 	}
 	memcpy (str, buffer, length);
+	sanitize(str, length + 1);


Anither missing soace

I mean space

radare · 2017-03-07T18:29:51Z

libr/util/r_x509.c

@@ -165,8 +172,7 @@ bool r_x509_parse_extensions (RX509Extensions *ext, RASN1Object * object) {
 	for (i = 0; i < object->list.length; ++i) {
 		ext->extensions[i] = (RX509Extension*) malloc (sizeof (RX509Extension));
 		if (!r_x509_parse_extension (ext->extensions[i], object->list.objects[i])) {
-			free (ext->extensions[i]);
-			ext->extensions[i] = NULL;
+			R_FREE(ext->extensions[i]);


Missing space

radare · 2017-03-07T18:35:05Z

libr/util/r_asn1.c

+	}
+	char* e = s;
+	while (s <= (e + len)) {
+		if(*s < 0x20 || *s > 0x7E)


Theres a function in rutil that does this already

actually no. i've checked, and no code does ASCII only.
if yes, i'll be happy to remove it.

radare · 2017-03-07T19:09:13Z

libr/util/r_asn1.c

@@ -311,14 +325,11 @@ RASN1Object *asn1_parse_header (const ut8 *buffer, ut32 length) {
 	if (!object) {


use R_NEW0 instead of malloc and remove the memset

radare · 2017-03-07T19:10:05Z

libr/util/r_pkcs7.c

 	length = 2048 + (container->signedData.certificates.length * 1024);
+	if(!length) return NULL;


missing space and missing braces

radare · 2017-03-07T19:10:18Z

libr/util/r_pkcs7.c

 	length = 2048 + (container->signedData.certificates.length * 1024);
+	if(!length) return NULL;
 	buffer = (char*) malloc (length);


use calloc and remove the memset

radare · 2017-03-07T19:12:14Z

libr/util/r_x509.c

@@ -6,6 +6,9 @@

 #include "r_x509_internal.h"

+#define MOVE_PTR(dst, src)			((dst) = (src)); \


wrong spacing and indentation

radare · 2017-03-07T19:42:32Z

this is if (IS_PRINTABLE(*s))

…

On 07 Mar 2017, at 20:39, Giovanni ***@***.***> wrote: @wargio commented on this pull request. In libr/util/r_asn1.c <#6925 (comment)>: > @@ -10,6 +10,19 @@ const char* _hex = "0123456789abcdef"; +static char* sanitize(char *s, ut32 len) { + if (!s) { + return NULL; + } + char* e = s; + while (s <= (e + len)) { + if(*s < 0x20 || *s > 0x7E) actually no. i've checked, and no code does ASCII only. if yes, i'll be happy to remove it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6925 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3-llY9nklwNBB2A97EtxRu02fKOwGOks5rjbJ7gaJpZM4MToc7>.

alvarofe · 2017-03-07T20:43:00Z

Do not merge still it crash

air:r2-regressions alvaro$ r2 bins/fuzzed/SEGV-a94-220-6d2
Warning: Invalid import directory size: 0xffff is now 0xa5b
=================================================================
==79370==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000027b53 at pc 0x0001043036f3 bp 0x7fff5ed8d890 sp 0x7fff5ed8d8
88
READ of size 1 at 0x602000027b53 thread T0
    #0 0x1043036f2 in sanitize r_asn1.c:20
    #1 0x1043035e2 in r_asn1_stringify_string r_asn1.c:80
    #2 0x1042fb7a6 in r_x509_parse_name r_x509.c:124
    #3 0x1042fd42e in r_x509_parse_tbscertificate r_x509.c:205
    #4 0x1042fe60a in r_x509_parse_certificate r_x509.c:262
    #5 0x1042f27e3 in r_pkcs7_parse_extendedcertificatesandcertificates r_pkcs7.c:53
    #6 0x1042f5abe in r_pkcs7_parse_signeddata r_pkcs7.c:264
    #7 0x1042f6618 in r_pkcs7_parse_cms r_pkcs7.c:307
    #8 0x10179487f in bin_pe_get_certificate pe.c:1991
    #9 0x101786218 in bin_pe_init pe.c:2032
    #10 0x101786641 in Pe32_r_bin_pe_new_buf pe.c:3050
    #11 0x101763fc0 in load_bytes bin_pe.c:32
    #12 0x10159ed87 in r_bin_object_new bin.c:1203
    #13 0x10159cfba in r_bin_file_new_from_bytes bin.c:1428
    #14 0x10159c4ab in r_bin_load_io_at_offset_as_sz bin.c:974
    #15 0x1015993f7 in r_bin_load_io_at_offset_as bin.c:988
    #16 0x101599181 in r_bin_load_io bin.c:831
    #17 0x101071856 in r_core_file_do_load_for_io_plugin file.c:429
    #18 0x10106e4bb in r_core_bin_load file.c:552
    #19 0x100e73b90 in main radare2.c:916
    #20 0x7fff9a6c15ac in start (libdyld.dylib+0x35ac)

-----
air:r2-regressions alvaro$ r2 bins/fuzzed/heap-buffer-overflow-2ce-030-5a6
Warning: Invalid import directory size: 0xffff is now 0xa73
=================================================================
==80554==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d00001df00 at pc 0x00010fe29c9e bp 0x7fff53246290 sp 0x7fff532462
88
READ of size 1 at 0x61d00001df00 thread T0
    #0 0x10fe29c9d in asn1_parse_header r_asn1.c:327
    #1 0x10fe2a621 in r_asn1_count_objects r_asn1.c:395
    #2 0x10fe2aa36 in r_asn1_create_object r_asn1.c:414
    #3 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #4 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #5 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #6 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #7 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #8 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #9 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #10 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #11 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #12 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427
    #13 0x10fe2ace9 in r_asn1_create_object r_asn1.c:427

alvarofe · 2017-03-07T21:30:58Z

This fix the crash, update it please. remove the gotcha message with eprintf and fix it correctly please

diff --git a/libr/util/r_asn1.c b/libr/util/r_asn1.c
index 37fcdfaf8..41e72fc85 100644
--- a/libr/util/r_asn1.c
+++ b/libr/util/r_asn1.c
@@ -11,19 +11,6 @@
 
 const char* _hex = "0123456789abcdef";
 
-static char *sanitize(char *s, ut32 len) {
-	if (!s) {
-		return NULL;
-	}
-	char* e = s;
-	while (s <= (e + len)) {
-		if (!IS_PRINTABLE(*s))
-			*s = '.';
-		s++;
-	}
-	return e;
-}
-
 RASN1String *r_asn1_create_string (const char *string, bool allocated, ut32 length) {
 	RASN1String *s;
 	if (!string || !length) {
@@ -72,12 +59,12 @@ RASN1String *r_asn1_stringify_string (const ut8 *buffer, ut32 length) {
 	if (!buffer || !length) {
 		return NULL;
 	}
-	str = (char*) malloc (length + 1);
+	str = (char*) calloc (1, length + 1);
 	if (!str) {
 		return NULL;
 	}
 	memcpy (str, buffer, length);
-	sanitize (str, length + 1);
+	r_str_filter (str, length);
 	str[length] = '\0';
 	return r_asn1_create_string (str, true, length);
 }
diff --git a/libr/util/r_pkcs7.c b/libr/util/r_pkcs7.c
index 2796781ae..9ece4a585 100644
--- a/libr/util/r_pkcs7.c
+++ b/libr/util/r_pkcs7.c
@@ -297,9 +297,7 @@ RCMS *r_pkcs7_parse_cms (const ut8 *buffer, ut32 length) {
 	memset (container, 0, sizeof (RCMS));
 	object = r_asn1_create_object (buffer, length);
 	if (!object || object->list.length != 2 || object->list.objects[1]->list.length != 1) {
-		if (object) {
-			free (object);
-		}
+		free (object);
 		free (container);
 		return NULL;
 	}
@@ -490,6 +488,11 @@ char* r_x509_signedinfo_dump (RPKCS7SignerInfo *si, char* buffer, ut32 length, c
 		}
 	}
 	s = si->digestEncryptionAlgorithm.algorithm;
+	if (p > length) {
+		eprintf ("GOTCHA\n");
+		return NULL;
+	}
+	//check if p is greater than length this pattern is throughtout all code
 	r = snprintf (buffer + p, length - p, "%sDigest Encryption Algorithm\n%s%s\n",
 				pad2, pad3, s ? s->string : "Missing");
 	p += (ut32) r;
@@ -526,6 +529,7 @@ char* r_x509_signedinfo_dump (RPKCS7SignerInfo *si, char* buffer, ut32 length, c
 			return NULL;
 		}
 	}
+	free (pad3);
 	return buffer + p;
 }
 
@@ -533,13 +537,12 @@ char *r_pkcs7_cms_dump (RCMS* container) {
 	RPKCS7SignedData *sd;
 	ut32 i, length, p;
 	int r;
-	char *buffer, *tmp = NULL;
+	char *buffer = NULL, *tmp = NULL;
 	if (!container) {
 		return NULL;
 	}
 	sd = &container->signedData;
 	p = 0;
-	buffer = NULL;
 	length = 2048 + (container->signedData.certificates.length * 1024);
 	if(!length) {
 		return NULL;
diff --git a/libr/util/r_x509.c b/libr/util/r_x509.c
index 51ae8e2b2..b1cbb19b0 100644
--- a/libr/util/r_x509.c
+++ b/libr/util/r_x509.c
@@ -555,7 +555,9 @@ char* r_x509_tbscertificate_dump (RX509TBSCertificate* tbsc, char* buffer, ut32
 		pad = "";
 	}
 	pad2 = r_str_newf ("%s  ", pad);
-	if (!pad2) return NULL;
+	if (!pad2) {
+		return NULL;
+	}
 	r = snprintf (buffer, length, "%sVersion: v%u\n"
 				"%sSerial Number:\n%s  %s\n"
 				"%sSignature Algorithm:\n%s  %s\n"
@@ -633,7 +635,7 @@ char* r_x509_tbscertificate_dump (RX509TBSCertificate* tbsc, char* buffer, ut32
 	return buffer + p;
 }
 
-char* r_x509_certificate_dump (RX509Certificate* certificate, char* buffer, ut32 length, const char* pad) {
+char* r_x509_certificate_dump(RX509Certificate* certificate, char* buffer, ut32 length, const char* pad) {
 //	RASN1String *signature,
 	RASN1String *algo = NULL;
 	ut32 p;
@@ -646,8 +648,12 @@ char* r_x509_certificate_dump (RX509Certificate* certificate, char* buffer, ut32
 		pad = "";
 	}
 	pad2 = r_str_newf ("%s  ", pad);
-	if (!pad2) return NULL;
-	if ((r = snprintf (buffer, length, "%sTBSCertificate:\n", pad)) < 0) return NULL;
+	if (!pad2) {
+		return NULL;
+	}
+	if ((r = snprintf (buffer, length, "%sTBSCertificate:\n", pad)) < 0) {
+		return NULL;
+	}
 	p = (ut32) r;
 	tbsc = r_x509_tbscertificate_dump (&certificate->tbsCertificate, buffer + p, length - p, pad2);
 	p = tbsc - buffer;
@@ -706,7 +712,9 @@ char* r_x509_crl_dump (RX509CertificateRevocationList *crl, char* buffer, ut32 l
 		pad = "";
 	}
 	pad3 = r_str_newf ("%s    ", pad);
-	if (!pad3) return NULL;
+	if (!pad3) {
+		return NULL;
+	}
 	pad2 = pad3 + 2;
 	algo = crl->signature.algorithm;
 	last = crl->lastUpdate;

wargio · 2017-03-08T00:02:17Z

FINALLY fixed.

@alvarofe can you check the crash again? thanks!

wargio · 2017-03-08T09:57:57Z

i hope to have fixed all frees now :|
i fucked up while testing the code..
next time before i'll push it, i'll review the diff.

radare · 2017-03-08T11:09:48Z

btw, passing the pointers as references seems like an anti-pattern compared to the rest of the code in r2. i would prefer not to change that.

radare · 2017-03-08T11:10:08Z

is responsability of the caller to nullify the pointer if that's going to be used later to avoid an UAF

radare · 2017-03-08T11:10:46Z

i can merge that because i think that fixing those segfaults is important, but i would prefer not to have ** pointers and the extra dereference needed for every access

wargio · 2017-03-08T11:25:22Z

Ok, I'll change it later

radare · 2017-03-08T11:33:12Z

ok merged for now, please send another PR rollbacking the **

Fixed SIGSEGV, fixed heap buffer overflow, heap use after free

b162e3a

alvarofe reviewed Mar 6, 2017

View reviewed changes

wargio added 4 commits March 6, 2017 10:10

removed useless checks on free()

7f9779f

removed forgotten debug printf, flavour

08153fd

Fixed sanitized string check

8f69133

Fixed string len

dfbc972

radare reviewed Mar 7, 2017

View reviewed changes

flavour, IS_PRINTABLE and used R_NEW0

f1bccaf

Fixed all memory leaks

83a25d1

fixed extension free

98b793f

radare merged commit 7e48260 into radareorg:master Mar 8, 2017

wargio deleted the asan-0 branch March 10, 2017 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed SIGSEGV, fixed heap buffer overflow, heap use after free #6925

Fixed SIGSEGV, fixed heap buffer overflow, heap use after free #6925

wargio commented Mar 6, 2017

alvarofe Mar 6, 2017

alvarofe Mar 6, 2017

alvarofe commented Mar 6, 2017

wargio commented Mar 7, 2017

radare commented Mar 7, 2017

wargio commented Mar 7, 2017 •

edited

Loading

radare Mar 7, 2017

radare Mar 7, 2017

radare Mar 7, 2017

radare Mar 7, 2017

radare Mar 7, 2017

wargio Mar 7, 2017

radare Mar 7, 2017

radare Mar 7, 2017

radare Mar 7, 2017

radare Mar 7, 2017

radare commented Mar 7, 2017 via email

alvarofe commented Mar 7, 2017

alvarofe commented Mar 7, 2017

wargio commented Mar 8, 2017

wargio commented Mar 8, 2017

radare commented Mar 8, 2017

radare commented Mar 8, 2017

radare commented Mar 8, 2017

wargio commented Mar 8, 2017

radare commented Mar 8, 2017

		@@ -311,14 +325,11 @@ RASN1Object asn1_parse_header (const ut8 buffer, ut32 length) {
		if (!object) {

		length = 2048 + (container->signedData.certificates.length * 1024);
		if(!length) return NULL;

		@@ -6,6 +6,9 @@

		#include "r_x509_internal.h"

		#define MOVE_PTR(dst, src) ((dst) = (src)); \

Fixed SIGSEGV, fixed heap buffer overflow, heap use after free #6925

Fixed SIGSEGV, fixed heap buffer overflow, heap use after free #6925

Conversation

wargio commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarofe commented Mar 6, 2017

wargio commented Mar 7, 2017

radare commented Mar 7, 2017

wargio commented Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radare commented Mar 7, 2017 via email

alvarofe commented Mar 7, 2017

alvarofe commented Mar 7, 2017

wargio commented Mar 8, 2017

wargio commented Mar 8, 2017

radare commented Mar 8, 2017

radare commented Mar 8, 2017

radare commented Mar 8, 2017

wargio commented Mar 8, 2017

radare commented Mar 8, 2017

wargio commented Mar 7, 2017 •

edited

Loading