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.

Sign up for GitHub

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

Use infallible conversion into instead of try_into #747

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Use infallible conversion into instead of try_into #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ec6d4c
Use infallible conversion into instead of try_into
gowthamsk-arm Jan 3, 2024
7a5f3e4
trusted_service: Add drop functions for AsymmetricEncryptIn and Asymm…
gowthamsk-arm Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions src/providers/trusted_service/context/asym_encryption.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,16 @@ use super::ts_protobuf::{
use super::Context;
use parsec_interface::operations::psa_algorithm::AsymmetricEncryption;
use parsec_interface::requests::ResponseStatus;
use std::convert::TryInto;
use zeroize::Zeroize;

impl Context {
pub fn asym_encrypt(
&self,
key_id: u32,
alg: AsymmetricEncryption,
mut plaintext: Vec<u8>,
Copy link
Collaborator

@minosgalanakis minosgalanakis Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am assuming zeroisation is no longer required and we trust the vector data to be the responsibility of the caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zeroisation was only happening when we would get an error from try_into(). With into() there is no error scenario so we would never zeroise. So it would be the caller's responsibility to clear as it is with the positive case.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what I find interesting is that we didn't zeroize it after sending the request, even though we don't pass ownership of the plaintext data, just a reference to it. Maybe we should do it before returning, regardless of whether it's an exception or not. Or maybe we didn't do it for some reason and I forgot the reason 🤔

Would it be possible for you to quickly check if zeroizing at the end breaks anything?

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 thought all the structs from ts_protobuf have Drop trail implemented here. But I can only see Drop being implemented for ImportKeyIn, ExportKeyOut, SignHashIn, VerifyHashIn. For these structs the drop method takes care of the zeroise part.

Not sure why AsymmetricEncryptIn and AsymmetricDecryptIn don't have them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test patch seems to work fine and there are no failures reported in the CI jobs for trusted services backend.

Copy link
Member

Choose a reason for hiding this comment

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

I would be more inclined for the Drop method of the involved types to handle the zeroization. Could you check if this is possible and easy to do? I'm approving in case it's not possible/easy.

mut salt: Vec<u8>,
plaintext: Vec<u8>,
salt: Vec<u8>,
) -> Result<Vec<u8>, ResponseStatus> {
let alg = alg.try_into().map_err(|e| {
plaintext.zeroize();
salt.zeroize();
e
})?;
let alg = alg.into();
let req = AsymmetricEncryptIn {
id: key_id,
alg,
Expand All @@ -37,14 +31,10 @@ impl Context {
&self,
key_id: u32,
alg: AsymmetricEncryption,
mut ciphertext: Vec<u8>,
mut salt: Vec<u8>,
ciphertext: Vec<u8>,
salt: Vec<u8>,
) -> Result<Vec<u8>, ResponseStatus> {
let alg = alg.try_into().map_err(|e| {
ciphertext.zeroize();
salt.zeroize();
e
})?;
let alg = alg.into();
let req = AsymmetricDecryptIn {
id: key_id,
alg,
Expand Down
5 changes: 2 additions & 3 deletions src/providers/trusted_service/context/asym_sign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use super::ts_protobuf::{SignHashIn, SignHashOut, VerifyHashIn};
use super::Context;
use log::info;
use psa_crypto::types::algorithm::AsymmetricSignature;
use std::convert::TryInto;

impl Context {
/// Sign a hash with an asymmetric key given its ID and the signing algorithm.
Expand All @@ -19,7 +18,7 @@ impl Context {
let proto_req = SignHashIn {
id: key_id,
hash,
alg: algorithm.try_into()?,
alg: algorithm.into(),
};
let SignHashOut { signature } = self.send_request(&proto_req)?;

Expand All @@ -39,7 +38,7 @@ impl Context {
id: key_id,
hash,
signature,
alg: algorithm.try_into()?,
alg: algorithm.into(),
};
self.send_request(&proto_req)?;

Expand Down
2 changes: 1 addition & 1 deletion src/providers/trusted_service/context/key_management.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ impl Context {
lifetime: KeyLifetime::Persistent as u32,
id,
policy: Some(KeyPolicy {
usage: key_attrs.policy.usage_flags.try_into()?,
usage: key_attrs.policy.usage_flags.into(),
alg: key_attrs.policy.permitted_algorithms.try_into()?,
}),
}),
Expand Down
14 changes: 14 additions & 0 deletions src/providers/trusted_service/context/ts_protobuf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,17 @@ impl Drop for VerifyHashIn {
self.signature.zeroize();
}
}

impl Drop for AsymmetricEncryptIn {
fn drop(&mut self) {
self.plaintext.zeroize();
self.salt.zeroize();
}
}

impl Drop for AsymmetricDecryptIn {
fn drop(&mut self) {
self.ciphertext.zeroize();
self.salt.zeroize();
}
}
Loading