gamringer/php-pkcs11

Support AWS CloudHSM SDK 1.1.1 and later

Closed this issue · 7 comments

natepage commented

Hi there,

I'm trying to use this extension with the AWS CloudHSM to encrypt/decrypt using the AES-GCM mechanism.
I'm facing an issue during encryption related to the IV.

I'm new to all this but after reading AWS docs many times and trying to understand the code here, it looks to me that it's not compatible because AWS CloudHSM SDK expects the IV of GcmParams to be a "zeroized buffer" and it sets it itself onto the GcmParams object, and this library expects a string for the IV on the \Pkcs11\GcmParams and set the ulIvLenbased on the given string.

Maybe there is something I'm not doing correctly but right now I end up in those situations:

  • Set $iv=random_bytes(12) on \Pkcs11\GcmParams and get error from AWS CloudHSM SDK [cloudhsm_pkcs11::encryption::aes_gcm] Iv invalid. Reason: Expect IV to be all 0
  • Set $iv to empty string on \Pkcs11\GcmParams and get error from AWS CloudHSM SDK [cloudhsm_pkcs11::encryption::aes_gcm] BP000: Expected 'ulIvBits' or 'ulIvLen' to be non-zero

Here are the AWS docs about the mechanism and the way it deals with IV: https://docs.aws.amazon.com/cloudhsm/latest/userguide/pkcs11-mechanisms.html#pkcs11-mech-annotations

natepage commented

Hi there,

Update on this one, I've managed to make this work by patching the current code and recompile the extension in our project.
I've modified the \Pkcs11\GcmParams class to allocate memory for the IV but not set any value for it and harcoded the IV length to 12 because that's what AWS CloudHSM expects. I've based this patch on the samples from AWS you can find here.

This is obviously not a viable solution as it breaks the \Pkcs11\GcmParams for any other integration. I was thinking about creating a dedicated GCM Params class for AWS which doesn't let you pass the IV as a constructor parameter and takes care of it the "AWS CloudHSM way" automatically.

Like I said I'm really new to working on PHP extension themselves but I'm happy to create a PR so it can benefit other people.

natepage commented

Here is the PR: #74

  • 👍1
  • 🎉1
gamringer commented

Hi @natepage,

sorry for the delay, I have been busy welcoming the newest addition to our family in the last few days.

I was unfortunately unable to test this extension with the AWS HSM because it seems that I misread the costs involved (I thought they had a $5000 upfront cost... or might have had at some point), so I am very glad that someone with access to it is opening this issue.

I am hesitant to create a new object specific to a vendor in this way just for the sake of integrating with AWS, especially considering that they provide their own vendor-defined mechanism that I think would be more suited to this. From the documentation page you linked in the first message, I find this:

CKM_CLOUDHSM_AES_GCM: This proprietary mechanism is a programmatically safer alternative to the standard CKM_AES_GCM. It prepends the IV generated by the HSM to the ciphertext instead of writing it back into the CK_GCM_PARAMS structure that is provided during cipher initialization. You can use this mechanism with C_Encrypt, C_WrapKey, C_Decrypt, and C_UnwrapKey functions. When using this mechanism, the pIV variable in the CK_GCM_PARAMS struct must be set to NULL. When using this mechanism with C_Decrypt and C_UnwrapKey, the IV is expected to be prepended to the ciphertext that is being unwrapped.

Have you tried CKM_CLOUDHSM_AES_GCM ? If you can find its value in /opt/cloudhsm/include/pkcs11t.h, add it to our own and see if that works. I think this would be a more elegant solution, also much simpler.

natepage commented

Hi @gamringer,

First congratulations! I hope everything went ok, and I created the issue only 2 days ago so I don't really call this a delay 😄

Agree, AWS CloudHSM is quite expensive, we are working on a big project with specific needs. In this context we have a dedicated AWS Account Manager and I've spent some time reading their documentation and had a video call with one of their experts in cryptography who recommended not to use this mechanism as it is vendor-defined and would create challenges later on in case we have to migrate our HSM solution.

If creating new parameters object isn't suitable, could we rework \Pkcs11\GcmParams to make $iv nullable and accept a new optional $ivLen parameter?
Then the logic would be something like:

IF $iv is null and $ivLen is numeric 
THEN set $iv to zeroized buffer using $ivLen for memory allocation

WDYT?

gamringer commented

one of their experts in cryptography who recommended not to use this mechanism as it is vendor-defined and would create challenges later on in case we have to migrate our HSM solution

I have issues with this as it

  1. Directly contradicts their own documentation
  2. Is also true of the avenue that he proposes as a replacement, since other vendors' solutions do not behave in that way

AWS corrupted the normal CKM_AES_GCM mechanism to modify its behaviour seemingly because they did not agree with some of its assumptions. This means that they made it vendor-defined as well, whether one agrees with their rationale or not.

I am fundamentally against modifying standard mechanisms to vendors' custom parameters mainly on the basis that if I start accommodating every vendor's idiosyncrasy, this project is likely to become overly complex, and those idiosyncrasies are in no way guaranteed to remain compatible across vendors, in which case we will end up having to choose between vendors.

In this case, the solution that you propose in your PR is, I believe, probably as elegant as possible, considering the above, but I still believe that using CKM_CLOUDHSM_AES_GCM would be the ideal way to go.

I will think about it some more over the coming days.

I would have probably kept it short under normal circumstance, but you did ask what I thought of it ;)

Regarding your PR, I believe that you forgot to add one file: pkcs11awsgcmparams.c

natepage commented

Thank you for explaining, and I completely get your point and agree that you cannot support every vendor out there otherwise it will become a gigantic work to maintain this library!

Regarding the documentation, our AWS Account Manager told me he will report the feedback internally as it was quite tidious to understand how everything works together and get to this point.

😄 you're right I've completely forgot to include the actual file for this new AWS GCM Parameters object, I've updated the PR.

Thank you for looking into this, and looking forward to hearing from you.

natepage commented

Hi @gamringer, hope you well and the newborn too 😄

Sorry to bump you here, but we have a project which will soon need to use this, and I was wordering if you had time to think about it?

Would love to use this extension instead of compiling our local fork ourselves 😄