Nikos Mavrogiannopoulos <n...@redhat.com> writes:
> This patch adds the SIV-CMAC algorithm to nettle (an update of the
> previous attempt). It is an atypical cipher which fits into the
> encrypt_message interface.
Thanks. Some comments below:
> --- a/nettle-types.h
> +++ b/nettle-types.h
> @@ -78,6 +78,21 @@ typedef void *nettle_realloc_func(void *ctx, void *p,
> size_t length);
> /* Ciphers */
> typedef void nettle_set_key_func(void *ctx, const uint8_t *key);
>
> +/* AEAD ciphers */
> +typedef void
> +nettle_encrypt_message(void *ctx,
> + size_t nlength, const uint8_t *nonce,
> + size_t alength, const uint8_t *adata,
> + size_t tlength,
> + size_t clength, uint8_t *dst, const uint8_t *src);
> +
> +typedef int
> +nettle_decrypt_message(void *ctx,
> + size_t nlength, const uint8_t *nonce,
> + size_t alength, const uint8_t *adata,
> + size_t tlength,
> + size_t mlength, uint8_t *dst, const uint8_t *src);
> +
In this patch, these types used in tests only. But I imagine you'd like
something in nettle-meta to represent a message-oriented aead?
> +@acronym{SIV-CMAC} mode is a combination of counter mode with message
> +authentication based on @acronym{CMAC}. Unlike other counter @acronym{AEAD}
> +modes, it provides protection against accidental nonce misuse, making it
> +a good choice for stateless-servers that cannot ensure nonce
> uniqueness.
Some detail on the nonce-reuse would be helpful. As I understood RFC
5297, the nonce used in SIV is only to make the ciphertexts of otherwise
identical messages look different to the attacker.
> --- /dev/null
> +++ b/siv-aes128-cmac.c
> @@ -0,0 +1,79 @@
[...]
> +void
> +siv_aes128_cmac_encrypt_message(struct siv_aes128_cmac_ctx *ctx,
> + size_t nlength, const uint8_t *nonce,
> + size_t alength, const uint8_t *adata,
> + size_t tlength,
> + size_t slength, uint8_t *dst, const uint8_t
> *src)
> +{
> + assert(tlength == SIV_DIGEST_SIZE);
The tlength argument doesn't look that useful, do we need it?. Also, I
think we agreed that the message length argument should be the size of
the *destination* area, i.e., ciphertext size for encrypt_message (see
ccm_*_encrypt_message):
> --- /dev/null
> +++ b/siv-cmac.c
> @@ -0,0 +1,194 @@
> +/* This is a common structure for AES-128 and AES-256 thus
> + * for the cipher part we simply pad to the maximum structure
> + * size plus 16 bytes to account for any alignment difference in
> + * the original structures */
> +struct cmac128_syn {
> + struct cmac128_ctx ctx;
> + struct {
> + uint8_t pad[NETTLE_MAX_CIPHER16_CONTEXT_SIZE+16];
> + } cipher;
> +};
I don't think this guarantees any alignment. You would either need
soemthing like
struct cipher_storage {
union {
uint64_t u64;
char c[NETTLE_MAX_CIPHER16_CONTEXT_SIZE];
} storage;
};
...
CMAC128_CTX(cipher_storage) ctx;
or use TMP_ALLOC_ALIGN (but in the latter case, you can't use the
CMAC128_* macros). Or let caller pass in the cipher context (see last
comment in this mail)
> +static
> +void _siv_s2v(const struct nettle_cipher *nc,
> + const uint8_t *s2vk, size_t alength, const uint8_t *adata,
> + size_t nlength, const uint8_t *nonce,
> + size_t plength, const uint8_t *pdata,
> + uint8_t *v)
> +{
> + struct cmac128_syn ctx;
> + union nettle_block16 D, S, T;
> + const uint8_t const_one[16] = {
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01
> + };
> + const uint8_t const_zero[16] = {
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> + };
Use static const.
> + assert(nc->context_size <= NETTLE_MAX_CIPHER16_CONTEXT_SIZE);
> +
> + /* ensure we have enough size of context plus any padding size */
> + CMAC128_SET_KEY(&ctx, nc->set_encrypt_key, nc->encrypt, s2vk);
> +
> + if (nlength == 0 && alength == 0) {
> + CMAC128_UPDATE(&ctx, nc->encrypt, 16, const_one);
> + CMAC128_DIGEST(&ctx, nc->encrypt, 16, v);
> + return;
> + }
Shouldn't the plaintext, plength, pdata, still be processed in this
case?
In this function, you treat empty associated data or nonce as those
elements missing in the input vector to S2V. E.g., if both adata and
nonce are empty, the input vector is { plaintext }, one single element.
But it could also be { "", "", plaintext }, with three elements, the
first two being empty strings.
To me, this sounds like a likely source of interop problems. Since RFC
5297 is general and allows the application to decide on the number of
elements and meaning of the input vector, it doesn't give much
guidance on this, as far as I see. The crucial case is when an
application specifies that SIV is used with associated data and/or a
nonce, but allows an empty string for either of those.
Also encoding empty adata, nonce "foo" in the same way as adata "foo",
empty nonce, seems like a subtlety contrary to the spirit of 1.3.3.
> +void
> +siv_cmac_set_key(struct siv_cmac_ctx *ctx, void *cipher,
> + const struct nettle_cipher *nc,
> + const uint8_t *key)
> +{
> + unsigned skey_size = nc->key_size;
> +
> + assert(skey_size <= SIV_MAX_KEY_SIZE/2);
> + memcpy(ctx->s2vk, key, skey_size);
I think this function should do the underlying key setup also for the
cipher instance used for s2v, not just store the key for later. So then
the function would be
void
siv_cmac_set_key(void *cmac_cipher, void *ctr_cipher,
const struct nettle_cipher *nc,
const uint8_t *key)
with no struct siv_cmac_ctx.
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
