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

Add injection and derivation support #8

Closed
wants to merge 6 commits into from
Closed

Conversation

szszszsz
Copy link

@szszszsz szszszsz commented Jun 29, 2023

Add injection and derivation support needed for Nitrokey Webcrypt

  • for derivation allow to support different hash algorithms (future)

Note: the current implementation of HMAC to P256 key derivation is not safe and should not be used in production

@szszszsz
Copy link
Author

@sosthene-nitrokey
Copy link
Collaborator

@sosthene-nitrokey Any clue how to resolve this one?

* https://github.com/Nitrokey/trussed-staging/actions/runs/5410357176/jobs/9831705431#step:5:225

You need to add your backend to the dispatch impl in the virt module.

type HmacSha256P256 = Hmac<sha2::Sha256>;

let key_id = request.key;
let key = keystore.load_key(key::Secrecy::Secret, None, &key_id)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please verify that the key kind is a Symmetric key.

Copy link
Author

Choose a reason for hiding this comment

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

Done, but had to revert. Will check this again later.

Comment on lines +173 to +185
pub fn inject_any_key(
keystore: &mut impl Keystore,
request: &request::InjectAnyKey,
) -> Result<reply::InjectAnyKey, Error> {
let key_id = keystore.store_key(
request.location,
key::Secrecy::Secret,
request.kind,
&request.raw_key,
)?;

Ok(reply::InjectAnyKey { key: Some(key_id) })
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this syscall bring compared to the existing unsafe_inject_key syscall?

Copy link
Author

@szszszsz szszszsz Jun 29, 2023

Choose a reason for hiding this comment

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

I forgot to check opcard's implementation in this regard. There are no advantages of using this extension over Trussed's currently AFAIK. In the future supporting other kinds like curve25519 could be handy.

Comment on lines +159 to +170
let derived_key: [u8; 32] = mac
.finalize()
.into_bytes()
.try_into()
.map_err(|_| Error::InternalError)?;
let key_id = keystore.store_key(
request.location,
key::Secrecy::Secret,
key::Kind::P256, // TODO use mechanism/kind from the request
&derived_key,
)?;
Ok(reply::DeriveFromHash { key: Some(key_id) })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure that this necessarily creates a key that is valid?

The key generation is a bit more complex than just using random bytes. This is the procedure we currently use in trussed: https://docs.rs/p256-cortex-m4/0.1.0-alpha.6/src/p256_cortex_m4/cortex_m4.rs.html#81-93.

Especially, the private key has to be within the $[1; n-1]$ where $n$ is the prime order of P-256.

Copy link
Author

Choose a reason for hiding this comment

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

That's a correct observation - there are no checks for key validation as of now. I was thinking about adding the same checks like done in the linked implementation, but I am gravitating now more towards using curve25519 instead, and remove the p256 support for derivation, unless proved to be safe to use in this way.
I've just noticed there is no ticket for that - to be done soon.
Connected: trussed-dev/trussed#44 (comment)

Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey Jun 29, 2023

Choose a reason for hiding this comment

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

For EdDSA this will work but for X25519 (ECDH) there is the same issue, except the constraints on the scalar are even worse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One solution could be to simply use the result of the hash to seed a ChaCha8Rng and use it to generate the key. We would need it to commit to having the key generation algorithm be stable so that the same RNG always gives the same key.

These are the features required by Nitrokey Webcrypt.
This fixes the "large size difference in enum" warning
To avoid accidental use of other secret material
This reverts commit fdc90f3.

This is required to make Nitrokey Webcrypt working at the moment, but should be removed in the future.
@sosthene-nitrokey
Copy link
Collaborator

Replaced by Nitrokey/nitrokey-websmartcard-rust#16

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