[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150906182508.GA4578@openwall.com>
Date: Sun, 6 Sep 2015 21:25:08 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags
Kai, magnum, Jim -
I primarily address this reply to Kai, but I'd like at least magnum and
Jim to read it as well, and to help with further discussion.
On Thu, Aug 27, 2015 at 10:40:12AM +0800, Kai Zhao wrote:
> On Tue, Jul 28, 2015 at 10:03 AM, Kai Zhao <loverszhao@...il.com> wrote:
> >
> > Finally, there are no formats have obvious problems with
> > FMT_SPLIT_UNIFIES_CASE flag. But there are 3 formats: MediaWiki,
> > PHPS, PHPS2 which do not contain the flag and their split do not change
> > case. But the 3 formats finally has the flag: FMT_SPLIT_UNIFIES_CASE.
>
> I created a patch to detect FMT_SPLIT_UNIFIES_CASE error:
>
> https://github.com/loverszhaokai/JohnTheRipper/commit/0148c3ddbce427f6c484e696021738180e2c14f7
I took a look. This patch tries to detect only the cases where the flag
is (not) set inconsistently with the implementation of split(). That's
fine and desirable, but there's also the aspect of some formats needing
both the flag and the implementation yet lacking both - but you don't
check for that. You could detect this other problem by trying to
"crack" a hash with the case of some characters in it altered. In the
simplest form of the problem, you'd see both binary() and salt()
returning the same values as before for hash encodings altered in this
way (you'd also check salt() in case you inadvertently modified the salt
rather than the binary). However, if binary() is implemented such that
it simply returns the string or its portion verbatim (doesn't actually
decode it) and the encoding is left for crypt_all() or/and cmp_*() to
handle, then you have to run the full set of functions as are run during
cracking. So you might prefer to just do that all the time.
As to your current patch above, I didn't review it closely, but I guess
you can just get it in (submit a pull request). You'll add to it later,
with separate commits.
> The result is almost the same as the previous test. There are 4 formats:
> dynamic=md5($p), MediaWiki, PHPS and PHPS2 which do not contain
> the flag and their split do not change case. But the 4 formats finally has
> the flag FMT_SPLIT_UNIFIES_CASE set. This probably caused by the
> following code:
>
> int dynamic_SETUP(DYNAMIC_Setup *Setup, struct fmt_main *pFmt)
> {
> [...]
> if ( (Setup->flags & ...)) == 0) {
> pFmt->params.flags |= FMT_SPLIT_UNIFIES_CASE;
> [...]
> }
> [...]
> }
>
> Is this ok for the 5 formats ? Should I add these 4 formats to whitelist ?
Probably not. I expect that we have many formats that don't handle
FMT_SPLIT_UNIFIES_CASE right, in one of several ways described above.
That's at least 3 possible errors:
1. Setting the flag, but not implementing case unification in split().
2. Not setting the flag, but implementing case unification in split().
3. Needing to implement case unification in split() and set the flag,
but not doing any of this. Alternatively, needing to refuse to handle
wrong-case encodings (e.g., insist on all-lowercase hex if that's what
the target system uses).
We probably have some of each of these 3.
There might also be:
4. Not needing to implement case unification in split() and set the
flag, yet doing both of this.
But I find #4 unlikely, except for cases where the alternative solution
to #3 could have been implemented.
magnum, Jim - can you review the specific formats that Kai reported
potential problems for here? Or have you already done that?
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.
