[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150910164231.GA31103@openwall.com>
Date: Thu, 10 Sep 2015 19:42:32 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags
Kai,
On Tue, Sep 08, 2015 at 11:42:15PM +0800, Kai Zhao wrote:
> On Mon, Sep 7, 2015 at 2:25 AM, Solar Designer <solar@...nwall.com> wrote:
> >
> > 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.
>
> Created.
>
> https://github.com/magnumripper/JohnTheRipper/pull/1736/
Thank you.
> Since JimF has add the flag for MediaWiki, PHPS and PHPS2, I think
> I should add these formats to whitelist. Maybe also includes
> dynamic=md5($p). Should I ?
I don't see why you would want to whitelist anything. Do your checks
still detect errors after Jim's fixes? If so, why do they?
> > 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.
>
> I think the current patch can detect the #1, #2 error. For #3, I wrote
> a pseudo code based on your ideas.
>
> original:
>
> original_split_ret = split(ciphertext, 0, format);
> original_binary_ret = binary(ciphertext);
> original_salt_ret = salt(ciphertext);
>
> set_key();
>
> original_crypt_ret = crypt_all(&count, NULL);
> original_cmp_ret = cmp_all(binary, original_crypt_all);
>
> copy original binary -> original binary
> copy original salt -> original salt
>
> change ciphertext case:
>
> new_split_ret = split(ciphertext, 0, format);
> new_binary_ret = binary(ciphertext);
> new_salt_ret = salt(ciphertext);
>
> set_key();
>
> new_crypt_ret = crypt_all(&count, NULL);
> new_cmp_ret = cmp_all(binary, original_crypt_all);
>
> copy new binary -> new binary
> copy new salt -> new salt
>
> compare:
>
> if (original_split_ret != new_split_ret)
> goto should_set_fmt_split;
>
> if (original_binary_ret != new_binary_ret)
> goto should_set_fmt_split;
>
> if (original_salt_ret != new_salt_ret)
> goto should_set_fmt_split;
>
> if (original_crypt_ret != new_crypt_ret)
> goto should_set_fmt_split;
>
> if (original_cmp_ret != new_cmp_ret)
> goto should_set_fmt_split;
>
> if (original bianry != new binary)
> goto should_set_fmt_split;
>
> if (original salt != new salt)
> goto should_set_fmt_split;
>
> should_set_fmt_split and implement it in split()
>
> Do you think this conform to your ideas ?
No, the above is wrong or irrelevant. You've since posted something
more relevant in another message - I'll comment on that.
Thanks,
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.
