john-dev - Re: Cracking "Password Safe" files with JtR

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120509171308.GA21144@openwall.com>
Date: Wed, 9 May 2012 21:13:08 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Cracking "Password Safe" files with JtR

Dhiru, all -

In addition to my reply on john-users, here are some development-focused
comments.  Some of these may be relevant for other formats as well.

On Wed, May 09, 2012 at 09:21:02PM +0530, Dhiru Kholia wrote:
> 1. gcc pwsafe2john.c -o pwsafe2john  # from src/ folder

1. Adding -Wall exposes warnings about unused fread() return values -
please deal with those.

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.

3. There ought to be a way to write this more cleanly:

#if defined(__APPLE__) && defined(__MACH__)
#ifdef __MAC_OS_X_VERSION_MIN_REQUIRED
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
#define COMMON_DIGEST_FOR_OPENSSL
#include <CommonCrypto/CommonDigest.h>
#else
#include <openssl/sha.h>
#endif
#else
#include <openssl/sha.h>
#endif
#else
#include <openssl/sha.h>
#endif

without the repeated "#include <openssl/sha.h>".  Maybe something like:

#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

or, if you don't like the long #if line (although I think it's fine):

#if defined(__APPLE__) && defined(__MACH__)
#ifdef __MAC_OS_X_VERSION_MIN_REQUIRED
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
#define COMMON_DIGEST_FOR_OPENSSL
#include <CommonCrypto/CommonDigest.h>
#endif

#ifndef COMMON_DIGEST_FOR_OPENSSL
#include <openssl/sha.h>
#endif

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.

5. Is 32 a hard limit imposed by Password Safe itself?  If not, is it
arbitrary choice of yours or is it based on anything at all?  I suggest
that if we're to pick an arbitrary length limit, we make it such that we
avoid extra calls into the SHA-256 compression function.

#define PLAINTEXT_LENGTH	32

Update: I see that the slow loop does not in any way depend on password
length.  So we can just use an arbitrary value here.

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.

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.

8. In "#pragma omp parallel for" directives, it is preferable to use
default(none) and explicitly specify the shared variables.

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);
		}

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

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. ;-)
The cracked[] array approach does have the advantage of avoiding writes
and thus cache bounces in the typical case, though.  Not that it matters
for a slow hash (or non-hash), but it's a curious observation.

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?

Thanks,

Alexander
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.