john-users - Re: patch for new john format: phpass (also works for phpBBv3)

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 9 Jul 2009 21:51:50 -0500
From: "JimF" <jfoug@....net>
To: <john-users@...ts.openwall.com>
Subject: Re: patch for new john format: phpass (also works for phpBBv3)


----- Original Message ----- 
From: "Solar Designer" <solar@...nwall.com>
To: <john-users@...ts.openwall.com>
Sent: Thursday, July 09, 2009 8:38 PM
Subject: Re: [john-users] patch for new john format: phpass (also works for 
phpBBv3)


> 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 see that now, and it is part of the phpass-3.diff.  One of the first 
things
I did, was to build the phpassmd5_binary() function.  It was not well
done, but 'worked' for me.  I put a comment there, because I also did not
like it.  I did not know how to use atoi64 at that time, and saw examples
'similar' to what I had done, in some other _fmt.c files, so went with that,
but knew it was bad.   However, looking for a bug in the wrong place,
did show some other issues (in the non-mmx code within the get_hash_x()
functions.  Now those functions are much simpler, with no conditional
compilation blocks.

The phpassmd5_binary is now done with atoi64[ARCH_INDEX()] and
it is much simpler, cleaner and easier to follow.  And yes, I do see the
portablity issues in the original code.  Worked on my box, but might have
had problems in other places.

>> 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.

I did not mean to infer the changes were going to be the way it is, just
that I was going to post, and put them out.   I have some additional
clean up to do, then get them into a couple diffs, so that they will
'intermix' and properly patch.

>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.


-- 
To unsubscribe, e-mail john-users-unsubscribe@...ts.openwall.com and reply
to the automated confirmation request that will be sent to you.