[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 10 May 2012 10:42:51 +0530
From: Dhiru Kholia <dhiru.kholia@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: Cracking "Password Safe" files with JtR
On Wed, May 9, 2012 at 10:43 PM, Solar Designer <solar@...nwall.com> wrote:
> Dhiru, all -
>> 1. gcc pwsafe2john.c -o pwsafe2john # from src/ folder
>
> 1. Adding -Wall exposes warnings about unused fread() return values -
> please deal with those.
I missed checking the program with -Wall. Will fix this.
> 2. The benchmark speeds for many salts and one salt are almost the same.
> Please set BENCHMARK_LENGTH to -1 to report just one of these.
Agreed and will fix.
> 3. There ought to be a way to write this more cleanly:
> #if defined(__APPLE__) && defined(__MACH__) && \
> defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && \
> __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
> #define COMMON_DIGEST_FOR_OPENSSL
> #include <CommonCrypto/CommonDigest.h>
> #else
> #include <openssl/sha.h>
> #endif
This looks good. Will use this for other formats too.
> 4. Maybe change the FORMAT_NAME from:
>
> #define FORMAT_NAME "Password Safe"
>
> to:
>
> #define FORMAT_NAME "Password Safe SHA-256"
>
> We mention the underlying crypto primitives in this way in other formats.
Okay, will fix this.
> 6. You have inconsistent uses of "#ifdef _OPENMP" vs. "#if defined
> (_OPENMP)" (there are several of each kind). I suggest that we
> standardize on "#ifdef _OPENMP". Of course, this is unimportant.
Will fix this.
> 7. Your get_salt() makes memory allocations and it leaks memory.
> get_salt() is not supposed to allocate memory for its result buffer.
> It may/should instead reuse the same result buffer with each call.
> It's the caller's responsibility to allocate memory and copy the
> returned salt value when needed.
Is the memory leak serious? Is get_salt called only once per hash? (I
think yes).
Currently we return / save only the address of dynamically allocated
salt_struct. I can fix this to save / return contents of salt_struct.
> 8. In "#pragma omp parallel for" directives, it is preferable to use
> default(none) and explicitly specify the shared variables.
Okay, will fix this.
> 9. Looks like you can simplify this:
>
> for(i = 0; i < salt_struct->iterations; i++) {
> SHA256_Init(&ctx);
> SHA256_Update(&ctx, hash, 32);
> SHA256_Final(hash, &ctx);
> }
> SHA256_Init(&ctx);
> SHA256_Update(&ctx, hash, 32);
> SHA256_Final(hash, &ctx);
>
> to:
>
> for (i = 0; i <= salt_struct->iterations; i++) {
> SHA256_Init(&ctx);
> SHA256_Update(&ctx, hash, 32);
> SHA256_Final(hash, &ctx);
> }
>
Will implement this.
> 10. Instead of setting "cracked[index] = 0;" here:
>
> if(!memcmp(hash, salt_struct->hash, 32))
> cracked[index] = 1;
> else
> cracked[index] = 0;
>
> it may be better to zeroize "cracked" before the parallel loop.
> Otherwise you unnecessarily bounce the cache lines between the CPUs in
> the typical case. Of course, this should not significantly affect
> performance anyway since the slow loop is so slow (given a decent
> iteration count).
Will use the approach you suggested for ssh_format (using any_cracked variable).
> 11. In fact, do you need this cracked[] array approach at all? Wouldn't
> it be cleaner to store the computed hashes and to move the memcmp() to
> cmp_all(), etc. like it's normally done? I think you're simply so used
> to implementing support for non-hashes in this hackish way that you did
> not consider doing things in the more usual way when you could. ;-)
Habit issue ;). Too used to hackish way.
> 12. I assume that the lack of binary_hash*() and get_hash*() functions
> is OK since we really don't expect more than one hash per salt here,
> correct? Is it at all realistic that a matching salt would be seen
> across different Password Safe files?
Collision of 32 byte salt (which I am assuming is randomly generated)
on two different computers seems unlikely :-)
--
Cheers,
Dhiru
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.
