Add memory zeroizing when needed #239
Conversation
hug-dev
added
the
security
|
Don't hesitate to tell if you think I have missed some data structures! |
| @@ -90,6 +90,7 @@ pub fn to_response_status(error: Error) -> ResponseStatus { | |||
| #[derive(Serialize, Deserialize)] | |||
| pub struct PasswordContext { | |||
| pub context: TpmsContext, | |||
| /// This value is confidential and needs to be zeroized by its new owner. | |||
| pub auth_value: Vec<u8>, | |||
There was a problem hiding this comment.
I think this should be Secret, actually, or does it not work with serializing?
There was a problem hiding this comment.
Agree, will try to change to it!
There was a problem hiding this comment.
does it not work with serializing
I first tried to put the auth_value in a Secret but it did not work because there is no way to take the ownership out of a Secret and that is needed to build an Auth (through a TryFrom).
Then I thought it would be best to directly use the tss_esapi Auth structure there directly. But that structure does not implement Serialize, Deserialize so it does not work.
I think I will leave how it is right now but we maybe should put the Auth value in the tss crate as a Secret and use it directly from there? It could be done as part of parallaxsecond/rust-tss-esapi#46
| @@ -284,6 +289,8 @@ impl TpmProviderBuilder { | |||
| .map_err(|_| { | |||
| std::io::Error::new(ErrorKind::InvalidData, "Invalid TCTI configuration string") | |||
| })?; | |||
| self.tcti.zeroize(); | |||
| self.owner_hierarchy_auth.zeroize(); | |||
src/providers/pkcs11_provider/mod.rs
Outdated
| @@ -275,10 +276,17 @@ impl Drop for Pkcs11Provider { | |||
| if let Err(e) = self.backend.finalize() { | |||
| format_error!("Error when dropping the PKCS 11 provider", e); | |||
| } | |||
| // The other fields containing confidential information should implement zeroizing on drop. | |||
| self.slot_number.zeroize(); | |||
| self.user_pin.zeroize(); | |||
There was a problem hiding this comment.
I'd say this one should probs be stored as a Secret too, and any other variable that stores a password/PIN etc.
There was a problem hiding this comment.
Added SecretString around the user_pin
I thought about that but then realised that the application name itself is not really confidential (but the token itself is)? As in, do you think an attacker would benefit from knowing the application name? |
Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
I guess it's only "confidential" in the case of |
|
I had a look at the code in the
Since |
Fix #122