Use GcmParams with AWS CloudHSM will cause undefined behavior

[zkonge]

trafficstars

AWS doc says

When performing AES-GCM encryption, the HSM does not accept initialization vector (IV) data from the application. You must use an IV that it generates. The 12-byte IV provided by the HSM is written into the memory reference pointed to by the pIV element of the CK_GCM_PARAMS parameters structure that you supply.

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

1. 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]>
2. add a new type GcmParamsMut and mark the GcmParams::new as unsafe (or safe is ok?).

[Direktor799]

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.

[wiktor-k]

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!

[keldonin]

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?

[zkonge]

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?

[hug-dev]

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?

[hug-dev]

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?

[zkonge]

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 😢.

[hug-dev]

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?

[wiktor-k]

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? 🤔