Nikos Mavrogiannopoulos <[email protected]> writes:
> On Sun, 2019-04-14 at 09:33 +0200, Niels Möller wrote:
>> > +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?
>
> I have never used the nettle-meta interface for ciphers, so I'm not
> sure about whether that's needed or not. CCM for example is not
> represented there. Would it make sense to separate the meta interface
> from the addition of the cipher, or should I avoid adding the type
> completely?
Put these typedefs in testutils.h for now, if you find them useful for
the tests.
>> 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.
> My understanding of this RFC is that a primary goal as in 1.3.2, is to
> be resistant to nonce misuse (e.g., if you re-use the nonce). I can add
> more information but I am not sure I understand which direction of
> detail you mean. What are you missing or think is unclear in the text?
Something brief on what the consequences are if one accidentally or
deliberately uses the same nonce twice. You could even quote the rtc, if
it expresses it well and concisely.
>
>> > --- /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?.
>
> If we remove the tlength then it would not implement the
> nettle_encrypt_message_func(). My goal in keeping tlength was to have a
> consistent interface across the message oriented ciphers.
I'd prefer to not have the tlength argument. If we do a
nettle-meta-style interface for messge-oriented aead, we may use a fix
tlength, so that, e.g., CCM with full tag length (16 bytes) nd truncated
tag (say, 4 bytes) would be considered distinct aead algorithms.
>> 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):
>
> I can name it clength, but there is the implicit assumption that
> clength = slength right? Otherwise if a larger buffer is provided for
> destination, I do not see how it figure the source size.
clength == tlength + slength. This is what the docs for the
corresponding ccm functions say (which do have tlength argument, since
ccm supports truncated tag) say:
The CCM message fuctions provides a simple interface that will perform
authentication and message encryption in a single function call. The
length of the cleartext is given by MLENGTH and the length of the
ciphertext is given by CLENGTH, always exactly TLENGTH bytes longer than
the corresponding plaintext. The length argument passed to a function
is always the size for the result, CLENGTH for the encryption functions,
and MLENGTH for the decryption functions.
>> > --- /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;
>
> The goal here was not to make c aligned, but to have enough pad so that
> when this structure is read as cmac_aes128_ctx, to account for any
> alignment in the latter. Maybe actually I should do just that, and add
> the real types as union.
I think it's important that the context struct gets the alignment it
needs. Now, since cmac128_ctx has aligned contents, uint8_t pad will
likely get sufficient alignment anyway, but I think it's a bit brittle
to depend on that; an uint8_t[...] inside a struct usually doesn't get
larger alignment than a single byte.
>> 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.
>
> Done. Why do you prefer this?
Do ensure that the constants are allocated in the rodata segment. With
just const, that's an abbreviation for const auto, and the compiler
typically allocates them on the stack and generates code to initialize
them on each call, like any other auto variables.
Not entirely sure why the compiler doesn't optimize it better, but using
rodata for variables not explicitly declared static might violate some
subtle requirement of the C standard. (I think const in C++ works
differently, but I don't understand all the subtleties there either).
In addition, it would be good if these could be typed as nettle_block16
rather than char arrays, for alignment. That's easy for the const_zero block
(this is also done in ax.c:eax_set_key), but I'm not sure of the top of my head
how to write a union initializer for a
static const union nettle_block16 const_one = {...}
>> > +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.
>
> Why do you think that? Are there any benefits in that way?
The idea of the set_key function is to do all preparations that don't
depend on the actual message, so they don't have to be repeated. And I
think it's a bit odd to handle the keying of the two involved cipher
contexts so differently.
Slightly related: I think the cipher context struct(s) should be const
void*, when passed to siv_cmac_encrypt_message and
siv_cmac_decrypt_message. One usecase may be a server calling set_key at
startup, and then spawning worker threads encrypting or decrypting
messages. If all key-related data is const by the time the threads are
spawned, it can be shared without any mutexes or the like.
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
_______________________________________________
nettle-bugs mailing list
[email protected]
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
