[this is a resend, sorry if you received it multiple times]
Hello, The attached patch adds the GOST R 34.11-94 hash algorithm. I based this code on Alexei Kravchenko's code from librhash. The original code [0] had inline assembly for x86 and x86-64 which is removed from this version.
Although this is not a modern algorithm it is being used in the russian digital signature standard (GOST R 34.11-2001).
I had issue making the test gost94-test compile. For some reason the makefile wouldn't link it against gmp (all the other tests were ok), and couldn't figure out why.
regards, Nikos
[0]. http://hg.splayer.org/splayer/src/5027d6aa04f1/Thirdparty/librhash/librhash/
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
The attached patch adds the GOST R 34.11-94 hash algorithm. I based this code on Alexei Kravchenko's code from librhash. The original code [0] had inline assembly for x86 and x86-64 which is removed from this version.
Thanks! I'll be a bit busy with other things in the weekend. I hope I get time to look into it within a week.
Regards, /Niels
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
The attached patch adds the GOST R 34.11-94 hash algorithm. I based this code on Alexei Kravchenko's code from librhash.
Some comments:
- Implementation written by Alexei Kravchenko.
- Ported to nettle by Nikos Mavrogiannopoulos.
- Copyleft:
- I hereby release this code into the public domain. This applies worldwide.
- I grant any entity the right to use this work for ANY PURPOSE,
- without any conditions, unless such conditions are required by law.
- */
The public domain notice, that is Alexei's, right? Do you intend your modified version to also be in the public domain? (If we end up doing any major changes to the file, I'd prefer to license the Nettle version as LGPL, but if changes are minor, keeping it public domain is fine with me).
The rhash license appear to be somewhat different (see http://rhash.anz.ru/license.php), and the sourceforge page says it's using the MIT license (see http://sourceforge.net/projects/rhash/).
+/**
- Calculate a lookup table from S-Boxes.
- A substitution table is used to speed up hash calculation.
- @param out pointer to the lookup table to fill
- @param src pointer to eight S-Boxes to fill the table from
- */
+static void +fill_gost94_sbox (uint32_t out[4][256], const uint8_t src[8][16])
Maybe this should be moved to a separate file gost-data, to generate the tables during the build process?
+void +gost94_init2 (gost94_ctx * ctx, unsigned int flag) +{
- memset (ctx, 0, sizeof (gost94_ctx));
- ctx->flag = flag;
- if (ctx->flag & GOST_FLAG_CRYPTOPRO)
ctx->sbox = (uint32_t*)gost94_sbox_cryptpro;- else
ctx->sbox = (uint32_t*)gost94_sbox;+}
Can you explain briefly what this gost94_sbox and gost94_sbox_cryptpro is about? I don't quite like having to put an sbox pointer into the context struct, but maybe there's a good reason? Or maybe it's better to treat them as two distinct hash functions (one could still share internal functions like gost_block_compress, and pass appropriate sboxes as an argument).
--- /dev/null +++ b/gost94.h @@ -0,0 +1,55 @@ +/* md5.h
That shouldn't say "md5" ;-)
+#undef GENERATE_GOST_LOOKUP_TABLE
I don't think GENERATE_GOST_LOOKUP_TABLE belongs in the header file.
+#define GOST94_DATA_SIZE 32 +#define GOST94_DIGEST_SIZE 32
+/* if set it enables the CryptoPro parameter set */ +#define GOST_FLAG_CRYPTOPRO 1
And neither does this, I think. If "CryptoPro" should be supported, that should be unconditional (but with any large tables in a separate file).
+/* algorithm context */ +typedef struct gost94_ctx +{
- uint32_t hash[8]; /* algorithm 256-bit state */
- uint32_t sum[8]; /* sum of processed message blocks */
- uint8_t message[GOST94_DATA_SIZE]; /* 256-bit buffer for leftovers */
- uint64_t length; /* number of processed bytes */
- uint32_t *sbox;
- unsigned flag; /* flag, type of sbox to use */
The flag is set but unused, as far as I see.
Regards, /Niels
On 10/03/2012 10:27 PM, Niels Möller wrote:
- Copyleft:
- I hereby release this code into the public domain. This applies worldwide.
- I grant any entity the right to use this work for ANY PURPOSE,
- without any conditions, unless such conditions are required by law.
- */
The public domain notice, that is Alexei's, right?
Yes.
Do you intend your modified version to also be in the public domain? (If we end up doing any major changes to the file, I'd prefer to license the Nettle version as LGPL, but if changes are minor, keeping it public domain is fine with me).
I claim no copyright in my changes (they are minor), so the original license applies. There is no problem with me to change it to LGPL though.
The rhash license appear to be somewhat different (see http://rhash.anz.ru/license.php), and the sourceforge page says it's using the MIT license (see http://sourceforge.net/projects/rhash/).
Do you want me to contact him for clarification? However, I think that the file's license overrides the library license.
+/**
- Calculate a lookup table from S-Boxes.
- A substitution table is used to speed up hash calculation.
- @param out pointer to the lookup table to fill
- @param src pointer to eight S-Boxes to fill the table from
- */
+static void +fill_gost94_sbox (uint32_t out[4][256], const uint8_t src[8][16])
Maybe this should be moved to a separate file gost-data, to generate the tables during the build process?
Is it really needed? They are already in gosthash94-tables.c. Is there any advantage in generating them during build time? I was thinking to even remove the fill_gost94_sbox() function. The sboxes are comparable in size to the blowfish sboxes. What do you think?
+void +gost94_init2 (gost94_ctx * ctx, unsigned int flag) +{
- memset (ctx, 0, sizeof (gost94_ctx));
- ctx->flag = flag;
- if (ctx->flag & GOST_FLAG_CRYPTOPRO)
ctx->sbox = (uint32_t*)gost94_sbox_cryptpro;- else
ctx->sbox = (uint32_t*)gost94_sbox;+}
Can you explain briefly what this gost94_sbox and gost94_sbox_cryptpro is about?
I can only speculate on that because I'm not familiar with the standard. However the gosthash is based on the gost encryption algorithm which had no fixed sboxes. You could have different application areas of the algorithm that used different sboxes.
For my use-case the original sboxes are ok. I just kept the additional ones. I could remove them.
I don't quite like having to put an sbox pointer into the context struct, but maybe there's a good reason?
If the sboxes are like now in a constant array there are no issues, with memcpy of contexts (if this is your concern).
treat them as two distinct hash functions (one could still share internal functions like gost_block_compress, and pass appropriate sboxes as an argument).
That would deviate from the format of the other hash functions in nettle.
+#undef GENERATE_GOST_LOOKUP_TABLE
I don't think GENERATE_GOST_LOOKUP_TABLE belongs in the header file.
Why would it be better? The header uses this definition.
btw. I've renamed the algorithm to gosthash94, to allow for a future gost encryption algorithm.
+/* algorithm context */ +typedef struct gost94_ctx +{
- uint32_t hash[8]; /* algorithm 256-bit state */
- uint32_t sum[8]; /* sum of processed message blocks */
- uint8_t message[GOST94_DATA_SIZE]; /* 256-bit buffer for leftovers */
- uint64_t length; /* number of processed bytes */
- uint32_t *sbox;
- unsigned flag; /* flag, type of sbox to use */
The flag is set but unused, as far as I see.
Removed.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
Do you want me to contact him for clarification? However, I think that the file's license overrides the library license.
It would be nice if the licensing were crystal clear, and I'd like to have a correct description also in the copyright section of the manual.
On 10/03/2012 10:27 PM, Niels Möller wrote:
+static void +fill_gost94_sbox (uint32_t out[4][256], const uint8_t src[8][16])
Maybe this should be moved to a separate file gost-data, to generate the tables during the build process?
Is it really needed?
It's not absolutely necessary, but I think it's nice to include code for generating various magic tables, like in aesdata.c, shadata.c, gcmdata.c, etc. Even if it's not done consistently for *all* such tables.
I was thinking to even remove the fill_gost94_sbox() function.
I don't think that function should be included in the built library. But if we have a gostdata tool, it obviously belongs there.
Can you explain briefly what this gost94_sbox and gost94_sbox_cryptpro is about?
I can only speculate on that because I'm not familiar with the standard. However the gosthash is based on the gost encryption algorithm which had no fixed sboxes. You could have different application areas of the algorithm that used different sboxes.
For my use-case the original sboxes are ok. I just kept the additional ones. I could remove them.
If no one else comes up with a use case for the "cryptpro" sboxes, I think we can leave them out for now.
treat them as two distinct hash functions (one could still share internal functions like gost_block_compress, and pass appropriate sboxes as an argument).
That would deviate from the format of the other hash functions in nettle.
I was thinking that the adveertised interface would be gost_hash_* and gost_hash_cryptpro_*, each following the same conventions as the other hash functions. The shared gost_block_compress, with an sbox argument, would either be purely internal, or a gost-specific piece of the advertised interface.
+#undef GENERATE_GOST_LOOKUP_TABLE
I don't think GENERATE_GOST_LOOKUP_TABLE belongs in the header file.
Why would it be better?
It seemed like an implementation detail to me (maybe that's not how it was intended?). If it is needed at all, I think it belongs in config.h or possibly nettle-internal.h or gost-internal.h.
The header uses this definition.
Only to make the prototype for gost_init_table conditional. But that function can be declared unconditionally (if we want it at all).
btw. I've renamed the algorithm to gosthash94, to allow for a future gost encryption algorithm.
I guess the "94" is the year. So a gost94 encryption algorithm is already defained, from the same time?
I should probably read RFC 5831 and any other relevant reference material before asking more questions.
Regards, /Niels
On 10/04/2012 09:46 PM, Niels Möller wrote:
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
Do you want me to contact him for clarification? However, I think that the file's license overrides the library license.
It would be nice if the licensing were crystal clear, and I'd like to have a correct description also in the copyright section of the manual.
Attached is an updated version of the patch. I have not yet contacted the author.
regards, Nikos
On 10/05/2012 08:26 PM, Nikos Mavrogiannopoulos wrote:
It would be nice if the licensing were crystal clear, and I'd like to have a correct description also in the copyright section of the manual.
Attached is an updated version of the patch. I have not yet contacted the author.
Hello, Any update on adding gost?
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
Any update on adding gost?
I intend to add it, but I haven't yet had the time needed.
I have also intended to write to Aleksey Kravchenko and ask for an appropriate (c) notice to put in the file (or write one up and ask if he agrees to it). From the private mail you forwarded to me, it seems clear that MIT license is intended (or a subset of those requirements, but I haven't tried to figure out the difference between "rhash license" and standard MIT). So the notice should include name of author, year(s) of authorship, and MIT licensing terms. But I haven't gotten around to that either.
Sorry for the slow response. Most recent checkins try to finalize the salsa20_core changes from some month ago (I'd like to get the simple assembly salsa20_core in, and the benchmarking of it).
Regards, /Niels
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
Attached is an updated version of the patch.
+void +gosthash94_digest (gosthash94_ctx * ctx, unsigned length, uint8_t *result) +{
- unsigned index = ctx->length & 31;
- uint32_t *msg32 = (uint32_t*)ctx->message;
- assert(length <= GOSTHASH94_DIGEST_SIZE);
- /* pad the last block with zeroes and hash it */
- if (index > 0)
{memset (ctx->message + index, 0, 32 - index);gost_compute_sum_and_hash (ctx, ctx->message);}- /* hash the message length and the sum */
- msg32[0] = (uint32_t) (ctx->length << 3);
- msg32[1] = (uint32_t) (ctx->length >> 29);
- memset (msg32 + 2, 0, sizeof (uint32_t) * 6);
- gost_block_compress (ctx, msg32);
- gost_block_compress (ctx, ctx->sum);
- /* convert hash state to result bytes */
- _nettle_write_le32(length, result, ctx->hash);
+}
Any good reason for reusing the ctx->message as msg32? The cast looks dangerous, even if maybe it isn't (potentially it could have bad alignment, but not with the current struct layout). I'd replace that with a local array,
uint32_t msg32[8];
Also, the _digest function should reset the state when it's done, in the same way as _init. Which is easy, just call _init, or memset directly.
Regards, /Niels
On Sun, Nov 4, 2012 at 11:04 PM, Niels Möller nisse@lysator.liu.se wrote:
Attached is an updated version of the patch.
+void +gosthash94_digest (gosthash94_ctx * ctx, unsigned length, uint8_t *result) +{
- unsigned index = ctx->length & 31;
- uint32_t *msg32 = (uint32_t*)ctx->message;
- assert(length <= GOSTHASH94_DIGEST_SIZE);
- /* pad the last block with zeroes and hash it */
- if (index > 0)
{memset (ctx->message + index, 0, 32 - index);gost_compute_sum_and_hash (ctx, ctx->message);}- /* hash the message length and the sum */
- msg32[0] = (uint32_t) (ctx->length << 3);
- msg32[1] = (uint32_t) (ctx->length >> 29);
- memset (msg32 + 2, 0, sizeof (uint32_t) * 6);
- gost_block_compress (ctx, msg32);
- gost_block_compress (ctx, ctx->sum);
- /* convert hash state to result bytes */
- _nettle_write_le32(length, result, ctx->hash);
+}
Any good reason for reusing the ctx->message as msg32? The cast looks dangerous, even if maybe it isn't (potentially it could have bad alignment, but not with the current struct layout). I'd replace that with a local array,
uint32_t msg32[8];
Hello, I remember I noticed that also, but postponed it for after I finished the porting. It seems I forgot it then, or had some issue with the change and abandoned it. I'll add it in my todo list, but if it is an easy fix you may want to just update it.
Also, the _digest function should reset the state when it's done, in the same way as _init. Which is easy, just call _init, or memset directly.
Added to.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
I'll add it in my todo list, but if it is an easy fix you may want to just update it.
I have it fixed in my working tree now (as well as _init at the end of _digest, and fixes for the carry handling in gost_compute_sum_and_hash (which I mailed Aleksey about and I hope he will comment on that), and an MIT-style (c) notice).
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
I have it fixed in my working tree now (as well as _init at the end of _digest, and fixes for the carry handling in gost_compute_sum_and_hash (which I mailed Aleksey about and I hope he will comment on that), and an MIT-style (c) notice).
I've pushed the gost code to the repo now, with various smaller changes compared to your patch. Please have a look, and tell me if I've missed something. Some remaining things, in order of importance:
1. Documentation (manual and NEWS entry).
2. Support in nettle-benchmark.
3. I think it would be nice to include a program for generating the tables (could be based on the code in the first version of your patch). It's stil not clear to me if if/when alternative sboxes are used.
4. Maybe some reindentation and M-x whitespace-cleanup, for consistency.
BTW, with increasing number of hash functions, I think it would be nice to add some more structure to the manual, and at least split it into "recommended hash function" and "other/legacy hash functions". Not sure which category to put gost in, though.
Regards, /Niels
On 11/08/2012 10:27 PM, Niels Möller wrote:
nisse@lysator.liu.se (Niels Möller) writes:
I have it fixed in my working tree now (as well as _init at the end of _digest, and fixes for the carry handling in gost_compute_sum_and_hash (which I mailed Aleksey about and I hope he will comment on that), and an MIT-style (c) notice).
I've pushed the gost code to the repo now, with various smaller changes compared to your patch. Please have a look, and tell me if I've missed something. Some remaining things, in order of importance:
- Documentation (manual and NEWS entry).
Attached. I have issues building the nettle documentation though. I get: nettle.texinfo:744: Must be after `@deftypevr' to use `@deftypevrx' nettle.texinfo:1469: Must be after `@deftypevr' to use `@deftypevrx'
- Support in nettle-benchmark.
I just added the gosthash struct. It seems to work.
- I think it would be nice to include a program for generating the tables (could be based on the code in the first version of your patch). It's stil not clear to me if if/when alternative sboxes are used.
Not done yet. I also don't know other applications with alternative sboxes.
BTW, with increasing number of hash functions, I think it would be nice to add some more structure to the manual, and at least split it into "recommended hash function" and "other/legacy hash functions". Not sure which category to put gost in, though.
GOST should be in the legacy functions. It is considered broken (see Cryptanalysis of the GOST Hash Function, a 2008 paper), but similarly to SHA-1 attacks aren't practical yet.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
I have issues building the nettle documentation though. I get: nettle.texinfo:744: Must be after `@deftypevr' to use `@deftypevrx' nettle.texinfo:1469: Must be after `@deftypevr' to use `@deftypevrx'
That's a bug in the markup. Will fix. I wonder why I don't see those errors (or is it warnings?), maybe you are using a more recent makeinfo than I? Mine is from texinfo 4.13.
I'll get back to the patches as soon as I can.
Regards, /Niels
On 11/10/2012 02:22 PM, Niels Möller wrote:
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
I have issues building the nettle documentation though. I get: nettle.texinfo:744: Must be after `@deftypevr' to use `@deftypevrx' nettle.texinfo:1469: Must be after `@deftypevr' to use `@deftypevrx'
That's a bug in the markup. Will fix. I wonder why I don't see those errors (or is it warnings?), maybe you are using a more recent makeinfo than I? Mine is from texinfo 4.13.
They are errors in my version. I tried both the cvs version of texinfo and 4.13.90.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
[2. text/x-patch; 0001-Added-documentation-on-gosthash94.patch]...
[3. text/x-patch; 0002-Added-gosthash-to-benchmark.patch]...
[4. text/x-patch; 0003-added-news-entry.patch]...
Commited now. Thanks.
Regards, /Niels
nettle-bugs@lists.lysator.liu.se
-
Nikos Mavrogiannopoulos -
nisse＠lysator.liu.se -
Tim Ruehsen