search

GoogleCloudPlatform / kms-integrations

https://cloud.google.com/kms
Apache License 2.0
36 stars 13 forks source link

Unable to use signtool /ds option with the CNG provider #20

Open obones opened 11 months ago

obones commented 11 months ago

Hello,

Because of the absence of an x86 version of the provider (See #18), I'm trying a workaround by using the following steps:

  1. get the file digest by using the /dg option

    signtool sign /q /f "%CertificatePublicKeyFilename%" /du %SignatureUrl%  /fd sha256 /dg . "%FileToSign%"

  2. Sign the generated digest with the CNG provider and the /ds option

    signtool sign /q /f "%CertificatePublicKeyFilename%"  /csp "%GoogleCSPName%" /kc "%GoogleCloudKeyVersionFullPath%" /ds "%DigestFile%"

  3. Integrate the signed digest with the /di option

    signtool sign /q /f "%CertificatePublicKeyFilename%" /di . "%FileToSign%"

  4. Timestamp the generated signature

    signtool timestamp /q /tr %SignatureTimeStampURL% /td sha256 "%FileToSign%"

When using a certificate whose private key is stored on a physical usb token (Yubikey), this works fine. But when I use the CNG provider, I get the following error:

I0927 12:56:16.817818   31368 logging.cc:81] returning 0x80090027 from SignHashFn due to status INVALID_ARGUMENT: at bridge.cc:421: unsupported pPaddingInfo [type.googleapis.com/kmscng.StatusDetails='SECURITY_STATUS=0x80090027']

Looking at bridge.cc:421 the error seems to be created on purpose but I'm not sure what to make of this. I would simply remove the test as it seems pPaddingInfo is not used at all in that function, but this must be a very naive approach.

Any help would be most welcome.

tdbhacks commented 10 months ago

Hello,

I am not familiar with this flow, but per the CNG spec, the pPaddingInfo parameter should really only be used when passing in structs of type BCRYPT_PKCS1_PADDING_INFO or BCRYPT_PSS_PADDING_INFO.

I guess the parameter description leaves some room for interpretation because it says "This parameter is only used with asymmetric keys and must be NULL otherwise", and ECDSA is an asymmetric algorithm, but we are expecting it to be NULL in our provider because it doesn't really make sense to specify it for ECDSA algorithms (unless my reading of the spec is wrong).

Just to confirm, are you trying to use this with an ECDSA key or an RSA-PKCS1 / RSA-PSS key? See the limitations section. If you are indeed using an ECDSA key, I'll tweak the provider logging when I have some time and try to go through the same flow to understand what it's trying to set it to. Thanks for bringing this up!

obones commented 10 months ago

To me this is an ECDSA key: image

And the CNG provider works if I don't use the /ds option but rather have signtool do all actions at once.

tdbhacks commented 10 months ago

Indeed, I found some time to reproduce this with my own EC-P256 key and I also ran into the same error.

Played with it for a bit and tweaked our logging to check what is being passed to the provider: it looks like SignTool is trying to use the BCRYPT_PAD_PKCS1 flag in combination with a BCRYPT_PKCS1_PADDING_INFO struct with pszAlgId == SHA256, so at this point I'm pretty confused because this whole flow should only apply to RSA-PKCS1 keys...

We should investigate further, unclear if this is our bug or a weird behavior of SignTool, but will keep this issue open as a bug for now.

ysichrisdag commented 5 months ago

Is there any plan to address this bug? Really need either this one addressed or https://github.com/GoogleCloudPlatform/kms-integrations/issues/18

tdbhacks commented 3 months ago

@obones @ysichrisdag as I was going through the open issues of the repo, I figured I'd try out this flow again now that RSA4096-PKCS1 support has been merged (#29). I haven't tried ECDSA, I assume the bug is still present and I don't think we can do much about that, but I believe I was able to sign something successfully using RSA_SIGN_PKCS1_4096_SHA256!

Is ECDSA a requirement? If not, it would be great if you could test this out with RSA

obones commented 3 months ago

I haven't tried ECDSA, I assume the bug is still present and I don't think we can do much about that

Well, this issue is here just because I tried a workaround issue #18 but if that other issue was to be addressed, then this one could go way down in the priority list.

Is ECDSA a requirement?

Yes, it is because it comes from my signing key provider as I require a signing key backed by the PKI recognized by the Windows operating system.

obones commented 2 weeks ago

FWIW, I tried with signtool from SDK 10.0.26100.0 but this did not change the situation