kmaragon / Konscious.Security.Cryptography

MIT License
201 stars 20 forks source link

Constructor for HMACBlake2B doesn't initialize when specifying Key and Size #8

Closed SparkDustJoe closed 7 years ago

SparkDustJoe commented 7 years ago

HMACBlake2B b2b = new HMACBlake2B(new byte[] { 1, 0, 0, 0, 0,0,0,0 }, X);

Fails if b2b.Initialize() is not called before b2b.ComputeHash(). "Instance was not initialized. Call Initialize() first" Shouldn't that be called internally if I'm specifying the parameters? HashSize is read only after this call, and if I specify the salt/password during the constructor, I should only have to call Initialize if the salt changes? (this expected behavior is what .Net itself does)

Also throws "Hash Size must be between 8 and 512" for all values not >= 64 so long as X is byte aligned. 64 produces 8 byte output array as expected, but then why specify 8 bits as a minimum if I can't use anything < 64? [LOTS OF EDITS FOR CLARITY]

SparkDustJoe commented 7 years ago

HMACBlake2B b2b = new HMACBlake2B(i) constructor shows same behavior. Must manually call Initialize, but then still fails with Hash Size exception if < 64. [EDIT FOR CLARITY]

kmaragon commented 7 years ago

Can you point me to the spec for this? I want to make sure I'm doing it right.

The existing implementations of any HashAlgorithms don't actually call Initialize() upfront. But the big ones, the HMAC derivatives, have Initialize() methods that don't actually do anything.

for example: https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/HMACSHA512.cs

The code comments in .NET corefx say that they expect the HashAlgorithm to call Initialize() and HashFinal(). But obviously, the HashAlgorithm calls Initialize() after HashFinal().

But at the same time, HashFinal() is protected. And Initialize is public.

It seems like neither the design nor the API docs clearly document the intended design. The only such documentation I have found is in the O'Reilly book .NET security that says the constructor should call it in one particular Adler32 example... in VB no less. :-(

I could just as well replace the throw new InvalidOperationException(...) with a Initialize() with zero extra effort or code. I just wanted to make sure the class properly conformed to a HashAlgorithm implementation. And I'm still not totally sure what that means.

SparkDustJoe commented 7 years ago

Examine the following:

            HMACBlake2B blake = new HMACBlake2B(new UTF8Encoding().GetBytes("SECRET"), 512);
            blake.Initialize();
            byte[] result = blake.ComputeHash(new UTF8Encoding().GetBytes("MESSAGE"));

            HMACSHA512 sha = new HMACSHA512(new UTF8Encoding().GetBytes("SECRET"));
            result = sha.ComputeHash(new UTF8Encoding().GetBytes("MESSAGE"));

I think that the default implementations, when they compute a hash, they remove all internal memory except for the result from the last hashing operation. But they don't require a call to Initialize() when using them with a constructor that defines a secret (SHA1 and 2 don't allow for variable output lengths, you just truncate, whereas Blake, Skein, SHA3, and others will actually produce different results with different resulting bit lengths).

In your linked example, the internal property for setting the Salt does internal setup when you call it. I am unable however to see any other internal code in that GitHub for the core processes, so I can't tell what HashCore and HashFinal are really doing under the hood. [EDITED FOR FORMATTING]

kmaragon commented 7 years ago

I updated this to automatically initialize rather than throw an exception if it wasn't initialized... which obviously required updating the test that asserted that it through an exception saying that no one called Initialize()

SparkDustJoe commented 7 years ago

Looks like this is fixed as well 👍 And it now accepts bit outputs between 8 and 512 (inclusive) and throws on non-byte-aligned values (as it should)