Open zkonge opened 2 months ago
According to the SAFETY comment, rust-cryptoki assumes all the supported mechanisms will not mutate the parameters, but it actually depends on the specific HSM vendor? All the pointer casts will be UB if the loaded PKCS11 library mutates the input.
In this case, neither the C_EncryptInit
signature nor the standard specified whether the input should be const or not.
Maybe it shouldn't make such a const assumption at all, but changing all the Mechanism
codes to &mut
just for this specific case sounds cumbersome. Not sure what's the best option here.
simplely modify the function signature, from &'a [u8] to &'a mut [u8], and it also affect the variance of GcmParams<'a> , from PhantomData<&'a [u8]> to PhantomData<&'a mut [u8]>
I'm okay with changing the function's signature and then fixing everything recursively upwards.
Thanks for reporting!
FYI,
In order to comply with FIPS requirements (i.e. when HSMS are operated in FIPS mode), the IV for GCM is generated (randomly) by the HSM and the user is not allowed to provide it. Some vendors require the IV to be "nullified", some will return the IV concatenated with the encrypted payload, while others will populate the IV in GcmParams. AES GCM is a hot mess in PKCS#11!
The behaviour is really vendor-specific. Room for "AES GCM" flavours in the library?
See https://github.com/parallaxsecond/rust-cryptoki/pull/226
This is trickier than expected.
With the added mutability, the entire Mechanism
structure becomes non-copyable/cloneable.
We might need to either redesign the Mechanism
abstraction or add a try_clone
method.
Or easier, is the non-copyable/cloneable Mechanism
struct acceptable?
Do we see any other operations that would also modify the input Mechanism
? Agree that it would be cumbersome to have to change the whole Mechanism
lifetime + bounds only to satisfy this one use-case.
Would it be ugly to add another return value to the encrypt
function (that would be ignored for encryption functions that do not return anything else than the encrypted payload)?
Or as @keldonin said
some will return the IV concatenated with the encrypted payload
we could also "force" that on our side?
After discussion on Slack and having thought about it a bit more, now agree with making Mechanism
mutable.
Could we make it clear though in the signature of the function that the Mechanism
can be changed? Is that what you propose with this commit?
Actually in the commit https://github.com/parallaxsecond/rust-cryptoki/commit/43c8863750065224f56b29a3ab2a16b51272bdbe, I make all methods accept the "owned" Mechanism
, instead of mutable Mechanism
ref.
In that case, users must create new Mechanism
each time, and move it in while calling, so I think the mutability is not important.
Update: Well, it depends on the inner structure contains mutable references. If the flatten one (such as AesCbc([u8; 16])
) is mutable, the mutable Mechanism
ref is still required. So I still need use the mut Mechanism
ref instead of owned Mechanism
😢.
I see! Then I think we could just change the signature of every function to take a &mut Mechanism
where before we took a &Mechanism
. Then we can remove the safety notice in make_mechanism
. Since there is nothing in the spec saying that a mechanism can not be changed by the implementation I guess that is the correct thing to do?
I was thinking about it and maybe mut ref is necessary anyway (instead of moved owned value) so that the caller can read the modified value after the call? 🤔
AWS doc says
and in code https://github.com/parallaxsecond/rust-cryptoki/blob/024976fc892b96ad72fbc4af38cf9449c42034c4/cryptoki/src/mechanism/aead.rs#L39 https://github.com/parallaxsecond/rust-cryptoki/blob/024976fc892b96ad72fbc4af38cf9449c42034c4/cryptoki/src/mechanism/aead.rs#L58
It violate the Rust aliasing model, cases undefined behavior, and it's impossible to extract IV content from AWS CloudHSM SDK.
Possible solutions
&'a [u8]
to&'a mut [u8]
, and it also affect the variance ofGcmParams<'a>
, fromPhantomData<&'a [u8]>
toPhantomData<&'a mut [u8]>
GcmParamsMut
and mark theGcmParams::new
as unsafe (or safe is ok?).