Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 9 Jul 2009 19:48:29 -0500
From: "JimF" <jfoug@....net>
To: <john-users@...ts.openwall.com>
Subject: Re: patch for new john format: phpass (also works for phpBBv3)

>> This version adds salt hash, binary hashes and sets the copyright data to 
>> be what was recommended.
> 
> Great.

the hash_salt was really needed for loader functions.  The binary_hash
stuff was probably not needed as much, due to this format having
such a good salt value.

I am still taking baby steps in the code.  I am working out just what
parts need to be set for each 'type' of format.

> I see that you're doing some weird
> stuff with strchr() calls on itoa64.  I recommend that you use
> atoi64[ARCH_INDEX(...)] instead, 

Done.  Again, I am still learning, and just getting my feet wet. But the
change to atoi64[ARCH_INDEX(...)]  does make the code much cleaner.

The one issue I had is:

static void phpassmd5_set_salt(void*), so using ARCH_INDEX()
I get this problem (in VC at least), claiming:
   'Error C2036: 'void *' : unknown size
so, here I had to typecast to char* within the ARCH_INDEX macro. Of
course, I use a char* in the parameter of the function (C lets me write
code with different parameters), but I would rather have the param be
proper, and typecast internal.

> Also, in phpassmd5_binary() you convert pointers, which may be
> 64-bit, to (unsigned), which is usually 32-bit.  Chances are that this
> will work right anyway, because you only use pointer differences in the
> end (so dropping the high 32 bits shouldn't affect the results), yet
> this is wrong.  Switching to the use of atoi64[ARCH_INDEX(...)] will
> eliminate the need for the pointer math anyway.

Actually, that conversion is simply taking a 16 byte array of binary
stuff.   It then typecasts to 32 bit array, dereferences first element
and strip's of the lowest X bits.  The exact same method is used within
the phpassmd5_get_hash_X functions, against the just encrypted data.
The typecasts should line up, and match just fine, in either a 64bit
or 32 bit system.   All this is doing is grabbing the highest 8 bits 
(for the get_hash_0 and get_binary_0) from the 16 byte hash, or it
gets the the 4th byte if the system is BIG_ENDIAN.  However, both
get_hash_x() and get_binary_x() do the same way, so even though a
different byte of the buffer is returned for LITTLE vs BIG endian,
it will still function properly.  Now, if there are performance gains
to be had on 64 bit systems for staying away from 32 bit access, then
I could change the typecasts to (unsigned) so that the 64 bit op-codes
are used.  However, I doubt that would make any difference at all, 
since the 2048 MD5 hash calls for EACH password, far outweigh any
optimizations in this part of the code, :)
 
> P.S. Please don't over-quote when you post to the list.  

10-4.  Sorry.

I now have a phpass-3.diff (but I need to test some).  But as we talked
about before (off list), I will look at getting this out in the bigger
set of changes I have.   I will post that message shortly, to the list
as you wanted.

Jim.

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.