Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ