[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120524132116.GA1582@openwall.com>
Date: Thu, 24 May 2012 17:21:16 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: allocating salts
Dhiru -
On Thu, May 24, 2012 at 04:58:45PM +0530, Dhiru Kholia wrote:
> static struct custom_salt {
> unsigned char userid[8 + 1];
> char unsigned hash[8];
> } *salt_struct;
What's the point in having a "hash" inside the salt struct above?
This looks weird to me. Also "salt_struct" actually being a pointer is
weird (if you make it a pointer, don't call it a struct). Yes, I am
nitpicking, but you asked for it. ;-)
> 1. #define SALT_SIZE sizeof(*salt_struct) and malloc'ing custom_salt
> structure for every salt (involves a single pointer copy but leaks
> memory) in get_salt().
How does that involve only a single pointer copy? sizeof(*salt_struct)
is the size of the actual struct - so that's what the loader.c code will
be copying. Did you possibly mean sizeof(salt_struct) here, which is
actually the size of a pointer given your weird declaration?
Well, this does waste a bit of memory on the extra pointer indirection.
(Not on the salts themselves, because the loader.c code would allocate
memory for them anyway. You're just doing its job manually, and you
make it allocate memory for pointers instead. Weird stuff.)
More importantly, this will prevent the detection of duplicate salts
from working. The loader.c code assumes that the salts themselves are
being returned by the salt() method. It won't know about the extra
pointer indirection. It will thus be comparing pointers and never see a
matching salt (since you're allocating memory for each salt anew).
On the other hand, in some of your "non-hash" formats crypt_all()
assumes that there's exactly one hash per salt, so it would fail if
there's a properly-detected matching salt. Maybe the solution would be
to move your crypt_all() logic into cmp_all() instead (since crypt_all()
is called per-salt, whereas cmp_all() is called per-loaded-hash), and to
have the hash tables disabled (to ensure that cmp_all() is in fact
called even if the hash per salt count is very high). The loader
doesn't allocate hash tables when the binary_hash[]() methods are all
fmt_default_binary_hash or NULL.
> 2. #define SALT_SIZE sizeof(struct custom_salt) and have a static
> custom_salt structure (involves whole buffer copy but doesn't leak
> memory)
This is the intended way of doing it. The whole buffer copy you're
probably referring to is done in loader.c, so it does not affect
cracking speed. The static structure you mentioned above may be local
to the salt() method. It is not the same structure that is used
between set_salt() and crypt_all(), or at least it doesn't have to be.
The question of whether/what gets copied on set_salt() is a separate
one. Initially, I intended it to actually copy the salt into a static
buffer (typical salts were tiny, so this was the essential approach) -
however, I think JtR does not actually depend on that. I think you may
instead have set_salt() merely record a pointer to the supplied salt,
and crypt_all() dereference that pointer.
To summarize and make this more specific, I think the following would
work (untested, typed into this message only):
typedef struct {
unsigned char userid[8 + 1];
} salt_t;
#define SALT_SIZE sizeof(salt_t)
static salt_t *current_salt;
static void *salt(char *ciphertext)
{
static salt_t out;
...
return &out;
}
static void set_salt(void *salt)
{
current_salt = salt;
}
static void crypt_all(int count)
{
... use current_salt->user_id, etc. ...
}
Alternatively, you may use:
static salt_t current_salt;
static void set_salt(void *salt)
{
current_salt = *(salt_t *)salt;
}
static void crypt_all(int count)
{
... use current_salt.user_id, etc. ...
}
The latter may actually be faster for small salts or/and if crypt_all()
loads the contents of the salt struct into registers many times (these
loads might be quicker without the extra pointer indirection and with
placement of the salt copy near other data that crypt_all() operates on).
These examples assume the usual crypt_all() that only does hashing (so
it does not care of there are multiple loaded hashes for a given salt),
but not computed vs. loaded hash comparisons.
Alexander
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.
