Modifies the newly-added GCMParams to allow reading IV

[jefferai]

After submitting #71 we found this little snippet from Amazon's CloudHSM
documentation:

Note

When performing AES-GCM encryption, the HSM ignores the initialization
vector (IV) in the request and uses an IV that it generates. The HSM
writes the generated IV to the memory reference pointed to by the pIV
element of the CK_GCM_PARAMS parameters structure that you supply.

This isn't, so far as we can tell, a requirement of the v2.4 standard,
but CloudHSM is already used quite a lot and that will only grow.

This changes the structure to allow pulling out the IV after an encrypt
operation, but with the caveat that manual freeing is necessary. We
didn't see a better way of handling this without significantly changing
the API of the actual PKCS11 calls as they're mapped into the library.
This does make an assumption that no library will free this memory for
you during its handling of the Encrypt call, but that would be behavior
at odds with how all other structures are handled, where the client code
performs the free.

The other way it might be handled would be to add EncryptInitGCM and
EncryptGCM but it feels more intrusive to create special purpose calls
for individual ciphers.

[jefferai]

Note: I work with @chrishoffman and he provided feedback and ✔️ on this.

[mtharp]

I'm not comfortable exposing a type that needs to be manually freed. A callback to copy the IV from the C arena back into the original IV slice (or a different slice that IV() would return) would be much safer.

The original patch also has a leak and double-free issue due to the free being invoked after the PKCS#11 function is called. It would be more straightforward if Mechanism had no C allocations (does not call cGCMParams), and just held onto the GCMParams object in a private member until cMechanismList is called.

[jefferai]

@mtharp Thanks for the feedback. I'm trying to think through the callback solution...the problem is that the param needs to be provided at EncryptInit time, but it isn't written with the new IV until after the Encrypt call. This is actually dangerous -- if it's freed after EncryptInit, the HSM library is still going to write to the (now freed) data at the address where the IV previously was.

What I need, in both a functional and safety sense, is a way for EncryptInit to not free memory after the call and defer it until after Encrypt. It didn't seem like there was any kind of decent public API for this, and it's an issue particular to AES-GCM. It may even be particular to this one kind of HSM, but since it's widespread enough it would be good to properly support. And I wouldn't put it past other vendors not to do the same thing, since using a bad IV (specifically, reusing an IV) with AES-GCM is catastrophic, so ensuring you are using something pulled from a random pool is a safety thing from the HSM vendor. But of course, here the automatic memory handling in this library is actually causing a safety issue.

Do you have any ideas for the best way to approach this? Making a callback is fine, but that still leaves the memory being freed after EncryptInit, which is still unsafe in this case.

[jefferai]

Also any chance you could be a bit more explicit about the double free issue in the original patch? I'm not really seeing the problem.

[mtharp]

If you reuse the same Mechanism object for multiple calls to EncryptInit then the first call will free the GCM params and the second one will try to use it and crash or have other undefined behavior. It's not a sane use case, but it shouldn't be possible to cause low-level faults like that from Go. Your patch fixes that by making the GCM params not automatically freed after the call.

It seems like CloudHSM's GCM behavior is relying on something the spec does not allow for. There are mechanisms that return data in buffers passed as a parameter, but they are single-function invocations (e.g. DeriveKey) so having a buffer survive a sequence of calls seems well outside what is required. But despite being stupid behavior from the vendor (which is par for the course when it comes to PKCS#11) I can think of 3 ways to approach this safely:

• Make it the caller's problem, as you have done here.
• Hide the buffer in the Ctx object. You can only have one operation running at a time, so other than guarding it with a mutex this wouldn't be too complicated, but it's kinda magical.
• Don't implement GCMParams in the pkcs11 package; instead, push it down to the new p11 sub-package which has a higher-level interface and single-step functions that can handle the allocation. This is brand new so I don't know whether it will work for you, I haven't really had a chance to look at it myself either.

I think the first approach, continuing with what you have done in this pull request, is probably the best way forward. Multiple calls to Free should be safe, so clear area after freeing it. Also the documentation should specify that Free must always be called and should be deferred until after the final Encrypt/Decrypt. An example in the doc string would probably help.

[jefferai]

Hi @mtharp ,

Thanks for explaining about the double free -- you are right, I just wasn't thinking about object reuse since I was mostly thinking about my changes :-)

I looked at the spec and it doesn't explicitly not allow, as far as I can tell...but...it's not great. I understand why they do it, and they probably just figure that since client libs are in charge of memory, client libs can wait to free memory related to an encryption operation until it's fully finished (which also seems like a semi-sane assumption that is just as sane to not adhere to). As you said, par for the course...

Option 2 does seem magical. I think it could be done but may set a weird precedent, not that the current way doesn't. Also if an operation is abandoned after the init it may be hard to realize that it should be freed without populating checks everywhere Ctx is used to run cleanups.

Option 3 still uses the pkcs11 package primitives so it could hide the problem from users but I think it still has the problem.

I've updated the PR to ensure multiple calls to free are safe, and have added more comments.

[mtharp]

Looks good to me. I was able to spin up a CloudHSM and verify. I was not able to validate against a SafeNet/Gemalto Luna SA, I'm getting a CKR_MECHANISM_PARAM_INVALID error and it seems like other people have had similar problems without any apparent solution. Doesn't really matter though, merged.

[jefferai]

Thanks!