Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 7 Feb 2012 18:41:39 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Was: RE: [john-users] Segmentation fault in john-1.7.9-jumbo-5 under some conditions

On Mon, Feb 06, 2012 at 09:45:06AM -0600, jfoug wrote:
> I am able to reproduce this under cygwin (which I have had very little
> success debugging), but I can also have it happen under a VC build, which I
> do have ability to debug.   However, I am not comfortable in any knowledge
> of your compiler code.  I have written some myself, for gmp-ecm, OpenPGW,
> and even a simple parser/compiler in pass_gen.pl.  However, I have not dug
> in, or figured out the compiler code in john.  So I will list some
> 'findings', and possibly we can track down just what is going on.

I did not expect this would have anything to do with the compiler -
I thought it'd be some jumbo-specific change.

> op: [

This led me to suspect that we have an out of bounds access in the
Keyboard external mode.  Looking at it with this suspicion, I found that
restore() does not bother to initialize id[] beyond id[0] - and indeed
these are used as array indices by generate().  init() also does not
bother to initialize anything but id[0], but with the default minlength
of 1 this did not matter.

To reproduce the problem, I did:

--- compiler.c.orig	2011-10-29 17:41:39 +0400
+++ compiler.c	2012-02-07 18:03:45 +0400
@@ -840,6 +840,7 @@
 
 		c_code_start = mem_alloc((size_t)c_code_ptr);
 		c_data_start = mem_alloc((size_t)c_data_ptr);
+		memset(c_data_start, 'x', (size_t)c_data_ptr);
 	}
 
 	return c_errno;

This worked - got a segfault when restoring a Keyboard mode session
(using otherwise clean 1.7.9 on Linux).

Then I patched the issue:

--- john.conf.orig	2011-09-18 11:35:50 +0400
+++ john.conf	2012-02-07 18:14:52 +0400
@@ -604,8 +604,9 @@
 		}
 	}
 
-	id[0] = 0;
-	length = minlength;
+	length = 0;
+	while (length < minlength)
+		id[length++] = 0;
 }
 
 void generate()
@@ -637,7 +638,8 @@
 
 	/* Calculate the length */
 	length = 0;
-	while (word[length]) length++;
+	while (word[length])
+		id[length++] = 0;
 
 	/* Infer the first character index */
 	i = -1;

We need to get this fix into both main and jumbo trees.  Also, we may
consider making external mode behavior more deterministic when there are
uninitialized variables - either initialize everything to zero (then the
above two problems in Keyboard would not appear - or they could be
considered valid uses of implied initialization to zero) or deliberately
trigger problems like I did for testing.

Jim - thank you, and sorry for spending your time on this whereas the
bug turned out to be mine and something I could debug myself with just a
little bit more effort.

Alexander

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.