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.