Re: [PATCH 1/6] Start separating GOST 28147-89 from GOST R 34.11-94
Dmitry Eremin-Solenikov writes:
> Hash function GOST R 34.11-94 (gosthash94) in its compression function
> uses Russian block cipher (GOST 28147-89, Magma). Start separating block
> cipher code from hash function code. For now there is no public
> interface for this cipher, it will be added later.
I'm having an initial look at this, with a few comments.
> --- /dev/null
> +++ b/gost28147.c
> +/*
> + * A macro that performs a full encryption round of GOST 28147-89.
> + * Temporary variables tmp assumed and variables r and l for left and right
> + * blocks.
> + */
> +#define GOST_ENCRYPT_ROUND(key1, key2, sbox) \
> + tmp = (key1) + r; \
> + l ^= (sbox)[0*256 + (tmp & 0xff)] ^ (sbox)[1*256 + ((tmp >> 8) & 0xff)] ^ \
> +(sbox)[2*256 + ((tmp >> 16) & 0xff)] ^ (sbox)[3*256 + (tmp >> 24)]; \
> + tmp = (key2) + l; \
> + r ^= (sbox)[0*256 + (tmp & 0xff)] ^ (sbox)[1*256 + ((tmp >> 8) & 0xff)] ^ \
> +(sbox)[2*256 + ((tmp >> 16) & 0xff)] ^ (sbox)[3*256 + (tmp >> 24)];
This code is just moved around in this patch, but I'd like to note that
it's preferable to always wrap function-like macros like this in do { ... }
while (0), and when used terminate with ;. And avoid using surrounding
variables; r and l could be macro arguments, and tmp (with some likely
unique prefix) could be a local in the do { ... } while block.
> --- /dev/null
> +++ b/gost28147.h
> @@ -0,0 +1,63 @@
> +struct gost28147_param
> +{
> + uint32_t sbox[4*256];
> +};
Why change to a flat array, and not keep it as
uint32_t sbox[4][256];
?
> +extern const struct gost28147_param gost28147_param_test_3411;
I find "test" in the name a bit odd. Is there a reason for that? And
declaration should probably not be in an installed header file, but in
gost-internal.h or so.
> +/* Internal interface for use by GOST R 34.11-94 */
> +void gost28147_encrypt_simple (const uint32_t *key, const uint32_t *sbox,
> + const uint32_t *in, uint32_t *out);
Same here: if internal, shouldn't be in an installed header file. And
"simple" looks a bit odd.
Should the sbox argument be of type const gost28147_param * ?
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
Re: [PATCH] cmac: add 64-bit mode CMAC
Dmitry Eremin-Solenikov writes:
> Signed-off-by: Dmitry Eremin-Solenikov
> ---
> cmac.c | 125 -
> cmac.h | 69 +++
> nettle-types.h | 6 +++
> 3 files changed, 199 insertions(+), 1 deletion(-)
>
> diff --git a/cmac.c b/cmac.c
> index 70ce8132d9d1..36ad8e58e45e 100644
> --- a/cmac.c
> +++ b/cmac.c
> @@ -1,9 +1,10 @@
> /*
> - AES-CMAC-128 (rfc 4493)
> + AES-CMAC-128 (rfc 4493) / CMAC-64
> Copyright (C) Stefan Metzmacher 2012
> Copyright (C) Jeremy Allison 2012
> Copyright (C) Michael Adam 2012
> Copyright (C) 2017, Red Hat Inc.
> + Copyright (C) 2019, Dmitry Eremin-Solenikov
>
> This file is part of GNU Nettle.
>
> @@ -57,6 +58,15 @@ _cmac128_block_mulx(union nettle_block16 *dst,
>dst->u64[0] = (src->u64[0] << 1) | (src->u64[1] >> 63);
>dst->u64[1] = (src->u64[1] << 1) ^ (0x87 & -carry);
> }
> +
> +static void
> +block_mulx8(union nettle_block8 *dst,
> + const union nettle_block8 *src)
> +{
> + uint64_t carry = src->u64 >> 63;
> +
> + dst->u64 = (src->u64 << 1) ^ (0x1b & -carry);
> +}
> #else /* !WORDS_BIGENDIAN */
> #define LE_SHIFT(x) x) & 0x7f7f7f7f7f7f7f7f) << 1) | \
> (((x) & 0x8080808080808080) >> 15))
> @@ -68,6 +78,15 @@ _cmac128_block_mulx(union nettle_block16 *dst,
>dst->u64[0] = LE_SHIFT(src->u64[0]) | ((src->u64[1] & 0x80) << 49);
>dst->u64[1] = LE_SHIFT(src->u64[1]) ^ (0x8700 & -carry);
> }
Patch looks nice, thanks! Is any of the implementation shared with
cmac128? I think it would be nice to move it to a separate source file
cmac64.c. Sharing the cmac.h header file is fine.
BTW, I'm sorry for the duplicated effort on nettle_block16 w; I'm
traveling and online only sporadically, so I gave it a try without being
up to date with your work.
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
Re: [PATCH 2/3] gcm: drop w field from nettle_block16
Dmitry Eremin-Solenikov writes: > "unsigned long w" comes from the time when Nettle didn't use uint64_t. > It is unused now and thus can be dropped. I've done something very similar on the block16-refactor branch. > + r->u64[0] = (x->u64[0] >> 1) ^ (mask & (GHASH_POLYNOMIAL << 56)); I've found this needs to be (uint64_t) GHASH_POLYNOMIAL << 56. Otherwise tests fail when I cross compile for (32-bit) mips and run under qemu. I'm also trying to move helper functions (most or all should be inline) to block16-internal.h. Next, I'm looking into unifying the various shift operations. It seems we have the following variants: Big-endian left shift: cmac, eax, polynomial 0x87 Little-endian left shift: xts, polynomial 0x87 Big-endian right shift: gcm, polynomial 0xE1 (bit-reverse of 0x87) If I understand it correctly after a quick look (long since I looked at GCM in detail), its represents the polynomials with a peculiar bit-order where what's otherwise the least significant bit represents the coefficient of the highest power of x. The multiplication is kind-of invariant under bit-reversal, but I'm not sure if it's possible to rearrange it to use a different bit order without explicit bit reversal of the input. At least, not an easy change. I'm thinking of some shared macros or inline functions to abstract the left shift operations, say block16_mulx_be, block16_mulx_le. Regards, /Niels -- Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677. Internet email is subject to wholesale government surveillance. ___ nettle-bugs mailing list nettle-bugs@lists.lysator.liu.se http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
