Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<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

Your e-mail address:

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