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