COSE Encryption Functionality (with HPKE-based key distribution) #46
Conversation
Change-Id: I190f58a82eb4a883e04f1a5cf17cd40bd27ea9ff
hannestschofenig
changed the title
There was a problem hiding this comment.
Overall, this looks pretty good. I think mostly cleaning up the whitespace to match the t_cose coding still is the main thing to do: spaces around operators, space after comma, consistent spacing (well, lack of space) inside parens, and maybe a few other things.
src/t_cose_encrypt_dec.c
Outdated
| QCBOR_DECODE_MODE_NORMAL); | ||
|
|
||
| /* Make sure the first item is a tag */ | ||
| result=QCBORDecode_GetNext(&DC,&Item); |
There was a problem hiding this comment.
The = operator should have a space, for consistency with the rest of the library. This comment applies throughout the rest of the source.
There was a problem hiding this comment.
In my most recent update I have worked on the coding style issues.
src/t_cose_encrypt_dec.c
Outdated
| if (kid_cbor.len==0 || | ||
| strncmp(me->kid.ptr,kid_cbor.ptr,me->kid.len)!=0 | ||
| ) { | ||
| return( EXIT_FAILURE ); |
There was a problem hiding this comment.
Also here, spacing should match the rest of the library, in this case no spaces inside the parens.
| @@ -15,7 +15,7 @@ add_definitions(${thirdparty_def}) | |||
|
|
|||
| if(MBEDTLS) | |||
|
|
|||
| set(T_COSE_SOURCE crypto_adapters/t_cose_psa_crypto.c src/t_cose_sign1_sign.c src/t_cose_parameters.c src/t_cose_sign1_verify.c src/t_cose_util.c) | |||
| set(T_COSE_SOURCE crypto_adapters/t_cose_psa_crypto.c src/t_cose_sign1_sign.c src/t_cose_parameters.c src/t_cose_sign1_verify.c src/t_cose_util.c src/t_cose_encrypt_enc.c src/t_cose_encrypt_dec.c) | |||
There was a problem hiding this comment.
This is getting long enough that it might make sense to break this into multiple lines.
|
Hi @hannestschofenig ... do you think this is something that could realistically make it into TF-M 1.6.0? Have you brought it up with that team at all so that an eventual t_cose update is on their radar? |
Change-Id: Id3ee1617bafd047d9337b07bb114c8a567f0a744
Change-Id: I7939754fd11e3492d1122d9dc806b8f732a7d1c7
There was a problem hiding this comment.
There’s a ton of good work created by an organized mind here! I know how much work it was to get signing done. :-)
I only reviewed the encryption interface, but the interface is the really important part. I’ve spent almost a week on it.
The big thing I’d like to do is rearrange how the recipients work to work well with all the different types of COSE recipients there are. We probably need a conference call to discuss as my comments probably aren’t entirely clear. I am happy to provide a lot of support and do some work here too.
Another thing is that I think we can get down to having two copies of the payload in RAM with this QCBOR improvement. laurencelundblade/QCBOR#137. Right now there are three when you count plaintext, cipher text and final encoded COSE_Enc.
I also made some examples of UsefulBuf that might help laurencelundblade/QCBOR#136. I won’t claim that UsefulBuf is some ultimate amazing coding solution, but it is the convention here and the code will be a lot cleaner if it is used through out.
I think this will come together for a really good COSE_Enc implementation!
| t_cose_encrypt_set_encryption_key( struct t_cose_encrypt_enc_ctx* context, | ||
| uint8_t* cek, | ||
| size_t cek_len); | ||
|
|
There was a problem hiding this comment.
cek should be a struct useful_buf_c
| t_cose_encrypt_hpke_set_encryption_key( struct t_cose_encrypt_recipient_hpke_ctx* context, | ||
| uint8_t* cek, | ||
| size_t cek_len); | ||
|
|
There was a problem hiding this comment.
cek should be a useful_buf_c
It can be optional to call this. If you don't call it the CEK will be generated automatically and passed to the recipient call back in the propose recipient design.
It will be an error to not call this for Encrypt0.
inc/t_cose/t_cose_encrypt_enc.h
Outdated
| QCBOREncodeContext* encrypt_ctx, | ||
| struct q_useful_buf_c payload, | ||
| struct q_useful_buf_c encrypted_payload | ||
| ); |
There was a problem hiding this comment.
by reading the code, it seems that encrypted_payload is a temporary internally used buffer to hold the output of the AEAD algorithm before it is CBOR-encoded into the output buffer. This means it must be slightly larger than payload or this will fail. This also means three copies of the payload in memory to encrypt 1) the plaintext, 2) the intermediate AEAD output and 3) the final COSE_Encrypt message. This 3x multiplication of the payload size in not so good for an embedded implementation with limited memory.
With a small addition QCBOR this can brought down to 2 and the need for this temporary buffer eliminated. I think this is a good idea because it a) saves memory and b) simplifies the interface. The down side is that it will require a new version of QCBOR. The change is not complicated.
laurencelundblade/QCBOR#137
If we keep it the way it is then there should be documentation that says that this is scratch buffer that is needed to complete the encryption and that it's size must be xxx buffer than the payload. xxx is calculated as....
It should also be a q_useful_buf, the one that is not const, because it is an empty buffer to which something will be written.
inc/t_cose/t_cose_encrypt_enc.h
Outdated
| struct q_useful_buf_c detached_payload, | ||
| struct q_useful_buf_c encrypted_payload, | ||
| size_t* encrypted_payload_size | ||
| ); |
There was a problem hiding this comment.
I made a UsefulBuf example here: laurencelundblade/QCBOR#136 Hope it helps explain this type of interface.
Here's the interface I propose for the last 3 parameters:
struct q_useful_buf_c payload,
struct q_useful_buf buffer_for_detached_ciphertext,
struct q_useful_buf_c &detached_ciphertext
It's the actual cipher text that is detached, not the payload/plaintext so the names are changed.
The caller must supply a buffer to hold the detached cipher text. It must be sized correctly by being xxx larger than the payload where xxx is calculated by ... to-be-filled-in... (the documentation should say how to calculate it, or better provide a cpp macro so it can be used to allocate a stack variable)
(Note that the pointer in buffer_for_detached_ciphertext is appropriately not const as it is going to be written into).
The pointer and length of the detached cipher text is placed in detached_ciphertext (which is an out parameter; the pointer to the data is in it is appropriately const as it is only to be read).
inc/t_cose/t_cose_encrypt_enc.h
Outdated
| */ | ||
| enum t_cose_err_t | ||
| t_cose_encrypt_hpke_create_recipient( struct t_cose_encrypt_recipient_hpke_ctx* context, | ||
| QCBOREncodeContext* EC); |
There was a problem hiding this comment.
This would turn into the implementation of the call back in the new proposed recipient design.
| struct q_useful_buf_c protected_parameters; | ||
| int32_t cose_algorithm_id; | ||
| uint8_t* key; | ||
| size_t key_len; |
There was a problem hiding this comment.
key should be a q_useful_buf_c
| uint32_t option_flags, | ||
| int32_t cose_algorithm_id, | ||
| struct q_useful_buf_c kid); | ||
|
|
There was a problem hiding this comment.
Enc0 doesn't require a kid and Encrypt doesn't exclude a kid, so this doesn't seem right. Seems like enc_init() and enc0_init() can be combined and there can be a set_kid() that can be used with either.
| t_cose_encrypt_add_recipient( struct t_cose_encrypt_enc_ctx* context, | ||
| QCBOREncodeContext* encrypt_ctx, | ||
| UsefulBufC* recipient_ctx); | ||
|
|
There was a problem hiding this comment.
This would take a different form altogether in the new recipient design.
| struct t_cose_key ephemeral_key; | ||
| struct t_cose_key recipient_key; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I’d like to do the interface for recipients in a general and expanded way to be able to accommodate all the different types of recipients COSE supports. This design uses a call back and is inspired by object-oriented principles.
There’d be different implementations of different types of recipients, hpke being just one of many. They’d share a base structure that holds a pointer to a call back function. They’d be separated out in different files from each other and from t_cose_encrypt_enc.
I’m pretty confident that this can work and it will give good results (small code size, very flexible, simple interface).
Here’s some very crude description. Probably want a conference call to discuss.
/* This is the definition of a function that t_cose_encrypt_enc
will callback for each recipient registered with it. See t_cose_encrypt_add_recipient().
When called this will take as input a pointer to t_cose_encrypt_recipient_hpke_ctx
or similar and the cek. It will then encode the COSE_Recipient to EC.
*/
typdef int (*get_recipient_f)(void recipient_context,
struct q_useful_buf_c cek,
QCBOREncodeContext EC);
/*
This is kind of abstract definition of a recipient. It is
just enough to hold the call back. It is common to
all the different types of recipient implementations.
t_cose_encrypt_recipient_hpke_ctx is just one of many.
*/
struct t_cose_recipient_ctx {
get_recipient_f get_recipient;
};
/**
- This is the context for creating a recipient structure for use with
- HPKE. The caller should allocate it and pass it to the functions here.
- The size of this structure is around 56 bytes.
/
/ This is the same as before but with the call back - added. It will be cast to/from a struct t_cose_recipient_ctx. /
struct t_cose_encrypt_recipient_hpke_ctx {
get_recipient_f get_recipient;
/ Private data structure /
int32_t cose_algorithm_id;
struct q_useful_buf_c kid;
uint8_t cek;
size_t cek_len;
uint32_t option_flags;
struct t_cose_key ephemeral_key;
struct t_cose_key recipient_key;
};
/* This registers a new recipient with the t_cose_encrypt_enc_ctx.
- When t_cose_encrypt_enc() is called the callbacks registered
- by this call will be fired to encode the COSE_Recipients. /
enum t_cose_err_t
t_cose_encrypt_add_recipient( struct t_cose_encrypt_enc_ctx context,
const struct t_cose_recipient_ctx *recipient);
| static void | ||
| t_cose_encrypt_hpke_set_ephemeral_key( struct t_cose_encrypt_recipient_hpke_ctx* context, | ||
| struct t_cose_key ephemeral_key); | ||
|
|
There was a problem hiding this comment.
Seems like you could get rid of the function entirely by putting the ephemeral key generation in the recipient functionality. That is in t_cose_encrypt_hpke_create_recipient().
This will make it simpler for the caller to use.
hannestschofenig
force-pushed
the
latest_t_cose
branch
2 times, most recently
from
bb2ca0c
to
c78ca24
Compare
|
With the most recent PR I have taken the coding style used in t_cose into account. The primary changes, however, are due to the use of the crypto adaptation layer, which was work done at the IETF hackathon in Vienna. |
| uint8_t *plaintext, size_t plaintext_len, | ||
| size_t *plaintext_output_len | ||
| ); | ||
|
|
There was a problem hiding this comment.
The interface looks pretty good. Glad it is pretty simple.
Aad support can be added by a separate set_aad() method I think, similar to the encrypt side.
Need to switch to usefulbuf for the inputs here.
|
|
||
| int ret; | ||
| uint8_t add_data[20]; | ||
| size_t add_data_len = sizeof(add_data); |
There was a problem hiding this comment.
Hard coding this length is not a good long term solution because it will limit the size of headers and aad. In the short term, I think this can be a hard and documented limit, but maybe this for the long term.
- Return an error code that indicates this was too small.
- A method that lets you get the necessary size of this buffer.
- A method that lets you supply this buffer.
| /* Temporary storge area for encrypted cek. */ | ||
| uint8_t tmp2[50]; | ||
| UsefulBufC ephemeral = {(uint8_t *) tmp, sizeof(tmp)}; | ||
| UsefulBufC cek_encrypted = {(uint8_t *) tmp2, sizeof(tmp2)}; |
There was a problem hiding this comment.
You shouldn't initialize a const usefulbufC with an empty buffer. Use UsefulBuf for that.
tmp[] and tmp2[] are not actually used here. They get overwritten by the GetBytes call later.
The compiler would have caught this as a const / non-const error.
| struct t_cose_key key, | ||
| struct q_useful_buf_c kid); | ||
|
|
||
|
|
There was a problem hiding this comment.
An area of future work is something to be able to get the headers without decrypting. This is necessary to get kid and such. It is probably part of work for generalizing recipient implementation. Hopefully it doesn't affect any of the other interfaces here. Doesn't seem like it to me.
There was a problem hiding this comment.
The main thing in this review was the crypto adapter interface. That is a public interface that should be kept stable in the long run so it is important it is thought through.
Mostly, I think what is there is organized well.
I haven't looked at what it would be like to implement using OpenSSL, but that should be checked eventually.
So far t_cose hasn't had to do anything with COSE_Key, but encryption changes that and begs a bunch of design questions. Eventually t_cose should have public methods for operating on COSE_Key, but that will be a lot of work. My thought is to start separating out COSE_Key now, but not make it public. However we will have to do some work in the crypto adapter layer to facilitate conversion of COSE_Key to struct t_cose_key. You've already written the code for this. It just needs to be rearranged to make the interfaces work right.
| enum t_cose_err_t | ||
| t_cose_crypto_export_public_key(struct t_cose_key key, | ||
| struct q_useful_buf pk_buffer, | ||
| size_t *pk_len); |
There was a problem hiding this comment.
should be a struct q_useful_buf_c * out, not a size_t -- for usefulbuf and const consistency.
| struct q_useful_buf_c plaintext, | ||
| struct q_useful_buf ciphertext, | ||
| size_t *ciphertext_len); | ||
|
|
There was a problem hiding this comment.
should be a struct q_useful_buf_c * out, not a size_t -- for usefulbuf and const consistency.
| struct q_useful_buf_c ciphertext, | ||
| struct q_useful_buf plaintext, | ||
| size_t *plaintext_len); | ||
|
|
There was a problem hiding this comment.
should be a struct q_useful_buf_c *out, not a size_t -- for usefulbuf and const consistency.
| t_cose_crypto_get_cose_key(int32_t cose_algorithm_id, | ||
| uint8_t *cek, | ||
| size_t cek_len, | ||
| uint8_t flags, |
There was a problem hiding this comment.
Should be a struct useful_buf_c in for the cek
| struct q_useful_buf_c add_data, | ||
| struct q_useful_buf_c ciphertext, | ||
| struct q_useful_buf plaintext_buffer, | ||
| size_t *plaintext_output_len); |
There was a problem hiding this comment.
should be a struct q_useful_buf_c * out, not a size_t -- for usefulbuf and const consistency.
|
|
||
| if (context == NULL || EC == NULL) { | ||
| return(T_COSE_ERR_INVALID_ARGUMENT); | ||
| } |
There was a problem hiding this comment.
I consider this check unnecessary and actually a waste of code.
If every function in t_cose and CBOR checked for EC == NULL it would bloat the whole implementation a lot.
| */ | ||
| enum t_cose_err_t | ||
| t_cose_crypto_get_random(struct q_useful_buf buffer); | ||
|
|
There was a problem hiding this comment.
There should be an additional out parameter that is a struct q_useful_buf_c * where const output is returned. Yes, this is entirely redundant in that the pointer and length will be the same, but this is the way usefulbufs work. Non-const buffer to write to [in], const buffer with data [out].
It will also clean up the calling code where you make the nonce. You won't need the cast to const there.
| */ | ||
| enum t_cose_err_t | ||
| t_cose_crypto_hpke_decrypt(int32_t cose_algorithm_id, | ||
| struct q_useful_buf_c pkE, |
There was a problem hiding this comment.
pKE should be a struct t_cose_key.
To make that work there should be a function to decode a COSE_Key into a struct t_cose_key that replaces a lot of code
in t_cose_encrypt_dec() and is part of the general support for encoding and decoding COSE_Key.
| } | ||
|
|
||
| /* Decode ephemeral */ | ||
| QCBORDecode_Init(&DC2, |
There was a problem hiding this comment.
This should turn into a COSE_Key decoder function in a separate file that supports encoding and decoding COSE_Key.
| */ | ||
| if (me->key_distribution == T_COSE_KEY_DISTRIBUTION_DIRECT) { | ||
| if (kid_cbor.len == 0 || | ||
| strncmp(me->kid.ptr, kid_cbor.ptr, me->kid.len) != 0 |
There was a problem hiding this comment.
Use usefulbuf compare function instead of strncmp. Use of strncmp here is bug because the data is binary, not a string.
|
replaced by #116 |
This PR contains COSE encryption functionality and offers key distribution via HPKE, as described in https://datatracker.ietf.org/doc/html/draft-ietf-cose-hpke-01.
Note: The HPKE library found at https://github.com/hannestschofenig/mbedtls/tree/hpke is necessary to compile this code.
Open Issue: To provide the necessary abstraction for cryptographic libraries in t_cose the relevant PSA Crypto API calls need to be moved into the abstraction layer.
Change-Id: I190f58a82eb4a883e04f1a5cf17cd40bd27ea9ff