Date: Wed, 03 Sep 2014 17:17:47 -0400
From: Bill Cox <waywardgeek@...hershed.org>
To: discussions@...sword-hashing.net
Subject: A review per day - MCS_PHS

-----BEGIN PGP SIGNED MESSAGE-----
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;
                break;
            }
        } else {
            for(i = 0; i < 64; i++)hash[i] += (uint8_t)i;
            if(Hash(512,hash,512,hash)) {
                dwErr = 2;
                break;
            }
        }

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

Bill
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUB4V4AAoJEAcQZQdOpZUZrFwP/Aupvb2Eog0vb6hsu+oB3nqc
5/5aQIzbqJwhvb0IPUYE1a6wk90ENvB1uzJ+6e7+ir3DllT46cbWS+VkTHbYE1x5
tVSN7sjBMGAynSa5PM6NDPp+ObpunY/eoK+F84IQvZ+/lI40D3rhdVAopagTBwtF
hPkxyG23pZmA5ZS7s5WCcG+CoFw2YR97FH2pbHC0ttARH/26Hlv0hBXBFPDzLnRD
JZl9+di1TBsHCaI9pLDqIserC4m6FsgzopdgZQyFFoFa1VLmYjtY5RCKijVm3ubs
jxN8Ra5H7NFQRxf2eZQWuQ88ZYURadmJqK8Rv/XqIM/tc08pGitRfxnsBoHzyV8W
OibkgG0yMjw9X6DEkyakPahRabzlvi93x4cBT4n5cg3PL4Khn/DFwoP2SacAKatS
26WiWKTdvBP2Ilx9ZY1Mh2gP2SacsJtQgbFAHmVYhqVxoyFgo0xjVquGDzI99OVV
8TNdCcjAyiQ5swf7v9RBaKr9CsWfqlq2MAgjiy38y6dgW4k7DyIBxsdDByT4nOSK
cVK796DwVKrvN1LZWq70CiGYGFOoYRoHskSSfCDlKLXhJvH3rPFAEZSCxTmD2xKX
VkbHd/GNhYrH8pDBEkdQXWKxSl74R99ONdtRng52YvO0PUmvBgsthGbfK1MAF5D5
2GYPBgYb5RmcEY8RAWwn
=r0wC
-----END PGP SIGNATURE-----

Powered by blists - more mailing lists