lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Wed, 03 Sep 2014 17:17:47 -0400
From: Bill Cox <>
Subject: A review per day - MCS_PHS

Hash: SHA1

I almost decided to pass on reviewing MCS_PHS, because it's not really
something I can evaluate properly, especially compared to some hashing
experts on this list.  As far as I can tell, the main point is to
introduce password hashing using the MCSSHA-8 function, the latest in
a series of hash functions, one (or more?) of which was in the SHA3
competition.  I see that there were security concerns about the prior
versions, and I have absolutely no idea if there has been any
cryptanalysis of this latest version.  To me, it just looks like
hashing code :-)

IMO, there is not much to review in MCS_PHS other than the the
MCSSHA-8 hash function, which seems to be the majority of what is
interesting here.  The wrapper is interesting, but it's a basic
PBKDF2-like scheme, without an HMAC call, which I prefer.  It gets the
input hashing right, without collisions, unlike PBKDF2-HMAC.

I would prefer to see the code have the main part factored out, and
instead of a do {...} while(0) with breaks in the middle, it could
just return.  That would make the odd testing of loop conditions after
the loop, with breaks inside the loop go away, too.  The outer
if(t_cost) statement can be removed on line 88 since the for loop wont
execute if t_cost is 0.

On line 66, I became confused why hashing calls are needed to reduce
the hash length from 64 to outlen.  Why not just truncate?

Part of the difficulty reading this code comes from the declaration of
the hash function:

HashReturn Hash(DataLength hashbitlen,
                       const BitSequence *data,
                       DataLength databitlen,
                       BitSequence *hashval);

The parameter order is screwy!  I hope that word doesn't offend the
author...  Normally we either keep length parameters next to the
pointer they refer to, or the lengths together and the pointers
together.  If these names mean what I think they mean, then there will
be a lot of bugs when people try to use this.

The code on line 91 confuses me as well:

        if(outlen != 64) {
            if(Hash(512,hash,outlen<<3,hash) ||
Hash(outlen<<3,hash,512,hash)) {
                dwErr = 2;
        } else {
            for(i = 0; i < 64; i++)hash[i] += (uint8_t)i;
            if(Hash(512,hash,512,hash)) {
                dwErr = 2;

For some reason, 64 is a magic number.  The upper hash calls seem to
expand the outlen hash to 512 bits, and then compress them back to
outlen.  Why?  The lower code adds different numbers to the hash value
and then calls hash again.  I'm really confused.  If Hash is secure,
why bother with this?

I think the internal buffers need to be cleared before returning.
Also, I didn't see the hashing context being cleared in the
finalization.  These of course are no big deal, but I was hoping the
hashing algorithm was ready for deployment, but the details aren't
there, yet.

So, overall, I don't know how to benchmark or rate MCS_PHS.  I see
some oddities in the code, and the way it was written makes me fearful
of using it.  If this is another one of those wonderful mathematical
entries please ignore my review :-)  This looks like code written by a

Version: GnuPG v1


Powered by blists - more mailing lists