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

RFC: support flexible key derivation APIs #857

Closed
DL8 opened this issue Aug 24, 2022 · 8 comments
Closed

RFC: support flexible key derivation APIs #857

DL8 opened this issue Aug 24, 2022 · 8 comments

Comments

@DL8
Copy link
Contributor

DL8 commented Aug 24, 2022

Taking the discussion about key derivation APIs from #855

Current key derivation APIs use hard-coded values for the key request, most notably SVNs (ISV, and CPU) are taken directly from the currently running enclave (see in sgx_get_seal_key()). This has several consequences:

  1. Backwards compatibility: keys are different per SVNs, so previously sealed blobs are no longer accessible after an enclave/platform SVN update.
  2. Key wear out protection: the key ID field is expected to be filled with a random value on encryption (the same value will be used to derive the key to decrypt it). In current implementation this value will always be zero, so derived keys might be overused, which may be a cryptographic weakness
  3. Compatibility with other frameworks: two enclaves communicating on the same platform may want to derive shared keys to communicate secrets with each other. The other enclave may use another framework that provides control over parts of the key request (e.g. key ID and SVNs). If the other enclave sends a sealed blob that was sealed with different ISV SVN or key ID != 0, it won't be possible to decrypt it using Gramine

This may not be a straightforward change, as it may require changes to key derivation APIs (PalGetSpecialKey() and corresponding pseudofiles) and may break backwards compatibility.

One solution I can think of:

  1. Modify sgx_get_seal_key() to get a more detailed key request
  2. Add a new special key. For the sake of the discussion, let's call it _sgx_sealing_custom
  3. Add capability to write key request parameters into _sgx_sealing_custom. The written value will be stored in internal state
  4. When reading _sgx_sealing_custom, _PalGetSpecialKey() will be modified such that it will use the previously stored key request parameters

Some opens with this proposal:

  1. How will it work with multithreading? I think key request should be stored per-thread
  2. Key request sanitization: I think some basic checks should be in place (e.g. allow sealing keys only) and to let the rest happen in EGETKEY. I'm not sure whether it should happen when writing the policy or when reading the key
  3. What happens to the policy after a successful key read? Is it reset or does it remain for next key request?

Another approach is to create a special handle just to write the policy and use it when reading _sgx_mrsigner and _sgx_mrenclave. I prefer the first proposal, because:

  1. It will be easier to add KSS support (see RFC: Add support for KSS (key separation and sharing) #800 )
  2. It might be too easy to break existing code: old code using _sgx_mrsigner might suddenly get different keys because of a stale key request
@mkow
Copy link
Member

mkow commented Aug 24, 2022

Key wear out protection: the key ID field is expected to be filled with a random value on encryption (the same value will be used to derive the key to decrypt it). In current implementation this value will always be zero, so derived keys might be overused, which may be a cryptographic weakness

I think this is because we use our own KDF on that key later, so there's no weakness here?

@DL8
Copy link
Contributor Author

DL8 commented Aug 24, 2022

How does the KDF work and when is it used? Do you mix some random value for each newly derived key? If not, the weakness may be applicable

@dimakuv
Copy link
Contributor

dimakuv commented Aug 25, 2022

@DL8 You can look at this function and its comment: https://github.com/gramineproject/gramine/blob/master/common/src/protected_files/protected_files.c#L142-L145

Basically, yes, we mix a random value (nonce) with the KDK (key derivation key) to generate a newly derived key. This happens for every new 4KB block of the file (including on re-writes of the same block).

@DL8
Copy link
Contributor Author

DL8 commented Aug 25, 2022

The solution you described is applicable only for protected files. What about keys derived by the application (read directly from key derivation APIs)?

@dimakuv
Copy link
Contributor

dimakuv commented Aug 25, 2022

What about keys derived by the application (read directly from key derivation APIs)?

What are these key derivation APIs? We don't expose such things in Gramine to the app.

@DL8
Copy link
Contributor Author

DL8 commented Sep 8, 2022

So if I understand correctly, the challenge here is how to let the developer configure protected filesystem keys in a flexible manner. I feel that using key name mechanism as is might be very cumbersome, because there are too many combinations. Another solution I have in mind is to add something like key parameters property to fs.mounts entries in the manifest. It will look something like this:

fs.mounts = [
  { type = "encrypted", path = "[PATH]", uri = "[URI]", key_name = "[KEY_NAME]", key_parameters = { ... } },
]

key_parameters default values will be defined in a backwards compatible manner. When the key is fetched, it will be parsed and the result will eventually propagate to sgx_get_seal_key() (PalGetSpecialKey() is probably a good candidate where to parse it).
I don't know what key_parameters should look like, so your feedback is welcome.

@dimakuv
Copy link
Contributor

dimakuv commented Sep 8, 2022

@DL8 I see your point now. But this new helper key_parameters is... well, extremely hard to explain to users.

At some point, Gramine needs to put limits on how configurable it is. Our manifest syntax is already very complex, and I feel that it's better for Gramine to just use some sensible defaults rather than leave it to the enclave developer (= manifest writer) to specify all the nitty-gritty SGX details.

@mkow
Copy link
Member

mkow commented Apr 9, 2023

I think we have no plans to implement this in any foreseeable future, so I'm closing it. Gramine doesn't expose any C APIs to the app, so the proposal from the original post is not viable, and doing it from the manifest looks quite complicated.
We may reopen this issue though, once a concrete use case appears.

@mkow mkow closed this as not planned Won't fix, can't repro, duplicate, stale Apr 9, 2023
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

No branches or pull requests

3 participants