Hello Niels,

Thanks for your feedback!

Le 2021-02-03 à 03 h 47, Niels Möller a écrit :

Thanks. Can you give a bit more details on the usecase? I've understood that it's part of web-related specs, but do you know any examples of protocols or applicatinos using it, and how?

The AES Key Wrap is an encryption scheme designed to encrypt/decrypt a cryptographic key, the RFC introduction says:

This key wrap algorithm needs to provide ample security to protect keys in the context of prudently designed key management architecture.

The AES key wrap is used in the JOSE standard to protect the encryption key used in some JWE (JSON Web Encryption) key management algorithms [1].

Basically, a JWE is an encrypted token. The payload encryption uses AES CBC|GCM with 128/192/256 bits key length. The AES key used to encrypt the payload is included encrypted in the token.

Concerning the algorithms used to encrypt the AES key, AES Key Wrap is required in 9 out of 17 possible algorithms.

I'm the author of the SSO Server Glewlwyd [2], Glewlwyd implements OpenID Connect, which uses JWT (JSON Web Tokens) via one of my library called Rhonabwy [3]. A JWT is a token signed (JWS) and/or encrypted (JWE). The encrypted tokens can be used for requests or response in lots of cases, to increase the security level of the application when it's required.

Currently Glewlwyd supports all signing algorithms specified, but not all encryption algorithm are supported yet. I intend to support as much possible encryption algorithm. And AES key Wrap is required in 9/17 specified encryption algorithms [4]

Should it really be AES only? It could be generalized to arbitrary block ciphers (as long as block size is an even number of bytes), or more easily to any 16-byte block cipher. I think the spec https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf mentioned a variant for "TDEA" (I guess that's triple-DES?), which doesn't seem that relevant.

It was designed to wrap key data, but not necessarily AES only. The kek must be an AES key though. The key data to wrap can be any data, as long as it's a set of 64 bits blocks.

Some comments on the code:

• Nettle usually doesn't return any status codes based on validation of inputs. The key wrap function should not have a return value. It can assume passed in pointers are valid, and use assert to check that lengths are valid. The unwrap function should obviously have a return value, but it doesn't need to check conditions that are trivial to check for the caller (not an obvious call if invalid ciphertext length for unwrap should trigger an assert or give a zero return value; whatever choice is made it needs to be clearly documented).

Noted, I'll remove the return statement for the key_wrap function, I'll also remove the input validation

• Try to avoid conditions on algorithm type. The common style in nettle is to have a general function taking an opaque context and needed function pointers as input, and then small convenience wrappers for algorithms of interest. So for the key wrap function, the cipher could be represented as a const void * for a cipher context already initialized with the "kek" key, and a nettle_cipher_func for invoking the cipher. If we want to support arbitrary block size (which is a bit questionable), it would also need a block size parametert.

Good point, instead of passing the kek in the input params, it's better to pass a struct aes_ctx which is supposed to be already initialized.

• If we restrict the implementation to 16-byte block size, it would be good to use wider types than uint8_t for the internal data shuffling. I think most or all internal state could be represented as union nettle_block16 or uint64_t. Movement (and xor) is independent of endianness. Easiest way to test for big-endian is likely to target mips, using cross compiler and qemu (like the mips builds in the gitlab ci). The

     I[7] ^= (n*j)+(i+1);
  
  

  line would differ depending on endianness, if we want to do on a 64-bit state variable. (Staying with bytes and memcpy may be fine if you think it makes the code simpler and there's no significant performance difference).

Good point too. Endianess is my kryptonite so my code would not be valid on all architectures...

• For the final memcmp in the unwrap function, you could use memeql_sec, to make the comparison side-channel silent (i.e., not leak information about the first differing byte).

noted too

• Since relation bewteen input and output size is very simple, we don't need both arguments. Just document the relation and provide appropriate #defined constants. The convention for similar functions is that the single length argument specifies the size of the *output* buffer.

So to be concrete on the interface comments, I'd suggest something like

void nist_keywrap16(const void *ctx, nettle_cipher_func *encrypt, const uint8_t *iv, size_t ciphertext_length, uint8_t *ciphertext, cosnt uint8_t *cleartext);

int nist_keyunwrap16(const void *ctx, nettle_cipher_func *decrypt, const uint8_t *iv, size_t cleartext_length, uint8_t *cleartext, const uint8_t *ciphertext);

We can also replace the uint8_t with uint64_t as well in the input values?

Not clear if default iv should be handled at this level. What are the usecases for specifying a different iv?

It's up to the calling algorithm. The IV is used for key data integrity [5]. Concerning using different IVs, the paragraph 2.2.3.2 mentions that alternative IVs can be used as part of larger key management protocols if the key data is not just an AES key, it may not always be a multiple of 64 bits.

Also, setting the default IV as a 'const uint8_t default_iv' in the function code isn't the best move, I totally agree.

Perhaps if instead of using uint8_t[8] I use uint64_t, then the default IV can be a classic #define DEFAULT_IV A6A6A6A6A6A6A6A6

...

I can add tests based on the tests vectors in the RFC [2], but I'm not sure how the test suites are build, should I need to create test_wrap functions like in aes-test.c or something else?

I think it would make sense to put tests in a new file. test_main should contain one fucntion call per test case, calling whatever helper functions are needed. It's probably sufficient to test algorithm-specific convenience functions.

I'll get back to the tests when the function code will be completed then.

Thanks for all the feedback and the help!

/Nicolas

[1] https://tools.ietf.org/html/rfc7518#section-4 [2] https://github.com/babelouest/glewlwyd [3] https://github.com/babelouest/rhonabwy [4] https://tools.ietf.org/html/rfc7518#section-4.1 [5] https://tools.ietf.org/html/rfc3394#section-2.2.3

Hello,

I've updated the MR with the new functions definitions and added test cases based on the test vectors from the RFC.

https://git.lysator.liu.se/nettle/nettle/-/merge_requests/19

Le 2021-02-04 à 01 h 48, Niels Möller a écrit :

...

It was designed to wrap key data, but not necessarily AES only. The kek must be an AES key though. The key data to wrap can be any data, as long as it's a set of 64 bits blocks.

If it doesn't add a lot of complexity, I think it would be nice to be able to substitute at least other 16-byte block ciphers (e.g., any other AES finalist, serpent and twofish being the once currently implemented in Nettle).

No problem, as long as cleartext is an array of 64bits, the function will wrap it, not matter what is is.

...

We can also replace the uint8_t with uint64_t as well in the input values?

It would be nice if we could have that alignment on input and output, but I think it's not a good idea to have uint64_t in this interface. We'd force alignment requirement on callers (who may be forced to make an extra copy to be able to call the function), and we'd also need to document endianness of the input. In short, since the specification defines the mechanism as operating on byte strings, the Nettle api should too.

I suspect that using byte strings in the interface and uint64_t internally makes it a bit difficult to allocate storage for internal state, since one can't just reuse the output buffer for working storage. Is the size needed for internal state same as the message size, or is if fixed size? I think it's doable but tricky to make it work without separate allocation for working storage.

After testing with uint64_t arrays for ciphertext and cleartext, I'd prefer getting back to uint8_t arrays instead. there are no iunt64_t data blocks used elsewhere in the library so one would have to convert all uint8_t to uint64_t and vice-versa, I think this change could be avoided.

Do you agree?

I was pointed to RFC 5649 (AES Key Wrap with Padding), is that relevant?

I'm not sure, the JOSE specification don't mention it so I wasn't aware of its existence before. I'll have a look but since it's an addon to RFC 3394, it may be implemented as an additional function.

/Nicolas

Hello,

Le 2021-03-04 à 04 h 14, Niels Möller a écrit :

I've added a couple of comments on the mr.

Thanks a lot!

I still have one uresolved comment about byte swapping but the rest are resolved.

One question: Do you intentionally limit message size to 64 bytes? Is that according to spec?

Not at all. At first I thought AES key wrap had an input limit because it's about wrapping cypher keys, so to me the limit was 64 bytes. But even if it's the intention, I don't see any specific limitation on input message in the specs.

The only limitation is to have cyphertext 8 bytes longer than cleartext, and cleartext to be at least 16 bytes to be a set of 8-bytes blocks.

Therefore I removed 'uint8_t R[64]' to use TMP_GMP_DECL(R, uint8_t); instead.

/Nicolas

Hello,

I haven't made all the changes you requested but here's my situation.

Le 2021-03-06 à 04 h 45, Niels Möller a écrit :

Nicolas Mora nicolas@babelouest.org writes:

• About the byteswapping comment, the code

  // A = MSB(64, B) ^ t where t = (n*j)+i
  A64 = READ_UINT64(B.b);
  A64 ^= (n*j)+(i+1);
  WRITE_UINT64(A.b, A64);
  
  

could be replaced by something like

#if WORDS_BIGENDIAN A.u64 = B.u64 ^ (n*j)+(i+1); #elif HAVE_BUILTIN_BSWAP64 A.u64 = B.u64 ^ __builtin_bswap64((n*j)+(i+1)); #else ... READ_UINT64 / WRITE_UINT64 or some other workaround ... #endif

The problem I have with this is the type of A and B: nettle_block8 and nettle_block16, and in nettle_block16, u64 is declared as 'uint64_t u64[2];'

So do I need to choose B.u64[0] or B.u64[1] depending on the architecture?

Preferably encapsulated into a single macro, so it doesn't have to be duplicated in both the wrap and the unwrap function. There's another example of using __builtin_bswap64 in ctr.c.

I've started something like that:

#if WORDS_BIGENDIAN #define MSB_XOR_T_WRAP(A, B, xor) A.u64 = B.u64 ^ (xor); #elif HAVE_BUILTIN_BSWAP64 #define MSB_XOR_T_WRAP(A, B, xor) A.u64 = B.u64 ^ __builtin_bswap64((xor)); #else #define MSB_XOR_T_WRAP(A, B, xor) \ uint64_t A64_##A##; \ A64_##A## = READ_UINT64 (B.b); \ A64_##A## ^= (xor); \ WRITE_UINT64 (A.b, A64_##A##); \ #endif

First I need to fix my B.u64 issue

• Intialization: If you don't intend to use the initial values, omit

initialization in declarations like

union nettle_block16 I = {0}, B = {0}; union nettle_block8 A = {0};

Fixed

• Some or all memcpys in the main loop can be replaced by uint64_t

operations, e.g.,

I.u64 = A.u64;

instead of

memcpy(I.b, A.b, 8);

I have the same problem with B.u64 being an array

...

Therefore I removed 'uint8_t R[64]' to use TMP_GMP_DECL(R, uint8_t); instead.

Unfortunately, that doesn't work: This code should go into libnettle (not libhogweed), and then it can't depend on GMP. You could do plain malloc + free, but according to the README file, Nettle doesn't do memory allocation, so that's not ideal.

I think it should be doable to reuse the output buffer as temporary storage (R = ciphertext for wrap, R = cleartext for unwrap). In-place operation (ciphertext == cleartext) should be supported (but no partial overlap), so it's important to test that case.

I've updated the MR to reuse ciphertext or cleartext as R. The original test cases still pass, I'll have to complete the tests.

Using the output area directly has the drawback that it isn't aligned, so you'll need to keep some memcpys in the main loop. One could consider using an aligned pointer into output buffer and separate handling of first and/or last block, but if that's a lot of extra complexty, I wouldn't do it unless either (i) it gives a significant performance improvement, or (ii) it turns out to actually be reasonably nice and clean.

I'll rely on your wisdom about that if you don't mind

• And one more nit: Indentation. It's fine to use TAB characters, but

they must be assumed to be traditional TAB to 8 positions: changing the appearance of TAB to anything else in one's editor is wrong, because it makes the code look weird for everyone else (e.g., in gitlab's ui). And the visual appearance should follow GNU standards, braces on their own lines, indent steps of two spaces, which means usually SPC characters, with TAB only for large indentation.

I agree, I've updated the indentation using gnu indent with gnu style

/Nicolas

Hello

Current status update

Le 2021-03-06 à 11 h 27, Niels Möller a écrit :

...

...

   // A = MSB(64, B) ^ t where t = (n*j)+i
   A64 = READ_UINT64(B.b);
   A64 ^= (n*j)+(i+1);
   WRITE_UINT64(A.b, A64);

I've added 2 macros definitions: MSB_XOR_T_WRAP and MSB_XOR_T_UNWRAP, I couldn't find how to make just one macro for both cases because of the direction of the xor.

...

I have the same problem with B.u64 being an array

.u64[0] for bytes 0--7, .u64[1] for bytes 8--15.

OK, I can set all .u64[0], but when it comes to .u64[1], I have a different behavior, example:

memcpy (I.b + 8, R + (i * 8), 8); // This one works I.u64[1] = *(R + (i * 8)); // This one doesn't work

Is there something I'm missing?

...

I agree, I've updated the indentation using gnu indent with gnu style

Thanks. One peculiarity with the gnu style is the space between function name and open parenthesis (which you have after reindent). Nettle isn't quite consistent, but new code mostly follows this convention.

No problem, keeping the code style consistent is very important for maintenance.

/Nicolas

Nicolas Mora nicolas@babelouest.org writes:

I've added 2 macros definitions: MSB_XOR_T_WRAP and MSB_XOR_T_UNWRAP, I couldn't find how to make just one macro for both cases because of the direction of the xor.

Hmm. Maybe better to define an optional swap operation. Like

#if WORDS_BIGENDIAN #define bswap_if_le(x) (x) #elif HAVE_BUILTIN_BSWAP64 #define bswap_if_le(x) (__builtin_bswap64 (x)) #else static uint64_t bswap_if_le(uint64_t x) { x = ((x >> 32) & UINT64_C(0xffffffff)) | ((x & UINT64_C(0xffffffff)) << 32); x = ((x >> 16) & UINT64_C(0xffff0000ffff)) | ((x & UINT64_C(0xffff0000ffff)) << 16); x = ((x >> 8) & UINT64_C(0xff00ff00ff00ff)) | ((x & UINT64_C(0xff00ff00ff00ff)) << 8); return x; } #endif

and then use as

B.u64[0] = A.u64 ^ bswap_if_le((n * j) + (i + 1));

Regards, /Niels

Nicolas Mora nicolas@babelouest.org writes:

I've updated the code with the new bswap_if_le and removed the commented code.

Hi, I haven't been paying attention to this for a few weeks, but I've had a nother look now. I've left a few comments on the MR.

For testing, it's important to test both in-place operation (that should be supported, right? Following the conventions documented at https://www.lysator.liu.se/~nisse/nettle/nettle.html#Conventions), and separate input and output area.

For the tests of unwrap, it is important to test the error case, i.e., try changing some bit in the input, and verify that unwrap returns an error.

The new feature also needs documentation, will you look into that once code, and in particular the interfaces, are solid?

Regards, /Niels

Nicolas Mora nicolas@babelouest.org writes:

...

The new feature also needs documentation, will you look into that once code, and in particular the interfaces, are solid?

Definitely! What do you think the documentation should look like? Should it be near paragraph 7.2.1? Like

7.2.1.1 AES Key Wrap

That's one possibility, but I think it would also be natural to put it somewhere under or close to "7.4. Authenticated encryption and associated data", even though there's no associated data. That section could perhaps be retitled to "Authenticated encryption" to generalize it?

Or possibly under "7.3 Cipher modes", if it's too different from the AEAD constructions.

Regards, /Niels

Hello,

I've added test cases to verify that unwrap fail if the input values are incorrect [1]. I reuse all the unwrap test cases, changed one ciphertext byte and expect the unwrap function to return 0.

Le 2021-03-29 à 13 h 32, Niels Möller a écrit :

That's one possibility, but I think it would also be natural to put it somewhere under or close to "7.4. Authenticated encryption and associated data", even though there's no associated data. That section could perhaps be retitled to "Authenticated encryption" to generalize it?

Or possibly under "7.3 Cipher modes", if it's too different from the AEAD constructions.

Until we come to a solution on where to put the documentation, I've started a first draft for the documentation. Can you give me feedback on it? I'm used to write documentation but for my projects and with my style, but I don't know if the text is too much or too few.

Also, I've never used LaTex. What tool do you recommend to write LaTex documentation? I've tried gummi but it says there are errors in the nettle.texinfo file...

/Nicolas

[1] https://git.lysator.liu.se/nettle/nettle/-/merge_requests/19/diffs#2c3905289...

Hello,

Le 2021-04-07 à 08 h 09, Aapo Talvensaari a écrit :

It says: "*ciphertext length must be cleartext_length-8" but shouldn't that be: "*ciphertext length must be cleartext_length+8"?

Indeed, the typo is similar in the other paragraph.

For void aesXXX_keywrap, it should say: "*cleartext length must be ciphertext_length-8."

For int aesXXX_keyunwrap, it should say: "*ciphertext length must be cleartext_length+8."

/Nicolas

Hello again,

Le 2021-03-06 à 11 h 27, Niels Möller a écrit :

...

I've updated the MR to reuse ciphertext or cleartext as R. The original test cases still pass, I'll have to complete the tests.

Hmm. I think you should leave the input buffer untouched, with type const uint8_t*, and only use the *output* area for temporary storage.

In the in-place case, those will be the same and the code needs to handle that case, but if caller passes separate input and output buffers, the input should not be modified.

I feel ashamed for that one :p

I've reverted the changes to use the output buffer as the intermediate value instead.

/Nicolas