[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 13 Aug 2012 15:49:05 -0500
From: "jfoug" <jfoug@....net>
To: <john-dev@...ts.openwall.com>
Subject: RE: Serious bug in -fixes and all other branches
>From: magnum [mailto:john.magnum@...hmail.com]
>
>We have a serious loader problem. It often segfaults while reading a pot
>file containing other formats than what we are loading. This is in all
>branches, including -fixes :(
>
>I think it was introduced in this commit in -fixes of Aug 9:
>
>1c637ba "dynamic: $HEX$ fixes" by JimF
This was done, for the HEX issues in dynamic. I really need to take some time, and get my head wrapped around all of the implications.
For 'fixes' version, lets simply roll back these changes to loader. The side effects, are that any found password of dynamic type, will not be removed from JtR on its next run, if the .pot line for that hash had a $HEX$ added. But this will remove the other 'bad' side effects of this change.
>specifically, these lines in ldr_load_pot_line() in loader.c:
>
>- if (format->methods.valid(ciphertext,format) != 1) return;
>-
>- ciphertext = format->methods.split(ciphertext, 0);
>...
>+ if (format->methods.valid(ciphertext, format) != 1) {
>+ ciphertext = format->methods.split(ciphertext, 0);
>+ if (format->methods.valid(ciphertext, format) != 1)
>+ return;
>+ } else
>+ ciphertext = format->methods.split(ciphertext, 0);
>
>after that patch, we call split() even though valid() returned false. I
>believe that is wrong, right? If this is supposed to be allowed, I fear
>we have a lot of work to do in a lot of formats :-/
>
>Also, I'm not sure what this has to do with $HEX$? Is some of that
>functionality done in dynamic's split()? So maybe if the above is
>reverted/fixed, the $HEX$ functionality need to be revised?
Dynamic modifies some found hashes, (the salts), changing them to $HEX$ type. Upon reload, if this is not 'undone', JtR will not detect and remove these, since it is not very smart in detecting duplicates. This is 'similar' to a format which lacks the SPLIT_UNIFIES_CASE. The hash is the same, but loader simply strcmp's to check.
This problem is more difficult than it appears, since the INPUT file may have $HEX$ values, and thus, within loader, those should not be undone. I am at a quandary about this issue. It may be that I have to modify the part of dynamic that makes the initial change, right before storing the .pot line, and not use $HEX$, but use some 'internal' value, that is only set prior to the .pot writing, and THEN if we detect that string, we undo it at split time.
For the loader call, I probably need to ONLY perform the split logic, if the format is a FMT_DYNAMIC type, otherwise, simply revert to the original behavior, and return. For dynamic, we will call split to 'possibly' chop back the hex. The 'other' option, is to make valid more complex, and slower (it already is the slowest valid() function by far).
Jim.
>Bottom line: I do not dare committing any fix until the above questions
>are answered.
Lets roll the loader logic back, for the git versions.
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.
