[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150929150311.GA547@openwall.com>
Date: Tue, 29 Sep 2015 18:03:12 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Reading loaded password hashes inside crypt-all.
Hi Sayantan,
> On Tue, Sep 29, 2015 at 3:05 PM, Sayantan Datta <std2048@...il.com> wrote:
> > Are password hashes removed from database as they are cracked ?
Yes, but only from fields actually used during a given cracking run, and
not necessarily right away. (In "single crack" mode, there's postponed
removal.)
> > Apparently
> > there are discrepancies between salt->count and number of remaining hashes
> > in salt->list. While salt-count is updated, it seems like salt->list is not
> > updated to reflect the cracked hashes.
> >
> > pw = salt -> list;
> > i = 0;
> > do {
> > bin = (int *)pw -> binary;
> > if (bin != NULL) {
> > store_binary();
> > i++ ;
> > }
> > } while ((pw = pw -> next)) ;
> >
> > Running this loop in the middle of cracking results in 'i' being greater
> > than 'salt->count' !!
Yes, and that's "normal". See crk_remove_hash(). It always updates
crk_db->password_count and salt->count, but it only updates the list
when there's no bitmap, and it only updates the bitmap and sometimes
the hash table when there is a bitmap.
OK, you don't actually rely on the list being updated - you only rely on
the removed entries being marked with "binary = NULL". That's easier
for cracker.c to support, and it happened to support this until recently.
> > Some recent change might have triggered this issue.
> > Earlier, such behavior were not noticed.
> >
> > This broke an optimization where hashes are re-loaded onto GPU after some
> > of them are cracked, resulting in better speed!!
Oh. We probably need to do something about this.
On Tue, Sep 29, 2015 at 04:18:57PM +0530, Sayantan Datta wrote:
> This one line patch fixes the issues but I'm not sure to commit it.
There was a good reason for this change (minimizing writes to the
database when running with --fork), so I'd rather not revert it.
> diff --git a/src/cracker.c b/src/cracker.c
> index 0456114..3276e2f 100644
> --- a/src/cracker.c
> +++ b/src/cracker.c
> @@ -308,7 +308,7 @@ static void crk_remove_hash(struct db_salt *salt, struct db_password *pw)
> * "single crack" mode, so mark the entry for removal by "single crack" mode
> * code if that's what we're running, instead of traversing the list here.
> */
> - if (crk_guesses)
> + //if (crk_guesses)
> pw->binary = NULL;
> }
To summarize, the issue is that cracker.c makes assumptions about how
the database is used in a given cracking run, and those assumptions
might not hold true for formats' own use of the database from
crypt_all(). Maybe we need to introduce a format flag and check it
here? Like this:
if (crk_guesses || (crk_params.flags & FMT_REMOVE))
pw->binary = NULL;
and your formats that need this would set FMT_REMOVE.
Moreover, when your code sees "binary = NULL", it will also remove that
entry from the list. crk_remove_hash() doesn't do this when there's a
bitmap to avoid traversing the list on every guess, but you're
traversing the list for transfer to GPU anyway.
The meaning of FMT_REMOVE would then be: if the generic cracker code
doesn't remove a cracked password from salt->list on its own, it must
nevertheless mark that entry for removal by setting its "binary = NULL",
and the format's crypt_all() will then eventually remove the entry from
the list.
How does this sound to you?
Thanks,
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.
