Add support for message-based encryption and decryption (PKCS#11 3.0) #255
Conversation
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
This is follow-up with the recommendations from parallaxsecond#248 and this will actually allow use to call the new functions from the crate. Signed-off-by: Jakub Jelen <jjelen@redhat.com>
|
The clippy failure is something in the code that I did not touch so I assume rust/clippy was updated somewhere and new warnings are showing up. On the first sight, its not clear to me how to correctly fix it. |
Yeah, thats how much I got from the message, but its the continuation of the list item so I am not sure how it will render in the docs when I will remove the indent for the list. But I can try. It will break at worst :) |
Jakuje
force-pushed
the
message-encrypt
branch
2 times, most recently
from
991a958 to
66c25e8
Compare
Turned out it just needed one less space than I would expect in the RSA case (and few less than in the other). Fixed in the last commit. |
cryptoki/src/mechanism/mod.rs
Outdated
| @@ -805,6 +805,9 @@ pub enum Mechanism<'a> { | |||
| AesKeyWrapPad, | |||
| /// AES-GCM mechanism | |||
| AesGcm(aead::GcmParams<'a>), | |||
| /// AES-GCM mechanism with message based API and parameters | |||
| /// TODO should we reuse the AesGcm and use Option<> to select parameter? | |||
There was a problem hiding this comment.
I think the TODO should use normal comment (//) not to be rendered as docs 🤔
|
|
||
| /// True if the mechanism can be used to decrypt encrypted messages | ||
| /// | ||
| /// See [`Session::decrypt`](crate::session::Session::decrypt_message) |
There was a problem hiding this comment.
The below is important to catch stuff like here (decrypt vs decrypt_message).
| self.flags.contains(MechanismInfoFlags::MESSAGE_DECRYPT) | ||
| } | ||
|
|
||
| /// True if the mechanism can be used with encrypt/decrypt_bessage_begin API. |
| .into_result(Function::DecryptMessage)?; | ||
| } | ||
|
|
||
| data.resize(data_len.try_into()?, 0); |
There was a problem hiding this comment.
Is this call necessary if the vec is already created with correct size in the first place? 🤔
There was a problem hiding this comment.
I used the same approach as in the Session::decrypt() as tried and tested :)
I agree that this might not be necessary for the PKCS#11 modules that return the right value on the first try, but the PKCS#11 specs says that the value does not have to be exact and could be larger so shrinking the vector is probably a good thing to do here:
- If pBuf is NULL_PTR, then all that the function does is return (in *pulBufLen) a number of bytes which would suffice to hold the cryptographic output produced from the input to the function.� This number may somewhat exceed the precise number of bytes needed, but should not exceed it by a large amount.� CKR_OK is returned by the function.
From 5.2 Conventions for functions returning output in a variable-length buffer
There was a problem hiding this comment.
Great, thanks for digging this bit up!
But if the value returned can be bigger than the vector we pass in wouldn't the PKCS#11 call overwrite memory right after the Vec's buffer? And then the resize would still overwrite that PKCS#11-filled memory with zeros (since Vec implementation would "think" it has been uninitialized).
I guess this is a rare condition since the Vec would probably have spare capacity but thinking about all these edge cases gets me 😬
I understand the feel about adjusting the code around and then the reviewer being unhappy about not your code though 😅 Maybe we can track that in a separate issue and close this conversation here? 🤔
There was a problem hiding this comment.
No, it should work only the other way round -- the size query can return larger value, but the operation itself will return the right value so no buffer should overrun in this code, unless I miss something.
There was a problem hiding this comment.
Ack. Then the Vec::truncate would be more appropriate, IMO. (I'm not saying you should adjust it, just for completeness for future work 🫠 )
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Toward this change, I had to go through some hops.