Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 8 Sep 2015 23:42:15 +0800
From: Kai Zhao <loverszhao@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags

Hi Alexander,

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/

>
>> 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:
>>
>>
>> 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.

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 ?

https://github.com/magnumripper/JohnTheRipper/commit/cc5ae475bad53ca46b9c74a82848bc86c6b9c314


> 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 ?


Thanks,

Kai

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.