[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150530072939.GA19410@openwall.com>
Date: Sat, 30 May 2015 10:29:39 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Fuzzing Report on external mode
Hi Kai,
Thank you for testing my code, and for the report!
At least some of these "bugs" are by design. External mode lacks bounds
checking when indexing arrays and it does not trap nor prevent possible
overflow on integer division. In both cases, the behavior is similar to
what you'd observe if you did this in your C program - e.g., right in
JtR source code. There's room for improvement here. I had actually
thought of and had an idea on how to add optional bounds checking with
little performance impact, but this does not appear to be worth my time
(and possibly not worth the increase in code complexity and the
performance impact either; instead, it is possible to go in the opposite
direction and make external mode faster).
I thought we had the lack of bounds checking documented in doc/EXTERNAL,
but I can't find it in there now. :-( Maybe it was in the documentation
for early external mode prior to JtR 1.5 and was lost in the rewrite for
1.5 (back in 1998). Maybe we need to add this to the documentation now.
There's a known shortcoming in the external mode compiler, where it does
not detect assignments to other than proper lvalue until runtime. So if
you do e.g. "1 = 1;" it will crash at runtime rather than fail to
compile. I was too lazy to fix that.
On Wed, May 27, 2015 at 03:43:55PM +0800, Kai Zhao wrote:
> $ cat test_pw
> $apr1$a2Jqm...$grFrwEgiQleDr0zR4Jx1b.
Oh, just why aren't you moving to a faster hash by now, after I provided
this advice to you a week ago or so? I recommend that you use the dummy
format for your fuzzing. e.g.:
$dummy$64756d6d79
> 2. Bug analysis
> --------------------
>
> I created 5 issues on github, but there maybe only two bugs.
... or one, or none.
> The bugs are **core john bugs**.
>
> Segment Fault, Heap buffer overflow and Global buffer overflow:
> (The 4 issues maybe by the same bug)
>
> https://github.com/magnumripper/JohnTheRipper/issues/1358
> https://github.com/magnumripper/JohnTheRipper/issues/1360
> https://github.com/magnumripper/JohnTheRipper/issues/1363
These 3 are crashes in op_index, which suggests out of bounds array
access. However, I don't immediately see a bug like this fuzzed into
the external mode programs. Do you? Can you please post diffs of them
from their original versions?
... Oh, I just saw that in #1358 you got the size of boundaries_symbols
changed from 32 to 12. This explains it. Now we need to figure out the
other two.
Your guess is that this is the same kind of issue that you found and I
patched recently, so you're suggesting that we change the initial sp
from &c_stack[2] to &c_stack[4]. I think you're probably wrong, but
have you tried? Does it help?
Here are two reasons why I think you're wrong about this:
1. In correct external mode programs, op_index is only used when there's
already something on the stack (namely, the array and the index). This
is different from op_push*, which may also be used at top level.
2. If initial sp at &c_stack[2] wasn't good enough for proper uses of
op_index, the problem would be seen on --enable-asan builds without any
fuzzing. ... or is this in fact the case? If so, why didn't you spot
it sooner, along with the previous one you reported a long while ago?
Let's figure this out.
> https://github.com/magnumripper/JohnTheRipper/issues/1364
This is a crash in op_assign, and it is an instance of the known
shortcoming with lack of compile-time checking for proper lvalue.
FWIW, when compiling the same code as in this external mode's init()
with gcc, I get "error: lvalue required as left operand of assignment".
So external mode compiler's parsing of this weird expression matches
gcc's. That's good. Putting braces around endchar = '~' makes this
compile with gcc fine, and I guess it should make it work with the
external mode compiler "right", too.
> Floating point exception:
>
> https://github.com/magnumripper/JohnTheRipper/issues/1362
As expected. You may also do:
[List.External:Overflow]
void init()
{
int i;
i = (-2147483647 - 1) / -1;
}
void generate()
{
}
I expected you would find these 3, and you did, which confirms that your
methodology is working. That's good.
Now we need to figure out your crashes in op_index. I fully expected
you would find many crashes in op_index, but I expected those would be
obvious out of bounds accesses in the external mode programs. There's
something here that I don't understand yet.
... Oh, I just saw the bug in #1360 - it's the non-lvalue issue in:
boundaries_numbers[i++] = 193273=284;
This should indeed crash at runtime.
So all that remains is to find the bug in the external mode program in
#1363, to confirm it's not anything new and unexpected. Can you just
diff it against the original, please? And we'll most likely be done
with these "bugs", then... except that this reminds me that they need to
be documented (I thought the lack of bounds checking already was...)
Thanks again,
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.
