-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime docs: Remove storage from cryptographic mailbox #1795
base: main-2.x
Are you sure you want to change the base?
Conversation
(Discussion offline: we'll change AES to use the init / update / final flow, so that we can remove CMK from the update / final ones, which reduces some of the confusion around that I think.) |
Mostly. We need to keep a limited amount of storage per AES-256-GCM key to track usage, since these keys can only be used a certain number of times due to their IV problems. For HMAC, we don't specify a key usage on the output MAC, since it has to be imported back into Caliptra to be used as a key, where its usage will have to be specified. We also fix a few other minor issues with the mailbox documentation: Fixes #1753 Fixes #1754 Fixes #1755 Fixes #1756
0ef0012
to
c6b1916
Compare
runtime/README.md
Outdated
| block offset | 24 | Starting block number | | ||
| size | 24 | Size (in bytes) of the data stored. | | ||
| | | Does not have to be a multiple of 16 bytes. | | ||
The key used to encrypt the CMKs is randomized on reset, which means that CMKS cannot be used between resets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/CMKS/CMKs/
runtime/README.md
Outdated
| size | 24 | Size (in bytes) of the data stored. | | ||
| | | Does not have to be a multiple of 16 bytes. | | ||
The key used to encrypt the CMKs is randomized on reset, which means that CMKS cannot be used between resets. | ||
The iv is a randomized 1-up counter that is incremented for every key created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: IV
| ---------- | ----------- | | ||
| 0 | Reserved | | ||
| 1 | HMAC | | ||
| 2 | HKDF | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"HMAC-SHA2-384", "HKDF-SHA2-384"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might support multiple sizes for each, though, and I don't know if it is worth having values for each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just seems a bit odd that the AES entry is fully-described, whereas HMAC/HKDF are not. Seems like they should be consistent. So, maybe for consistency with the HMAC commands below, we just have this table include an "AES" entry, and pass the AES algorithm to CM_AES_GCM_[ENCRYPT/DECRYPT]_INIT
(while taking care to ensure that one can't create a non-block-sized AES key).
runtime/README.md
Outdated
|
||
The IV is the concatenation of `block offset || size`. | ||
This is necessary for AES-256-GCM in particular to ensure that keys are only used a certain number of times, as per [NIST SP 800-38D, Section 8.3](https://doi.org/10.6028/NIST.SP.800-38D). | ||
Only AES-256-GCM keys need to be tracked in this table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/need to be tracked/are tracked/? (Though I'm ok if we just track everything for consistency.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add some language to clarify that we might track others, but it is not guaranteed for now. It will depend on the implementation I think.
runtime/README.md
Outdated
| context size | u32 | | | ||
| context | u8[...] | | | ||
| iv | u8[12] | | | ||
| cipertext size | u32 | MAY be 0 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAY be 0
We would expect this to always equal the plaintext size
from the input args, right? Ditto for other ciphertext/plaintext size return args below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I was probably thinking of the SHA cases, where you have to buffer a whole block, but this should always be equal to the plaintext size. I'll adjust this language.
| -------------- | -------- | ------------------------------------ | | ||
| chksum | u32 | | | ||
| fips_status | u32 | FIPS approved or an error | | ||
| tag verified | u32 | 1 if tags matched, 0 if they did not | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should just fail the mailbox command if the tag mis-matched? Or with the tag
arg are you anticipating a debugging use-case?
Risk is that someone could forget to check the tag verified
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am anticipating a debug use case. I've seen cases before where something was wrong with the AAD or the tag computation but it is useful to know what the hardware / firmware thought the tag should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Suggest putting language up top noting that callers MUST check the "tag verified" output argument to verify integrity.
runtime/README.md
Outdated
| ---------- | -------- | --------------------------------------- | | ||
| chksum | u32 | | | ||
| key usage | u32 | Tag to specify how the data can be used | | ||
| input size | u32 | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the input size must agree with the key usage?
| context size | u32 | | | ||
| context | u8[...] | | | ||
| cipertext size | u32 | MUST be equal to ciphertext size | | ||
| ciphertext | u8[...] | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "MUST be equal to ciphertext size" note should also for consistency apply to the context as well as the ciphertext.
That being said, I think we could try and clean this up a bit. Something like:
| context_size | u32 |
| context | u8[context_size] |
| cipertext_size | u32 |
| ciphertext | u8[cipertext_size] |
| ---------- | ----------- | | ||
| 0 | Reserved | | ||
| 1 | HMAC | | ||
| 2 | HKDF | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just seems a bit odd that the AES entry is fully-described, whereas HMAC/HKDF are not. Seems like they should be consistent. So, maybe for consistency with the HMAC commands below, we just have this table include an "AES" entry, and pass the AES algorithm to CM_AES_GCM_[ENCRYPT/DECRYPT]_INIT
(while taking care to ensure that one can't create a non-block-sized AES key).
@@ -1120,13 +1128,15 @@ The sequence to use these are: | |||
|
|||
For each command, the context from the previous command's output must be passed as an input. | |||
|
|||
The CMK must have been created for HMAC / HKDF usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we will allow a CMK whose usage tag says HKDF (and not HMAC) to be passed to CM_HMAC commands? Seems like we should either disallow HKDF-tagged keys here, or just remove the HKDF entry in the key tag list (and maybe rename HMAC to HMAC_HKDF). Similar comment for the language under CM_HKDF_EXTRACT
.
@@ -1206,6 +1216,8 @@ Command Code: `0x434D_4846` ("CMHF") | |||
|
|||
Implements HKDF-Extract as specified in [RFC 5869](https://www.rfc-editor.org/rfc/rfc5869.html). | |||
|
|||
The CMK must have been created for HMAC / HKDF usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's document which usage tag the resulting CMK is given.
| | | with HKDF-Expand | | ||
|
||
### CM_HKDF_EXPAND | ||
|
||
Implements HKDF-Expand as specified in [RFC 5869](https://www.rfc-editor.org/rfc/rfc5869.html). | ||
|
||
The CMK must have been created for HMAC / HKDF usage. | ||
|
||
The output length will be automatically chosen to match the key usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key usage tag enum currently has three entries: HMAC, HKDF, and AES-256-GCM. If the caller passes AES-256-GCM, there's no ambiguity and Caliptra can pull out the correct amount of keys.
However, if the caller passes HMAC or HKDF, the key size is ambiguous.
Since the key size is not fully described by the key usage tag, it sounds like we can't actually just make the output size automatic.
However, it would be nice to prevent callers from doing silly things like creating a 10-byte HMAC key, or a non-block-sized AES key.
| PRK CMK | CMK | | | ||
| key usage | u32 | usage tag of the kind of key that will be output | | ||
| Info length | u32 | | | ||
| Info | u8[...] | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potential use-case for key derivation could involve pulling multiple keys out of one larger one. (Bits 0..255 are an AES key, bits 256..511 are an HMAC key, etc.)
We could enable that use-case without too much trouble by allowing the caller to pass in byte offsets when requesting a key be expanded. That would require the caller to invoke this command multiple times to get all the keys out, but that should be ok.
This would dovetail with the comment made above about how we maybe can't just infer the key size based on the usage tag.
| -------------- | -------- | ------------------------------------ | | ||
| chksum | u32 | | | ||
| fips_status | u32 | FIPS approved or an error | | ||
| tag verified | u32 | 1 if tags matched, 0 if they did not | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Suggest putting language up top noting that callers MUST check the "tag verified" output argument to verify integrity.
| free blocks | u32 | Available 16-byte blocks | | ||
| total blocks | u32 | Total 16-byte blocks | | ||
| free CMKs | u32 | Available CMKs | | ||
| total CMKs | u32 | Total CMKs | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The figures reported by this command would only correspond to CMEKs for which Caliptra tracks usage counters, right? If the implementation doesn't allocate any internal memory for HMAC/HKDF keys, those keys would not be reflected by this command response. A few options I see:
- Just track usage for every key and make it consistent.
- Specify that only AES keys are tracked, and have this command return stats only relevant for AES keys.
- Continue to allow the implementation to be flexible, and somehow report to the user whether a given key type is reflected in this stat.
Mostly.
We need to keep a limited amount of storage per AES-256-GCM key to track usage, since these keys can only be used a certain number of times due to their IV problems. We can keep track of this storage (and remove things from it) using the
CM_STATUS
, etc., commands we had previously, though we will have to also keep track that we don't allow deleted CMKs to be used again.We also have some potential issues across resets, since this data is not persistent.
For HMAC, we don't specify a key usage on the output MAC, since it has to be imported back into Caliptra to be used as a key, where its usage will have to be specified.
We also fix a few other minor issues with the mailbox documentation:
Fixes #1753
Fixes #1754
Fixes #1755
Fixes #1756