[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090710013856.GA30026@openwall.com>
Date: Fri, 10 Jul 2009 05:38:56 +0400
From: Solar Designer <solar@...nwall.com>
To: john-users@...ts.openwall.com
Subject: Re: patch for new john format: phpass (also works for phpBBv3)
On Thu, Jul 09, 2009 at 07:48:29PM -0500, JimF wrote:
> 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.
This sounds about right. Keep in mind, though, that a specific web app
could choose to generate salts on its own (rather than use my original
phpass code) and do so poorly - then you'd have matching salts, and the
binary hashes would be of more importance.
> 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.
Great.
> 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.
Doing a typecast inside the function is correct.
> > 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.
No, I was referring to lines like:
+ sixbits = (unsigned)strchr(itoa64, *pos++);
+ sixbits -= (unsigned)itoa64;
Here you unnecessarily get into the dark area of casting pointers to
integers (even to integers of different size on some architectures).
A more correct way to write this would be:
sixbits = strchr(itoa64, *pos++) - itoa64;
that is, doing pointer math, which is well-defined (the result is of
signed integer type known as ptrdiff_t on modern systems). This assumes
that the string had been pre-validated, such that strchr() wouldn't
return NULL (I think you do have such pre-validation in place).
Of course, changing to atoi64[ARCH_INDEX(...)] is an even better fix.
> 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.
No, we have not agreed about anything like that. I merely asked you to
post to the list about your bigger set of changes. For now, I'd prefer
you to release your phpass-3.diff stuff separately, if you don't mind.
You can also have it as part of your bigger patch, if you need that in
order to take advantage of other changes that you're making.
> I will post that message shortly, to the list as you wanted.
Thank you!
Alexander
--
To unsubscribe, e-mail john-users-unsubscribe@...ts.openwall.com and reply
to the automated confirmation request that will be sent to you.
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.
