Skip to content
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

Open
wants to merge 3 commits into
base: main-2.x
Choose a base branch
from

Conversation

swenson
Copy link
Contributor

@swenson swenson commented Nov 15, 2024

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

runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Show resolved Hide resolved
runtime/README.md Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
runtime/README.md Outdated Show resolved Hide resolved
@swenson
Copy link
Contributor Author

swenson commented Nov 18, 2024

(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
@swenson swenson force-pushed the docs/crypto-mailbox-fixes branch from 0ef0012 to c6b1916 Compare November 20, 2024 18:28
runtime/README.md Show resolved Hide resolved
| 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/CMKS/CMKs/

| 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.
Copy link
Contributor

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 |
Copy link
Contributor

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"?

Copy link
Contributor Author

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?


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.
Copy link
Contributor

@bluegate010 bluegate010 Nov 20, 2024

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.)

Copy link
Contributor Author

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.

| context size | u32 | |
| context | u8[...] | |
| iv | u8[12] | |
| cipertext size | u32 | MAY be 0 |
Copy link
Contributor

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.

Copy link
Contributor Author

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 |
Copy link
Contributor

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.

Copy link
Contributor Author

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.

| chksum | u32 | |
| fips_status | u32 | FIPS approved or an error |
| tag verified | u32 | 1 if tags matched, 0 if they did not |
| tag | u8[16] | Computed tag |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to return the tag if it was given in the init stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the tag did not match, it might be useful for debugging to know what the hardware thought the tag should be.

@@ -1402,14 +1523,15 @@ Command Code: `0x434D_4546` ("CMEF")
| chksum | u32 | |
| context size | u32 | size of context |
| context | u8[...] | This MUST come from the output of the `CM_ECDH_GENERATE` |
| key usage | u32 | usage tag of the kind of key that will be output |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps worth noting here that the key usage must be compatible with a 384-bit key (like, we can't pass AES-256-GCM here).

| ---------- | -------- | --------------------------------------- |
| chksum | u32 | |
| key usage | u32 | Tag to specify how the data can be used |
| input size | u32 | |
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants