On Mon, Jan 15, 2018 at 9:37 PM Niels Möller nisse@lysator.liu.se wrote:
Nikos Mavrogiannopoulos nmav@redhat.com writes:
Re-sending as it seems I forgot to remove cmac-internal from makefile.
I've had a first reading, and a few comments.
Thank you for the review. I reply inline.
diff --git a/cmac.c b/cmac.c new file mode 100644 index 00000000..b4886808 --- /dev/null +++ b/cmac.c
+static const uint8_t const_Rb[] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x87+};
+static inline void block_lshift(const union nettle_block16 *in, + union nettle_block16 *out) +{
uint64_t b1 = READ_UINT64(in->b);uint64_t b2 = READ_UINT64(in->b+8);unsigned overflow = b2 & 0x8000000000000000;b1 <<= 1;b2 <<= 1;if (overflow)b1 |= 0x01;I'd write the shift as
b1 = (b1 << 1) | (b2 >> 63); b2 <<= 1;
I have bad experience with shifts over 31. I could switch it to that, but my understanding is that they don't universally work especially in 32-bit systems offerring 64-bit quantities. Given that this code is being used unconditionally and is not performance critical, I'd stay with the safe version, unless you have strong opinion about it.
I think it could return the bit shifted out (since the next thing done is examining that bit). Or maybe even better, move the
if (one-bit shifted out) { memxor const_Rb }
into this function, which would then correspond to multiplication by in Z_2[x] / (Rb polynomial). Nettle conventions would also put the destination argument first. I'd suggest rewriting the function as
Nice suggestion. Done.
+void cmac128_set_key(struct cmac128 *ctx, void *key,
nettle_cipher_func *encrypt)+{
memset(ctx, 0, sizeof(*ctx));/* step 1 - generate subkeys k1 and k2 */encrypt(key, 16, ctx->L.b, const_Zero);block_lshift(ctx->L.b, ctx->K1.b);if (ctx->L.b[0] & 0x80) {memxor(ctx->K1.b, const_Rb, 16);}Is ctx->L used for anything after key setup? If not, it should be moved to a local variable, out of the context struct.
No. The reason it was used like than in samba is was so that when overwritting the state, any intermediate data of the operation were erased. That's not a bad idea, though, we don't need an additional variable as 'block' is available for temp use at this stage. I've modified it to use that.
block_lshift(ctx->K1.b, ctx->K2.b);if (ctx->K1.b[0] & 0x80) {memxor(ctx->K2.b, const_Rb, 16);}+}
+#define MIN(x,y) ((x)<(y)?(x):(y))
+void cmac128_update(struct cmac128 *ctx, void *key,
nettle_cipher_func *encrypt,size_t msg_len, const uint8_t *msg)+{
/** check if we expand the block*/if (ctx->last_len < 16) {size_t len = MIN(16 - ctx->last_len, msg_len);memcpy(&ctx->last.b[ctx->last_len], msg, len);msg += len;msg_len -= len;ctx->last_len += len;}Other similar code uses different naming convenstions, in particular ctx->block rather than ctx->last, and ctx->index rather than ctx->last_len. Not terribly important, but it is nice to have some consistency in style.
Done.
if (msg_len == 0) {/* if it is still the last block, we are done */return;}/** now checksum everything but the last block*/memxor3(ctx->Y.b, ctx->X.b, ctx->last.b, 16);encrypt(key, 16, ctx->X.b, ctx->Y.b);while (msg_len > 16) {memxor3(ctx->Y.b, ctx->X.b, msg, 16);encrypt(key, 16, ctx->X.b, ctx->Y.b);msg += 16;msg_len -= 16;}/** copy the last block, it will be processed in* cmac128_digest().*/memset(ctx->last.b, 0, sizeof(ctx->last.b));I'd prefer to postpone the memset/zero-padding until cmac128_digest.
Done.
memcpy(ctx->last.b, msg, msg_len);ctx->last_len = msg_len;+}
+void cmac128_digest(struct cmac128 *ctx, void *key,
nettle_cipher_func *encrypt,unsigned length,uint8_t *out)+{
uint8_t tmp[16];Is both last_len == 0 and last_len == 16 possible here? If practical, it would be nice to arrange so that the update function above processes all full blocks.
if (ctx->last_len < 16) {ctx->last.b[ctx->last_len] = 0x80;memxor3(tmp, ctx->last.b, ctx->K2.b, 16);} else {memxor3(tmp, ctx->last.b, ctx->K1.b, 16);}If that works out, this conditional would be replaced by
if (ctx->last_len > 0) { /* Pad last block, xor with K2 rather than K1 */ assert (ctx->last_len < 16); ctx->last.b[ctx->last_len] = 0x80; memset(last.b + ctx->last_len + 1, 0, 15 - ctx->last_len); memxor3(tmp, ctx->last.b, ctx->K2.b, 16) } /* Note: No else clause */
Other code reuses the block buffer for temporary values in the digest function. Maybe that would work here too, with memxor rather than memxor3. Since memxor is a byte-by-byte operation, one could also consider eliminating the memset, and instead do memxor + memcpy, but not sure if that's an improvement in perforamance or readability.
memxor3(ctx->Y.b, tmp, ctx->X.b, 16);assert(length <= 16);if (length == 16) {encrypt(key, length, out, ctx->Y.b);} else {encrypt(key, length, tmp, ctx->Y.b);memcpy(out, tmp, length);}Maybe I'm missing something, but to me both clauses look equivalent?
They are updated to: if (length == 16) { encrypt(key, 16, out, ctx->Y.b); } else { encrypt(key, 16, tmp, ctx->Y.b); memcpy(out, tmp, length); }
The difference is that tmp is used to store the output of encrypt() in the second clause.
diff --git a/cmac.h b/cmac.h new file mode 100644 index 00000000..5f0553b7 --- /dev/null +++ b/cmac.h @@ -0,0 +1,117 @@ +struct cmac128 {
union nettle_block16 K1;union nettle_block16 K2;union nettle_block16 L;union nettle_block16 X;union nettle_block16 Y;union nettle_block16 last;size_t last_len;+};
To me, it looks like L and Y don't belong in the context struct. As I understand it, K1 and K2 are the mmac key, X is the state, and last is the buffer needed to handle one block at a time.
Updated.
I've also added some documentation and enhanced the cmac test to check operation on re-use of structure, as well as on byte-to-byte encryption.