Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.