Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 15 May 2013 00:49:35 +0400
From: Alexander Cherepanov <cherepan@...me.ru>
To: john-dev@...ts.openwall.com
Subject: Re: Yet more crashes

On 2013-05-03 01:37, Alexander Cherepanov wrote:
> Great, the following formats still crash:
>
> dynamic_21
> pkzip
> sunmd5

Fixed.

Tried it on hashes from selftests but it would be great if someone tests 
these formats on more real-life samples, especially pkzip.

-- 
Alexander Cherepanov

>From 0cc754fbf198e77a71ce36d459b080ad238eaa3c Mon Sep 17 00:00:00 2001
From: Alexander Cherepanov <cherepan@...me.ru>
Date: Tue, 14 May 2013 19:59:40 +0400
Subject: [PATCH 1/3] Improve robustness of valid() in pkzip format.

---
 src/pkzip_fmt_plug.c |   59 ++++++++++++++++++++++---------------------------
 1 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/src/pkzip_fmt_plug.c b/src/pkzip_fmt_plug.c
index c3f4bd4..d728030 100644
--- a/src/pkzip_fmt_plug.c
+++ b/src/pkzip_fmt_plug.c
@@ -175,8 +175,8 @@ static u8 *GetFld(u8 *p, u8 **pRet) {
 static int is_hex_str(const u8 *cp) {
 	int len, i;
 
-	if (!cp)
-		return 1; /* empty is 'fine' */
+	if (!cp || !*cp)
+		return 0; /* empty is NOT 'fine' */
 	len = strlen((c8*)cp);
 	for (i = 0; i < len; ++i) {
 		if (atoi16[ARCH_INDEX(cp[i])] == 0x7F)
@@ -208,7 +208,7 @@ unsigned get_hex_num(const u8 *cp) {
 static int valid(char *ciphertext, struct fmt_main *self)
 {
 	u8 *p, *cp;
-	int cnt, len, data_len;
+	int cnt, data_len;
 	u32 crc;
 	FILE *in;
 	const char *sFailStr;
@@ -224,54 +224,53 @@ static int valid(char *ciphertext, struct fmt_main *self)
 
 	p = &cp[7];
 	p = GetFld(p, &cp);
-	if (!p || *p==0) {
+	if (!is_hex_str(cp)) {
 		sFailStr = "Out of data, reading count of hashes field"; goto Bail; }
 	sscanf((c8*)cp, "%x", &cnt);
 	if (cnt < 1 || cnt > MAX_PKZ_FILES) {
 		sFailStr = "Count of hashes field out of range"; goto Bail; }
 	p = GetFld(p, &cp);
-	if (!p || *p==0 || *cp < '1' || *cp > '2') {
+	if (cp[0] < '1' || cp[0] > '2' || cp[1]) {
 		sFailStr = "Number of valid hash bytes empty or out of range"; goto Bail; }
 
 	while (cnt--) {
 		p = GetFld(p, &cp);
-		if ( !p || *cp<'1' || *cp>'3') {
+		if (cp[0]<'1' || cp[0]>'3' || cp[1]) {
 			sFailStr = "Invalid data enumeration type"; goto Bail; }
-		type = *cp - '0';
+		type = cp[0] - '0';
 		p = GetFld(p, &cp);
-		if ( !p || !is_hex_str(cp)) {
+		if (!is_hex_str(cp)) {
 			sFailStr = "Invalid type enumeration"; goto Bail; }
 		if (type > 1) {
 			p = GetFld(p, &cp);
-			sscanf((c8*)cp, "%x", &(complen));
-			if ( !p || !cp[0] || !is_hex_str(cp)) {
+			if (!is_hex_str(cp)) {
 				sFailStr = "Invalid compressed length"; goto Bail; }
-			sscanf((c8*)cp, "%x", &len);
+			sscanf((c8*)cp, "%x", &complen);
 			p = GetFld(p, &cp);
-			if ( !p || !cp[0] || !is_hex_str(cp)) {
+			if (!is_hex_str(cp)) {
 				sFailStr = "Invalid data length value"; goto Bail; }
 			p = GetFld(p, &cp);
-			if ( !p || !cp[0] || !is_hex_str(cp)) {
+			if (!is_hex_str(cp)) {
 				sFailStr = "Invalid CRC value"; goto Bail; }
 			sscanf((c8*)cp, "%x", &crc);
 			p = GetFld(p, &cp);
-			if ( !p || !cp[0] || !is_hex_str(cp)) {
+			if (!is_hex_str(cp)) {
 				sFailStr = "Invalid offset length"; goto Bail; }
 			sscanf((c8*)cp, "%lx", &offset);
 			p = GetFld(p, &cp);
-			if ( !p || !cp[0] || !is_hex_str(cp)) {
+			if (!is_hex_str(cp)) {
 				sFailStr = "Invalid offset length"; goto Bail; }
 			sscanf((c8*)cp, "%x", &offex);
 		}
 		p = GetFld(p, &cp);
-		if ( !p || (*cp != '0' && *cp != '8')) {
+		if ((cp[0] != '0' && cp[0] != '8') || cp[1]) {
 			sFailStr = "Compression type enumeration"; goto Bail; }
 		p = GetFld(p, &cp);
-		if ( !p || !is_hex_str(cp)) {
-			sFailStr = "data length value"; goto Bail; }
+		if (!is_hex_str(cp)) {
+			sFailStr = "Invalid data length value"; goto Bail; }
 		sscanf((c8*)cp, "%x", &data_len);
 		p = GetFld(p, &cp);
-		if ( !p || !is_hex_str(cp) || strlen((c8*)cp) != 4) {
+		if (!is_hex_str(cp) || strlen((c8*)cp) != 4) {
 			sFailStr = "invalid checksum value"; goto Bail; }
 		p = GetFld(p, &cp);
 		if (type > 1) {
@@ -284,7 +283,7 @@ static int valid(char *ciphertext, struct fmt_main *self)
 					fprintf(stderr, "Error loading a pkzip hash line. The ZIP file '%s' could NOT be found\n", cp);
 					return 0;
 				}
-				sFailStr = ValidateZipContents(in, offset, offex, len, crc);
+				sFailStr = ValidateZipContents(in, offset, offex, complen, crc);
 				if (*sFailStr) {
 					/* this error is listed, even if not in pkzip debugging mode. */
 					fprintf(stderr, "pkzip validation failed [%s] Hash is %s\n", sFailStr, ciphertext);
@@ -305,16 +304,17 @@ static int valid(char *ciphertext, struct fmt_main *self)
 			} else {
 				/* 'inline' data. */
 				if (complen != data_len) {
-					return 0;
-				}
-				if ( !p || !is_hex_str(cp) || strlen((c8*)cp) != data_len<<1) {
-					sFailStr = "invalid checksum value"; goto Bail;
-				}
+					sFailStr = "length of full data does not match the salt len"; goto Bail; }
+				if (!is_hex_str(cp) || strlen((c8*)cp) != data_len<<1) {
+					sFailStr = "invalid inline data"; goto Bail; }
 			}
-		}
+		} else {
+			if (!is_hex_str(cp) || strlen((c8*)cp) != data_len<<1) {
+				sFailStr = "invalid partial data"; goto Bail; }
+                }
 	}
 	p = GetFld(p, &cp);
-	return !strcmp((c8*)cp, "$/pkzip$");
+	return !strcmp((c8*)cp, "$/pkzip$") && !*p;
 
 Bail:;
 #ifdef ZIP_DEBUG
@@ -600,11 +600,6 @@ static void *get_salt(char *ciphertext)
 					salt->H[i].datlen = 384;
 				}
 			} else {
-				/* 'inline' data. */
-				if (salt->compLen != salt->H[i].datlen) {
-					fprintf(stderr, "Error, length of full data does not match the salt len. %s\n", ciphertext);
-					return 0;
-				}
 				salt->H[i].h = mem_alloc_tiny(salt->compLen, MEM_ALIGN_WORD);
 				for (j = 0; j < salt->H[i].datlen; ++j)
 					salt->H[i].h[j] = (atoi16[ARCH_INDEX(cp[j*2])]<<4) + atoi16[ARCH_INDEX(cp[j*2+1])];
-- 
1.7.2.5


>From 8615dbec4f7a97ca9df9844018d7831e46314d50 Mon Sep 17 00:00:00 2001
From: Alexander Cherepanov <cherepan@...me.ru>
Date: Tue, 14 May 2013 21:35:40 +0400
Subject: [PATCH 2/3] Make valid() in sunmd5 format robust.

---
 src/sunmd5_fmt_plug.c |   58 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/src/sunmd5_fmt_plug.c b/src/sunmd5_fmt_plug.c
index 60e6504..0b0e82a 100644
--- a/src/sunmd5_fmt_plug.c
+++ b/src/sunmd5_fmt_plug.c
@@ -102,10 +102,6 @@
 // it is salted, but very slow, AND there is no difference between 1 and multi salts, so simply turn off salt benchmarks
 #define BENCHMARK_LENGTH		-1
 
-// There 'ARE' more types, but we only handle these 2, at this time.
-#define MAGIC  "$md5,"
-#define MAGIC2 "$md5$"
-
 /* THIS one IS a depricated sun string, but for real:  $md5$3UqYqndY$$6P.aaWOoucxxq.l00SS9k0: Sun MD5 "password"  */
 /* $md5,rounds=5000$GUBv0xjJ$$mSwgIswdjlTY0YxV7HBVm0   passwd  This one was the python code from http://packages.python.org/passlib/lib/passlib.hash.sun_md5_crypt.html, but the rounds are busted. */
 
@@ -259,15 +255,55 @@ static void init(struct fmt_main *self)
 
 static int valid(char *ciphertext, struct fmt_main *self)
 {
-	char *cp;
-	if (strncmp(ciphertext, MAGIC, sizeof(MAGIC) - 1) && strncmp(ciphertext, MAGIC2, sizeof(MAGIC2) - 1))
+	char *pos;
+
+	/* Common prefix. */
+	if (strncmp(ciphertext, "$md5", 4))
 		return 0;
-	cp = strrchr(ciphertext, '$');
-	if (!cp) return 0;
-	if (strlen(cp) != 23) return 0;
-	while (*++cp)
-		if (atoi64[ARCH_INDEX(*cp)] == 0x7F)
+	ciphertext += 4;
+
+	/* Optional rounds. */
+	if (!strncmp(ciphertext, ",rounds=", 8) ||
+	    !strncmp(ciphertext, "$rounds=", 8)) {
+		pos = ciphertext += 8;
+		while (*ciphertext >= '0' && *ciphertext <= '9')
+			ciphertext++;
+		/* Accept only numbers from 0 to 999999? */
+		/* Zero-padding is ok */
+		if (ciphertext - pos < 1 || ciphertext - pos > 6)
 			return 0;
+	}
+	if (*ciphertext++ != '$')
+		return 0;
+
+	/* Salt per se. */
+	pos = ciphertext;
+	while (atoi64[ARCH_INDEX(*ciphertext)] != 0x7F)
+		ciphertext++;
+	/* Upto 16 salt chars? */
+	if (ciphertext - pos > 16)
+		return 0;
+
+	/* One or two $ */
+	if (*ciphertext++ != '$')
+		return 0;
+	if (*ciphertext == '$')
+		ciphertext++;
+
+	/* Hash per se. */
+	pos = ciphertext;
+	while (atoi64[ARCH_INDEX(*ciphertext)] != 0x7F)
+		ciphertext++;
+	/* Samples from CMIYC-12 have garbage in padding bits.
+	   Hence the check is disabled for now. */
+	if (ciphertext - pos != 22
+	    /* || atoi64[ARCH_INDEX(*(ciphertext - 1))] & 0x0F */)
+		return 0;
+
+	/* No garbage at the end */
+	if (*ciphertext)
+		return 0;
+
 	return 1;
 }
 
-- 
1.7.2.5


>From 76e2bc71b8a9cea4eac1ac8328ff3faea536790f Mon Sep 17 00:00:00 2001
From: Alexander Cherepanov <cherepan@...me.ru>
Date: Wed, 15 May 2013 00:36:38 +0400
Subject: [PATCH 3/3] Fix a crash in dynamic with an overlong salt.

---
 src/dynamic_fmt.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/dynamic_fmt.c b/src/dynamic_fmt.c
index 76e5cdd..b7e016e 100644
--- a/src/dynamic_fmt.c
+++ b/src/dynamic_fmt.c
@@ -741,6 +741,8 @@ static int valid(char *ciphertext, struct fmt_main *pFmt)
 
 	if (cp[cipherTextLen] && cp[cipherTextLen] != '$')
 		return 0;
+	if (strlen(&cp[cipherTextLen]) > SALT_SIZE)
+		return 0;
 	if (pPriv->dynamic_FIXED_SALT_SIZE && ciphertext[pPriv->dynamic_SALT_OFFSET-1] != '$')
 		return 0;
 	if (pPriv->dynamic_FIXED_SALT_SIZE > 0 && strlen(&ciphertext[pPriv->dynamic_SALT_OFFSET]) != pPriv->dynamic_FIXED_SALT_SIZE) {
-- 
1.7.2.5


Powered by blists - more mailing lists

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ