Added asymmetric encrypt and decrypt #63

sbailey-arm · 2020-07-06T11:08:42Z

Signed-off-by: Samuel Bailey samuel.bailey@arm.com

hug-dev

That looks pretty good! A few comments.

hug-dev · 2020-07-06T11:52:43Z

src/operations/psa_asymmetric_decrypt.rs

+    /// Validate the contents of the operation against the attributes of the key it targets
+    ///
+    /// This method checks that:
+    /// * the key policy allows encrypting messages


encrypting should perhaps be decrypting in this context

Yep, whoops! I somehow missed that but caught the one at the end...

hug-dev · 2020-07-06T11:56:46Z

src/operations/psa_asymmetric_decrypt.rs

+    /// * the key policy allows encrypting messages
+    /// * the key policy allows the encryption algorithm requested in the operation
+    /// * the key type is compatible with the requested algorithm
+    /// * if the algorithm is RsaPkcs1v15Crypt, it has no salt (it is not compatible with salt)


Just an idea: would it be wrong to include the salt inside the AsymmetricEncryption structure (in the RsaOaep structure)? That way this condition would be checked statically. But that seems to be a big deviation from the PSA API.

I think that's a good idea, even if it deviates from the PSA API a bit. It sounds more 'Rusty' to me 😉.

If we do that, we need to have separate implementations of RsaOaep for policy and for operations - it doesn't make any sense to have a salt for a policy. Then, if we do have an operation-specific enum for algorithms, I don't see why we couldn't include algorithm-specific fields inside those structures/variants.

I see, would be good but probably not as part of this PR.

hug-dev · 2020-07-06T12:04:06Z

src/operations_protobuf/generated_ops.rs

+    fn clear_message(&mut self) { self.plaintext.zeroize();
+    self.salt.zeroize(); }


hmm is rustfmt drunk?

Doesn't appear to be drunk, does appear to be AWOL though! Running cargo fmt --all on parsec-book doesn't change that file and presumably the CI checks don't catch it either.

ionut-arm · 2020-07-06T13:23:36Z

src/operations/psa_asymmetric_decrypt.rs

+pub struct Result {
+    /// The `plaintext` field contains the decrypted short message.
+    #[derivative(Debug = "ignore")]
+    pub plaintext: Secret<Vec<u8>>,


I'm not convinced a plaintext is worth wrapping in Secret - we need to be careful to wipe it out of memory when done, but it's not "passwords, cryptographic keys, access tokens or other credentials". I'm mostly concerned because this is a user-facing structure, if it was internal to the service it wouldn't have mattered.

hmmm yeah forcing the consumer to use Secret might not be a good idea. If plaintext is put into a Vec<u8> and give ownership of that to the consumer, does that not mean its their responsibility to deal with zeroing the memory?

It could be passed in Zeroizing<Vec<u8>> which would handle the cleaning up - and it's inherently their data to deal with anyway. I'm in two minds about this, because it does make sense to have more protection over plain text, but Secret seems overkill (and not really intended for the purpose). Wondering if we need a new structure of our own for this kind of data.

Oh are you talking about cyphertext in decrypt? I thought you meant about returning it in encrypt. I'm in the process of migrating to Zeroizing<Vec<u8>> in decrypt.

I think I'm getting confused here - my initial question was whether plaintext should be wrapped in a Secret. If we decide to do that, we should keep it consistent and do that both in the encrypt operation and in the decrypt result. Tbh I'd lean towards just using Zeroizing, but if you or @hug-dev disagree, you can leave it as a Secret!

I think if someone is going to the effort of encrypting something, it is secret from something or someone (otherwise, why encrypt it?). And, if they are encrypting a password, cryptographic keys or access tokens - plaintext will then contain something that Secret was designed to wrap (according to the description of the crate) 😄.

I guess the question is, where do you draw the line? If we are deciding between Zeroize and Secret, what are the downsides to Secret over Zeroize in this situation?

because this is a user-facing structure

I think it does not have to be in all clients! We were talking about changing the interface in the Basic client to have references everywhere, so that the client is free to use any type they want, but we zeroize internally everything we clone.

Any of the two is good for me, the important part being that both are zeroizing. Secret is more difficult to use, but easier to audit and less prone to accidental leaking.

Most probably, the simplicity benefits of using Zeroizing outweigh the security advantages of Secret, and hence I would lean towards using Zeroizing here.

Well, there are 2 sides to this: is the "protection" needed? and is it semantically correct? I'd say the first answer is yes because as you say, we want to protect the client data, but the second is no, because the crate and its contents are explicitly aimed at protecting sensitive material (based on its documentation).

It kinda goes both ways in terms of (perceived) protection, though, if you put the plaintext at the same level as the secret key - some people might say "wow, you really take care of my data", others could see it as "hmm, does that mean my keys are only as important as my data to you?".

In any case, as I've said, I'm ok with this being Secret, we just need to document it thoroughly. Although this might mean we have to switch the hash in the signing operations to Secret too, it's kinda at the same level

ionut-arm · 2020-07-06T13:41:41Z

src/operations/psa_asymmetric_decrypt.rs

+// Debug derived as NativeResult enum requires it, even though nothing inside this Result is debuggable
+// as `plaintext` is sensitive.
+#[derive(Derivative)]
+#[derivative(Debug)]
+pub struct Result {
+    /// The `plaintext` field contains the decrypted short message.
+    #[derivative(Debug = "ignore")]
+    pub plaintext: Secret<Vec<u8>>,
+}


I'm curious what the debug print of this would be. It might be worth implementing Debug manually to print something like Result { plaintext: [REDACTED] } if you're keen on keeping plaintext as Secret

That might still be needed when using Zeroizing, if we are keeping Debug on those structures? I'm sure that with the #[derivative(Debug = "ignore")] it would print something similar as what you wrote, we can check.

impl<Z: Debug + Zeroize> Debug for Zeroizing<Z>, Zeroizing implements Debug. Do we still want to ignore it? Most other places that Zeroize is used don't ignore, it's only data in psa_import_key.

Indeed, stuff that's in Zeroizing is generally not ignored, but maybe it should be. I don't think anyone would want their data (any data) to be printed by mistake somewhere - guess that's another place where having Secret helps .

I've added it to plaintext. I'm not sure about ciphertext or salt, they don't sound sensitive enough to me to warrant adding ignore to, but perhaps users might not agree?

I don't think anyone would want their data (any data) to be printed by mistake somewhere

Though if we're going to the effort of Zeroizing them...

I'm not sure about ciphertext or salt, they don't sound sensitive enough to me

It's not plaintext data, but could be helpful to an attacker and our goal is to make it as hard as possible for them to access it. And as you say, if we wipe it from memory but log it somewhere...

ionut-arm · 2020-07-06T13:47:13Z

src/operations_protobuf/generated_ops.rs

+impl ClearProtoMessage for psa_asymmetric_encrypt::Operation {
+    fn clear_message(&mut self) {
+        self.plaintext.zeroize();
+        self.salt.zeroize();
+    }
+}
+
+impl ClearProtoMessage for psa_asymmetric_decrypt::Operation {
+    fn clear_message(&mut self) { self.salt.zeroize(); }
+}
+
+impl ClearProtoMessage for psa_asymmetric_decrypt::Result {
+    fn clear_message(&mut self) { self.plaintext.zeroize(); }
+}


For the fields that implement zeroizing on Drop (i.e. Secret and Zeroizing) you don't need to zeroize them here, it'll be done later anyway. So actually, only ciphertext in psa_asymmetric_encrypt::Result needs clearing (even if it's encrypted). Actually, I'm wondering if it should use Zeroizing as well, since it's a buffer

Ok. Is there a reason that these don't need to be zerioized here but other operations do need to zerioize their Zeroizing data here? E.g. psa_verify_hash::Operation has hash and signature that are both Zeroizing<Vec<u8>> and they are zeroized here.

Ah, sorry, yeah, I realised that this is zeroizing the protobuf stuff 😅 carry on! (although I'd still recommend zeroizing ciphertext too)

hug-dev · 2020-07-07T08:39:52Z

src/operations/psa_asymmetric_decrypt.rs

+#[derive(Debug)]
+pub struct Result {
+    /// Decrypted message
+    pub plaintext: Vec<u8>,


Let's put the same Zeroizing here that we use on Operation of psa_asymmetric_encrypt! Basically let's just put Zeroizing on every plaintext, ciphertext and salt.

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

sbailey-arm · 2020-07-07T09:53:38Z

To summarise, the changes are ciphertext, plaintext and salt are all Zeroized and are all marked as ignore for Debug.

ionut-arm

Thanks for all the changes!

sbailey-arm marked this pull request as ready for review July 6, 2020 11:14

hug-dev reviewed Jul 6, 2020

View reviewed changes

hug-dev approved these changes Jul 6, 2020

View reviewed changes

sbailey-arm force-pushed the add-asym-encrypt-decrypt branch from 89e1c3e to 0feda29 Compare July 6, 2020 12:50

ionut-arm reviewed Jul 6, 2020

View reviewed changes

sbailey-arm force-pushed the add-asym-encrypt-decrypt branch from 0feda29 to c361f5d Compare July 6, 2020 14:41

hug-dev approved these changes Jul 6, 2020

View reviewed changes

sbailey-arm mentioned this pull request Jul 6, 2020

Added asymmetric encrypt and decrypt to Mbed Crypto provider parallaxsecond/parsec#196

Merged

hug-dev reviewed Jul 7, 2020

View reviewed changes

sbailey-arm force-pushed the add-asym-encrypt-decrypt branch 3 times, most recently from 46618a0 to 5d76e17 Compare July 7, 2020 09:43

Added asymmetric encrypt and decrypt

bc47296

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

sbailey-arm force-pushed the add-asym-encrypt-decrypt branch from 5d76e17 to bc47296 Compare July 7, 2020 09:50

ionut-arm approved these changes Jul 7, 2020

View reviewed changes

ionut-arm merged commit fcc6fdf into parallaxsecond:master Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added asymmetric encrypt and decrypt #63

Added asymmetric encrypt and decrypt #63

sbailey-arm commented Jul 6, 2020

hug-dev left a comment

hug-dev Jul 6, 2020

sbailey-arm Jul 6, 2020

hug-dev Jul 6, 2020

sbailey-arm Jul 6, 2020

ionut-arm Jul 6, 2020

hug-dev Jul 6, 2020

hug-dev Jul 6, 2020

sbailey-arm Jul 6, 2020

ionut-arm Jul 6, 2020

sbailey-arm Jul 6, 2020

ionut-arm Jul 6, 2020

sbailey-arm Jul 6, 2020

ionut-arm Jul 6, 2020

sbailey-arm Jul 7, 2020 •

edited

Loading

hug-dev Jul 7, 2020

ionut-arm Jul 7, 2020 •

edited

Loading

ionut-arm Jul 6, 2020

hug-dev Jul 7, 2020

sbailey-arm Jul 7, 2020 •

edited

Loading

ionut-arm Jul 7, 2020

sbailey-arm Jul 7, 2020

sbailey-arm Jul 7, 2020

ionut-arm Jul 7, 2020

ionut-arm Jul 6, 2020

sbailey-arm Jul 6, 2020 •

edited

Loading

ionut-arm Jul 6, 2020

hug-dev Jul 7, 2020

sbailey-arm commented Jul 7, 2020

ionut-arm left a comment

		fn clear_message(&mut self) { self.plaintext.zeroize();
		self.salt.zeroize(); }

Added asymmetric encrypt and decrypt #63

Added asymmetric encrypt and decrypt #63

Conversation

sbailey-arm commented Jul 6, 2020

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbailey-arm Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbailey-arm Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbailey-arm Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbailey-arm commented Jul 7, 2020

ionut-arm left a comment

Choose a reason for hiding this comment

sbailey-arm Jul 7, 2020 •

edited

Loading

ionut-arm Jul 7, 2020 •

edited

Loading

sbailey-arm Jul 7, 2020 •

edited

Loading

sbailey-arm Jul 6, 2020 •

edited

Loading