Add BLAKE2[sb] hash #191

ksherlock · 2017-04-08T03:54:26Z

Hi,

This adds the blake2s hash (https://blake2.net, https://www.ietf.org/rfc/rfc7693.txt) based on the reference implementation (https://github.com/BLAKE2/BLAKE2/blob/master/ref/blake2s-ref.c)

karel-m · 2017-04-08T15:32:56Z

Looks good, do you plan to add blake2b as well?

ksherlock · 2017-04-08T19:45:45Z

I have added blake2b as well.

karel-m · 2017-04-09T20:29:06Z

Ad Travis-CI failures e.g. https://travis-ci.org/libtom/libtomcrypt/jobs/220116643

you have to move all declaration at the block beginning like this:

 static inline int blake2s_init0(hash_state *md)
 {
+   int i;
    XMEMSET(&md->blake2s, 0, sizeof(struct blake2s_state));
 
-   for (int i = 0; i < 8; ++i)
+   for (i = 0; i < 8; ++i)

ksherlock · 2017-04-09T21:24:54Z

It has been updated.
Thanks,
Kelvin

karel-m · 2017-04-10T05:01:42Z

One more place that needs a fix

src/hashes/blake2s.c: In function ‘blake2s_init0’:
src/hashes/blake2s.c:153:4: error: ‘for’ loop initial declarations are only allowed in C99 mode
src/hashes/blake2s.c:153:4: note: use option -std=c99 or -std=gnu99 to compile your code

karel-m · 2017-04-08T15:23:49Z

src/hashes/blake2s.c

+{
+   XMEMSET(&md->blake2s, 0, sizeof(struct blake2s_state));
+
+   for (int i = 0; i < 8; ++i)


move declaration int i at the block beginning

karel-m · 2017-04-08T15:24:57Z

src/hashes/blake2s.c

+static int blake2s_init_param(hash_state *md, const struct blake2s_param *P)
+{
+   blake2s_init0(md);
+   ulong32 *p = (ulong32 *)(P);


declaration ulong32 *p should go at the block beginning before any code

karel-m · 2017-04-08T15:25:27Z

src/hashes/blake2s.c

+   ulong32 *p = (ulong32 *)(P);
+
+   /* IV XOR ParamBlock */
+   for (size_t i = 0; i < 8; ++i) {


move declaration size_t i at the block beginning

karel-m · 2017-04-08T15:26:08Z

src/hashes/blake2s.c

+      LOAD32L(m[i], buf + i * sizeof(m[i]));
+   }
+
+   for (size_t i = 0; i < 8; ++i)


remove the declaration size_t i

karel-m · 2017-04-08T15:26:28Z

src/hashes/blake2s.c

+   ulong32 m[16];
+   ulong32 v[16];
+
+   for (size_t i = 0; i < 16; ++i) {


move declaration size_t i at the block beginning

karel-m · 2017-04-08T15:27:49Z

src/hashes/blake2s.c

+   ROUND(8);
+   ROUND(9);
+
+   for (size_t i = 0; i < 8; ++i)


remove the declaration size_t i

karel-m · 2017-04-10T09:49:32Z

src/hashes/blake2s.c

+   /* IV XOR ParamBlock */
+   for (i = 0; i < 8; ++i) {
+      ulong32 tmp;
+      LOAD32L(tmp, &p[i]);


I propose a change like this:

static int blake2s_init_param(hash_state *md, const struct blake2s_param *P) { unsigned long i; - ulong32 *p = (ulong32 *)(P); + unsigned char *p = (unsigned char *)(P); blake2s_init0(md); /* IV XOR ParamBlock */ for (i = 0; i < 8; ++i) { ulong32 tmp; - LOAD32L(tmp, &p[i]); + LOAD32L(tmp, p + i*4); md->blake2s.h[i] ^= tmp; }

c.f. my comments to blake2b_init_param()

karel-m · 2017-04-10T09:54:17Z

src/hashes/blake2s.c

+      STORE32L(md->blake2s.h[i], buffer + sizeof(md->blake2s.h[i]) * i);
+
+   XMEMCPY(out, buffer, md->blake2s.outlen);
+#ifdef LTC_CLEAN_STACK


md is not on stack

& you forgot to clean buffer

karel-m · 2017-04-10T09:55:09Z

src/hashes/blake2b.c

+   XMEMCPY(out, buffer, md->blake2b.outlen);
+#ifdef LTC_CLEAN_STACK
+   zeromem(buffer, sizeof(buffer));
+   zeromem(md, sizeof(hash_state));


md is not on stack

IMO zeromem(md, sizeof(hash_state)) should be out of #ifdef LTC_CLEAN_STACK ... #endif

IMO zeromem(md, sizeof(hash_state)) should be out of #ifdef LTC_CLEAN_STACK ... #endif

true

sjaeckel

Regarding my comments to the struct blake2[bs]_param I also don't have a good solution yet...

sjaeckel · 2017-04-10T12:13:27Z

src/headers/tomcrypt_hash.h

+    ulong32 h[8];
+    ulong32 t[2];
+    ulong32 f[2];
+    unsigned char buf[2 * 64];


why 2*64 here and 128 for blake2b?

sjaeckel · 2017-04-10T12:16:03Z

src/headers/tomcrypt_hash.h

+    ulong32 t[2];
+    ulong32 f[2];
+    unsigned char buf[2 * 64];
+    ulong32 curlen;


also these types (curlen, outlen, last_node) could be the same type as in blake2b or not? (or in blake2b the same as here, whichever makes most sense, at least it should be consistent)

sjaeckel · 2017-04-10T12:22:07Z

src/hashes/blake2b.c

+   /* IV XOR ParamBlock */
+   for (i = 0; i < 8; ++i) {
+      ulong64 tmp;
+      LOAD64L(tmp, p + sizeof(md->blake2b.h[i]) * i);


ugh @ the typecast before and then p + sizeof(md->blake2b.h[i]) * i

especially since the blake2b_param struct isn't packed... and I'm not really a fan of packed structs regarding portability...

sjaeckel · 2017-04-10T12:29:46Z

src/hashes/blake2b.c

+}
+
+/* init xors IV with input parameter block */
+int blake2b_init_param(hash_state *md, const struct blake2b_param *P)


this can be static

sjaeckel · 2017-04-10T12:38:55Z

src/hashes/blake2s.c

+   /* IV XOR ParamBlock */
+   for (i = 0; i < 8; ++i) {
+      ulong32 tmp;
+      LOAD32L(tmp, &p[i]);


c.f. my comments to blake2b_init_param()

sjaeckel · 2017-04-10T12:42:08Z

src/hashes/blake2s.c

+      STORE32L(md->blake2s.h[i], buffer + sizeof(md->blake2s.h[i]) * i);
+
+   XMEMCPY(out, buffer, md->blake2s.outlen);
+#ifdef LTC_CLEAN_STACK


& you forgot to clean buffer

sjaeckel · 2017-04-10T12:55:23Z

src/hashes/blake2s.c

+{
+   int err;
+   err = _blake2s_compress(md, buf);
+   burn_stack(sizeof(ulong32) * 32);


you forgot i :)

…fer.

sjaeckel · 2017-04-10T14:51:35Z

src/hashes/blake2b.c

   return CRYPT_OK;
 }

 int blake2b_init(hash_state *md, unsigned long outlen)
 {
-   struct blake2b_param P;
+   unsigned char P[BLAKE2B_PARAM_SIZE];


sjaeckel · 2017-04-10T14:55:53Z

src/hashes/blake2s.c

 }

 /* Some helper functions, not necessarily useful */
+static int blake2s_is_lastblock(const hash_state *md) { return md->blake2s.f[0] != 0; }


sjaeckel · 2017-04-10T14:56:11Z

src/hashes/blake2b.c

+static void blake2b_set_lastnode(hash_state *md)
+{
+   md->blake2b.f[1] = CONST64(0xffffffffffffffff);
+}

 /* Some helper functions, not necessarily useful */
 static int blake2b_is_lastblock(const hash_state *md) { return md->blake2b.f[0] != 0; }


sjaeckel · 2017-04-10T15:06:06Z

src/hashes/blake2s.c


   XMEMCPY(out, buffer, md->blake2s.outlen);
 #ifdef LTC_CLEAN_STACK
-    zeromem(md, sizeof(hash_state));
+   zeromem(buffer, sizeof(buffer));
+   zeromem(md, sizeof(hash_state));


~~shouldn't we better remove the zero'ing of md for both _done functions? or is the hash descriptor unusable anyways after having called done on it?~~ I just realized that's in line with all the other hash implementations...

sjaeckel · 2017-04-10T22:06:39Z

I was thinking about changing blake2[sb]_init as we use it as far as I see only in blake2[sb].c files. We will obviously need to change related parts like:
- int blake2b_160_init(hash_state *md) { return blake2b_init(md, 20); }
+ int blake2b_160_init(hash_state *md) { return blake2b_init(md, 20, NULL, 0); }
But I am not strongly against separate blake2[sb]_init_key functions. Before you start perhaps wait for @sjaeckel's point of view.

sounds fine by me, it doesn't look like a blake2[sb]_init_key is needed as blake2[sb]_init would be yet another wrapper.

The next step would be creating something like src/mac/blake2[sb]mac/blake2[sb]mac.c with:

int blake2smac_init(blake2smac_state *st, unsigned long outlen, const unsigned char *key, unsigned long keylen);
int blake2smac_process(blake2smac_state *st, const unsigned char *in, unsigned long inlen);
int blake2smac_done(blake2smac_state *st, unsigned char *mac, unsigned long *maclen);

And to make it complete we need:

src/mac/blake2[sb]mac/blake2[sb]mac_file.c 
src/mac/blake2[sb]mac/blake2[sb]mac_memory.c 
src/mac/blake2[sb]mac/blake2[sb]mac_memory_multi.c 
src/mac/blake2[sb]mac/blake2[sb]mac_test.c

also sounds fine by me, I'd just put them in src/mac/blake2/blake2[sb]mac_[file,memory,...].c

please have a look at #184 regarding the structure & API signatures used.

karel-m · 2017-04-11T05:33:44Z

Yes, having both BLAKE2 based MACs in src/mac/blake2/*.c looks better.

sjaeckel · 2017-04-18T12:42:12Z

src/hashes/blake2b.c

+
+const struct ltc_hash_descriptor blake2b_160_desc =
+{
+    "blake2b_160",


can you please rename the identifiers for all blake2[bs] hash descriptors to blake2[bs]-XXX etc.

sjaeckel · 2017-04-18T12:57:49Z

please also add this patch and then we're ready to merge

diff --git a/demos/hashsum.c b/demos/hashsum.c
index 8f94af5..9951a85 100644
--- a/demos/hashsum.c
+++ b/demos/hashsum.c
@@ -105,6 +105,18 @@ void register_algs(void)
 #ifdef LTC_WHIRLPOOL
   register_hash (&whirlpool_desc);
 #endif
+#ifdef LTC_BLAKE2S
+  register_hash (&blake2s_128_desc);
+  register_hash (&blake2s_160_desc);
+  register_hash (&blake2s_224_desc);
+  register_hash (&blake2s_256_desc);
+#endif
+#ifdef LTC_BLAKE2B
+  register_hash (&blake2b_160_desc);
+  register_hash (&blake2b_256_desc);
+  register_hash (&blake2b_384_desc);
+  register_hash (&blake2b_512_desc);
+#endif
 #ifdef LTC_CHC_HASH
   register_hash(&chc_desc);
   if ((err = chc_register(register_cipher(&aes_enc_desc))) != CRYPT_OK) {
diff --git a/src/hashes/blake2b.c b/src/hashes/blake2b.c
index 3e7dd10..55409cc 100644
--- a/src/hashes/blake2b.c
+++ b/src/hashes/blake2b.c
@@ -489,6 +489,16 @@ int blake2b_256_test(void)
         0x31, 0x71, 0xef, 0x3f, 0xee, 0x98, 0x57, 0x9b,
         0x94, 0x96, 0x4e, 0x3b, 0xb1, 0xcb, 0x3e, 0x42,
         0x72, 0x62, 0xc8, 0xc0, 0x68, 0xd5, 0x23, 0x19 } },
+    { "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890",
+      { 0x0f, 0x6e, 0x01, 0x8d, 0x38, 0xd6, 0x3f, 0x08,
+        0x4d, 0x58, 0xe3, 0x0c, 0x90, 0xfb, 0xa2, 0x41,
+        0x5f, 0xca, 0x17, 0xfa, 0x66, 0x26, 0x49, 0xf3,
+        0x8a, 0x30, 0x41, 0x7c, 0x57, 0xcd, 0xa8, 0x14 } },
 
     { NULL, { 0 } }
   };
diff --git a/src/hashes/blake2s.c b/src/hashes/blake2s.c
index 1c4164e..dbb1414 100644
--- a/src/hashes/blake2s.c
+++ b/src/hashes/blake2s.c
@@ -382,6 +382,16 @@ int blake2s_256_test(void)
         0xe1, 0xa7, 0x2b, 0xa3, 0x4e, 0xeb, 0x45, 0x2f,
         0x37, 0x45, 0x8b, 0x20, 0x9e, 0xd6, 0x3a, 0x29,
         0x4d, 0x99, 0x9b, 0x4c, 0x86, 0x67, 0x59, 0x82 } },
+    { "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890"
+      "12345678901234567890123456789012345678901234567890",
+      { 0xa3, 0x78, 0x8b, 0x5b, 0x59, 0xee, 0xe4, 0x41,
+        0x95, 0x23, 0x58, 0x00, 0xa4, 0xf9, 0xfa, 0x41,
+        0x86, 0x0c, 0x7b, 0x1c, 0x35, 0xa2, 0x42, 0x70,
+        0x50, 0x80, 0x79, 0x56, 0xe3, 0xbe, 0x31, 0x74 } },
 
     { NULL, { 0 } }
   };

karel-m · 2017-04-18T19:05:35Z

Ad blake2b_160 vs. blake2b-160 I prefer the underscore variant as can be seen in my recent sha3_nnn addition. But the fact is that sha512 family uses a dash. We should be definitely consistent. Before merge test vectors should be regenerated via tv_gen.

sjaeckel · 2017-04-18T19:11:07Z

I don't think it should be a personal preference, so I just use the notation that is used in the standards... therefore - and not _
:-)

karel-m · 2017-04-18T19:43:51Z

And what about zeroing md always (not only #ifdef LTC_CLEAN_STACK) as mentioned in my comment? Especially if we'll use the same routines for BLAKE2 based MACs, the MD will contain key related material.

sjaeckel · 2017-04-18T20:45:09Z

And what about zeroing md always (not only #ifdef LTC_CLEAN_STACK) as
mentioned in my comment?

true, I missed that

sjaeckel · 2017-04-18T20:47:18Z

src/hashes/blake2b.c

+   XMEMCPY(out, buffer, md->blake2b.outlen);
+#ifdef LTC_CLEAN_STACK
+   zeromem(buffer, sizeof(buffer));
+   zeromem(md, sizeof(hash_state));


IMO zeromem(md, sizeof(hash_state)) should be out of #ifdef LTC_CLEAN_STACK ... #endif

true

sjaeckel · 2017-04-18T20:47:37Z

src/hashes/blake2s.c

+   XMEMCPY(out, buffer, md->blake2s.outlen);
+#ifdef LTC_CLEAN_STACK
+   zeromem(buffer, sizeof(buffer));
+   zeromem(md, sizeof(hash_state));


sjaeckel · 2017-04-20T10:27:51Z

you need something like this

make tv_gen
./tv_gen
mv *_tv.txt notes/
git add notes/
git commit -m "update testvectors"

@karel-m should we add something like the following to helper.pl?

for f in demos/tv_gen.c demos/hashsum.c testprof/x86_prof.c ; do for h in $(rgrep ltc_hash_desc ./src/hashes/ | awk '{print $4}'); do rgrep -q $h $f || echo $h not in $f ; done ; done

...which immediately shows

rmd320_desc not in demos/tv_gen.c
sha512_224_desc not in demos/tv_gen.c
sha512_256_desc not in demos/tv_gen.c
rmd256_desc not in demos/tv_gen.c
sha3_224_desc not in demos/hashsum.c
sha3_256_desc not in demos/hashsum.c
sha3_384_desc not in demos/hashsum.c
sha3_512_desc not in demos/hashsum.c
rmd320_desc not in demos/hashsum.c
sha512_224_desc not in demos/hashsum.c
sha512_256_desc not in demos/hashsum.c
rmd256_desc not in demos/hashsum.c

sjaeckel · 2017-04-20T14:10:44Z

I don't know how you created these testvectors, but the ones you pushed fail on my machine as well 😮
What's the architecture&OS you're working on?

ksherlock · 2017-04-20T15:23:34Z

The hash_sizes in blakes2-160 and blake2s-224 were a little off. hmac_tv.txt should be a little more consistent now.

karel-m · 2017-04-20T18:27:09Z

👍 I am for merging

sjaeckel · 2017-04-20T19:04:10Z

thx @ksherlock & @karel-m for the good review process

ksherlock added 8 commits April 7, 2017 20:47

blake2s hash

68296e0

add blake2s to various places.

647b53d

clang format

51f6b06

moe clang-format

1b5ae95

sort and number the ltc_hash_descriptors.

4449cdc

more tests.

063df0b

use hash_state * for all arguments.

5107572

makefile updates.

b0e9a23

ksherlock added 4 commits April 8, 2017 15:40

tweak source to conform a little better

5924bd4

add blake2b hash

7c5fef9

clang-format

6f6e2d3

update makefiles for blake2b.

b412ab5

add LTC_ARGCHK for blake2b_process.

268c87e

remove inline and c99 variable declarations.

b4594ab

sjaeckel requested review from karel-m and sjaeckel April 10, 2017 12:11

karel-m requested changes Apr 10, 2017

View reviewed changes

sjaeckel requested changes Apr 10, 2017

View reviewed changes

sjaeckel reviewed Apr 10, 2017

View reviewed changes

src/hashes/blake2s.c Outdated

{

int err;

err = _blake2s_compress(md, buf);

burn_stack(sizeof(ulong32) * 32);

Copy link

Member

sjaeckel Apr 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot i :)

ksherlock added 4 commits April 10, 2017 10:21

minor cleanup.

afc6e8d

cleanup, resins with updated reference source

6acdfe9

replace packed parameter struct with offsets into a unsigned char buf…

6b46b44

…fer.

add missing const.

dfbdb62

sjaeckel reviewed Apr 10, 2017

View reviewed changes

add key/keylen parameters to blake2x_init

4ee9d76

sjaeckel reviewed Apr 18, 2017

View reviewed changes

sjaeckel requested changes Apr 18, 2017

View reviewed changes

sjaeckel assigned ksherlock Apr 18, 2017

ksherlock added 4 commits April 19, 2017 16:46

use - in hash descriptors.

2dc574d

always zeromem hash_state when done

c65d242

more tests.

4905232

update tv_gen.c

0109e3c

update test vectors

4f72641

fix blake2s-160/224 hash sizes.

56a2efa

updated hash_tv.txt

609d0af

sjaeckel approved these changes Apr 20, 2017

View reviewed changes

sjaeckel merged commit 513fcf3 into libtom:develop Apr 20, 2017

sjaeckel changed the title ~~blake2s hash~~ Add BLAKE2[sb] hash Apr 20, 2017

karel-m mentioned this pull request Apr 21, 2017

helper.pl --check-hashes + related fixes #194

Merged

sjaeckel modified the milestone: v1.18.0 Jun 27, 2017

sjaeckel unassigned ksherlock Oct 9, 2017

Add BLAKE2[sb] hash #191

Add BLAKE2[sb] hash #191

Conversation

ksherlock commented Apr 8, 2017

karel-m commented Apr 8, 2017

ksherlock commented Apr 8, 2017

karel-m commented Apr 9, 2017

ksherlock commented Apr 9, 2017

karel-m commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjaeckel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjaeckel Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

sjaeckel commented Apr 10, 2017

karel-m commented Apr 11, 2017

Choose a reason for hiding this comment

sjaeckel commented Apr 18, 2017

karel-m commented Apr 18, 2017 via email

sjaeckel commented Apr 18, 2017

karel-m commented Apr 18, 2017 via email

sjaeckel commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjaeckel commented Apr 20, 2017

sjaeckel commented Apr 20, 2017

ksherlock commented Apr 20, 2017

karel-m commented Apr 20, 2017 via email

sjaeckel commented Apr 20, 2017

sjaeckel Apr 10, 2017 •

edited

Loading