Support HMAC signatures and verification #2385

williamcroberts · 2021-08-13T22:08:28Z

V6:

Fix make dist check for leftover files from test added in V5.

V5:

Added a test
Fixed LGTM error on invalid format specifier (passing unsigned long to unsigned), should be 0x%lx not 0x%x.

V4:

Created a new field for storing metadata in the mf_info struct so it can be tracked per mechanism
Populated the HMAC routines I can verify

V3:

Only try to look for a secret key on decrypt or sign ie ignore that for derive.
Have the failure message only include secret key on sign or decrypt failures.

V2: (Ignore)

V1: Initial

This PR adds support for signing and verifying with HMAC using pkcs11-tool.

For example:

# Alias to make life simpler
alias tpm2pkcs11-tool='pkcs11-tool --module /home/wcrobert/workspace/tpm2-pkcs11/src/.libs/libtpm2_pkcs11.so.0.0.0'

# C_Sign
tpm2pkcs11-tool --token-label=label --login --pin=myuserpin --sign \
  --id='393837363534333231' --input-file=data.msg \
  --mechanism=SHA-1-HMAC --output-file=data.sig
Using signature algorithm SHA-1-HMAC

# C_Verify
tpm2pkcs11-tool --token-label=label --login --pin=myuserpin --verify \
  --id='393837363534333231' --input-file=data.msg --mechanism=SHA-1-HMAC \
  --signature-file=data.sig
Using signature algorithm SHA-1-HMAC
Signature is valid

Checklist

PKCS#11 module is tested

Jakuje · 2021-08-16T10:49:56Z

src/tools/pkcs11-tool.c

 					opt_object_id_len ? opt_object_id : NULL,
 					opt_object_id_len, 0))
-			util_fatal("Private key not found");
+			util_fatal("Private key nor secret key found");


You should probably split the above conditions to match only your intended operation (Sign). Although decrypt might work also for some secret keys and mechanisms, I am not sure about the derive operation.

Not to mention that in cryptography there's no signing with symmetric keys, only MAC'ing.

It's good to be able to compute keyed hash (e.g., HMAC) of an object, but I wonder what's the actual use case is.

Not to mention that in cryptography there's no signing with symmetric keys, only MAC'ing.

Which is effectively and practically a symmetric signature and is referred to that in both the TCG TPM Architectural docs as well as practically through PKCS11 via the C_Sign and C_Verify operations as well as OpenSSL's C API of EVP_Sign and EVP_Verify interfaces.

It's good to be able to compute keyed hash (e.g., HMAC) of an object, but I wonder what's the actual use case is.

There are a lot of specific use cases for an HMAC, but the general use case is always the same, detect authenticity and tampering.

You should probably split the above conditions to match only your intended operation (Sign). Although decrypt might work also for some secret keys and mechanisms, I am not sure about the derive operation.

I split it up, trying to keep the patch as small as possible to accomplish the goal, HMAC sign and verify.

dengert

Sorry this should not be a review I am submitting it as a comment.

mouse07410 · 2021-08-16T18:10:38Z

It's good to be able to compute keyed hash (e.g., HMAC) of an object, but I wonder what's the actual use case is.

There are a lot of specific use cases for an HMAC, but the general use case is always the same, detect authenticity and tampering.

I'm familiar with a lot of HMAC use cases, having participated in IPsec development back when. I wonder what is the PKCS#11-related use case for HMAC, given that to "verify" symmetric "signature", both endpoints need to have a copy of the symmetric key, which somewhat defeats the idea of having only one copy of the signing key inside an HSM.

williamcroberts · 2021-08-16T18:17:55Z

It's good to be able to compute keyed hash (e.g., HMAC) of an object, but I wonder what's the actual use case is.

There are a lot of specific use cases for an HMAC, but the general use case is always the same, detect authenticity and tampering.

I'm familiar with a lot of HMAC use cases, having participated in IPsec development back when. I wonder what is the PKCS#11-related use case for HMAC, given that to "verify" symmetric "signature", both endpoints need to have a copy of the symmetric key, which somewhat defeats the idea of having only one copy of the signing key inside an HSM.

I could sign something that I hand out to a client and get back later, like in a JWT use case. If a user needed it on both ends, you could have the same key imported into multiple TPMs.

dengert · 2021-08-16T18:24:04Z

Keeping "the patch as small as possible" is a good goal, but I think there are some other issues and now would be the time to take care of them.

PKCS11 V2.40 and V3.0 have HMAC.

In PKCS11(v3.0) C_Sign can be called with CKM_SHA256_HMAC and a secret key. So they are overloading the term "Sign" to include "MAC'ing" And it looks like C_Verify" is overloaded with "Decrypt"

https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.pdf
See tables:
Table 105, General-length SH but there might ne A-1-HMAC: Key And Data Length
Uses C_Sign and C_Verify with "generic secret"

Table 109, SHA-256 Mechanisms vs. Functions (And other SHA-2)
use the term "generic secret keys"

Key Type CKK_GENERIC_SECRET is a subset of CKO_SECRET_KEY. So it might be possible to use an AES key i.e. CKK_AES as a CKK_GENERIC_SECRET but there might be truncation issues.

Another concern is the use of CKA_ID i.e. object_id for a secret key.
PKCS11 says "In the case of secret keys, the meaning of the CKA_ID attribute is up to the application.)
I.e. a secret key may not have a CKA_ID or it might match a CKA_ID used for private key, public key and certificate.

I would rewrite the code to base the find object code on the mechanism so each has a different error message,
and searched for secret key.

The output of a C_Derive can be a handle, which could be used with a HMAC operation. So searching for a secret key could be a problem.

pkcs11-tool does not support multipart instructions, i.e. derive a CKK_GENERIC_SECRET via ECDH, and use it with HMAC. Should be support this in pkcs11-tool? (The derived CKK_GENERIC_SECRET could be a session key on not on the token too.)

williamcroberts · 2021-08-16T19:00:49Z

Keeping "the patch as small as possible" is a good goal, but I think there are some other issues and now would be the time to take care of them.

PKCS11 V2.40 and V3.0 have HMAC.

In PKCS11(v3.0) C_Sign can be called with CKM_SHA256_HMAC and a secret key. So they are overloading the term "Sign" to include "MAC'ing" And it looks like C_Verify" is overloaded with "Decrypt"

https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.pdf
See tables:
Table 105, General-length SH but there might ne A-1-HMAC: Key And Data Length
Uses C_Sign and C_Verify with "generic secret"

Table 109, SHA-256 Mechanisms vs. Functions (And other SHA-2)
use the term "generic secret keys"

Key Type CKK_GENERIC_SECRET is a subset of CKO_SECRET_KEY. So it might be possible to use an AES key i.e. CKK_AES as a CKK_GENERIC_SECRET but there might be truncation issues.

Wouldn't this be implementation specific and require the CKM_HMAC_*_GENERAL functions? The normal ones should require a fixed size output equal to the length of the hash alg.

Another concern is the use of CKA_ID i.e. object_id for a secret key.
PKCS11 says "In the case of secret keys, the meaning of the CKA_ID attribute is up to the application.)
I.e. a secret key may not have a CKA_ID or it might match a CKA_ID used for private key, public key and certificate.

Yep and what happens if I have an HSM loaded with N>1 matching objects and types....Which one does the tool use? First one returned by C_FindObjects as the tool only asks for one.

I would rewrite the code to base the find object code on the mechanism so each has a different error message,
and searched for secret key.

The output of a C_Derive can be a handle, which could be used with a HMAC operation. So searching for a secret key could be a problem.

Crawl, walk run; Minimally Viable Product. Right now the goal is that C_Sign and C_Verify are exposed for the HMAC operations. If you have CKO_SECRET objects with a CKA_ID that collides, you're probably borked anyways.

pkcs11-tool does not support multipart instructions, i.e. derive a CKK_GENERIC_SECRET via ECDH, and use it with HMAC. Should be support this in pkcs11-tool? (The derived CKK_GENERIC_SECRET could be a session key on not on the token too.)

Don't need this support right now.

pkcs11-tool has a ton of limitations and issues, and I don't think fixing them all right now is really in the scope of just getting simple HMAC support.

williamcroberts · 2021-08-16T19:28:51Z

~~I'd also like to point out that sign is already supported, just not verify. So really V1 only added support for verify and has the same limitations as the current sign logic.~~

Disregard looking at the wrong branch.

dengert · 2021-08-17T00:26:58Z

Yep and what happens if I have an HSM loaded with N>1 matching objects and types....Which one does the tool use? First one returned by C_FindObjects as the tool only asks for one.

If you have CKO_SECRET objects with a CKA_ID that collides, you're probably borked anyways.

I disagree.

You patch calls find_object(session, CKO_PRIVATE_KEY,...) and if that fails calls find_object(session, CKO_SECRET_KEY,...) But there is no PKCS#11 requirement (I can find) that says a CKO_PRIVATE_KEY and a CKO_SECRET_KEY must not have the same CKA_ID. If some token does both with the same CKA_ID, your code will always find the CKO_PRIVATE_KEY, and never the CKO_SECRET_KEY.

Based on the mechanism requested, you could tell if you are looking for a CKO_PRIVATE_KEY or a CKO_SECRET_KEY and only look for one or the other.

P.S.

I am taking back my statement: "And it looks like C_Verify" is overloaded with "Decrypt". That is because HMAC does not encrypt or decrypt. One can derive or create keys by using HMAC, then these keys can be used with AES for example to encrypt and decrypt. See 2.9 HMAC mechanisms

williamcroberts · 2021-08-17T14:07:20Z

Yep and what happens if I have an HSM loaded with N>1 matching objects and types....Which one does the tool use? First one returned by C_FindObjects as the tool only asks for one.

If you have CKO_SECRET objects with a CKA_ID that collides, you're probably borked anyways.

I disagree.

You patch calls find_object(session, CKO_PRIVATE_KEY,...) and if that fails calls find_object(session, CKO_SECRET_KEY,...) But there is no PKCS#11 requirement (I can find) that says a CKO_PRIVATE_KEY and a CKO_SECRET_KEY must not have the same CKA_ID. If some token does both with the same CKA_ID, your code will always find the CKO_PRIVATE_KEY, and never the CKO_SECRET_KEY.

Based on the mechanism requested, you could tell if you are looking for a CKO_PRIVATE_KEY or a CKO_SECRET_KEY and only look for one or the other.

That's what I am saying, no requirement for it to match, no requirement for it not to match. Cryptoki doesn't require everything to match, its all intended and not enforced. The general paradigm is that CKA_ID will match amongst related public/private/certificate objects (this is in the spec as intended wording). Further, cryptoki doesn't even require their to not be duplicates. So to add a secret key that matches another CKA_ID, is probably bad form and you're stuff will break. We shouldn't really be doing all these checks in pkcs11-tool because to support use cases requires modifications to the tool, it prevents users from arbitrarily building up a call to C_Sign, which may be good to do the checks, but the Token can check and return error codes so why do them?

But in the spirit of community, ill go fix pkcs11-tool and give the mechanism design an overhaul so it will be easy to extend.

lgtm-com · 2021-08-17T17:41:14Z

This pull request introduces 1 alert when merging 83c7651 into 5582cd2 - view on LGTM.com

new alerts:

1 for Wrong type of arguments to formatting function

lgtm-com · 2021-08-17T18:34:05Z

This pull request introduces 1 alert when merging 4c86326 into 5582cd2 - view on LGTM.com

new alerts:

1 for Wrong type of arguments to formatting function

Before verification would only look for CKO_PUBLIC_KEY or CKO_PRIVATE_KEY objects, however, for HMAC to work through C_Verify it needs to look for CKO_SECRET_KEY objects as well. Before: tpm2pkcs11-tool --token-label=label --login --pin=myuserpin --verify --id='393837363534333231' --input-file=data.msg --mechanism=SHA-1-HMAC --signature-file=data.sig error: Public key nor certificate not found Aborting. After: tpm2pkcs11-tool --token-label=label --login --pin=myuserpin --verify --id='393837363534333231' --input-file=data.msg --mechanism=SHA-1-HMAC --signature-file=data.sig Using signature algorithm SHA-1-HMAC Signature is valid To help promote extensibility in the future, various bits of meta data can be associated with a mechanism via the mf_info table. The new bits of metadata are MF_FLAGS and contain information to quickly acertain what type of CKA_CLASS or C_.* interfaces are supported by the mechanism. This table can be slowly populated over time so support for other mechanisms can be added and the old code paths can be removed. Signed-off-by: William Roberts <[email protected]>

This tests that a Token with a single CKO_SECRET_KEY can be used in HMAC for SHA1, SHA256, SHA284 and SHA512 hash versions. Since pkcs11 tool does not support --id setting on CKO_SECRET_KEY nor respects the --label option for C_Sign and C_Verify, one may not get the key they expect. Signed-off-by: William Roberts <[email protected]>

Jakuje

looks generally good

Jakuje · 2021-08-18T07:56:03Z

src/tools/pkcs11-tool.c

+      { CKG_MGF1_SHA256,	"MGF1-SHA256", NULL, MF_MGF },
+      { CKG_MGF1_SHA384,	"MGF1-SHA384", NULL, MF_MGF },
+      { CKG_MGF1_SHA512,	"MGF1-SHA512", NULL, MF_MGF },
+      { 0, NULL, NULL, MF_UNKNOWN }


Is modification of this list needed when the flag is not used anywhere? The MF_MGF is not used anywhere else either. for PKCS#11, the MGF is used only for the RSA-PSS and OAEP padding so it does not go through your filtering if I am right.

True, not used, we can drop that. It was just low hanging fruit to fill in over leaving it as MF_UNKNOWN.

Jakuje · 2021-08-18T08:22:31Z

src/tools/pkcs11-tool.c

+      { CKM_DH_PKCS_PARAMETER_GEN,"DH-PKCS-PARAMETER-GEN", NULL, MF_UNKNOWN },
+      { CKM_X9_42_DH_PARAMETER_GEN,"X9-42-DH-PARAMETER-GEN", NULL, MF_UNKNOWN },
+      { CKM_AES_KEY_WRAP,	"AES-KEY-WRAP", NULL, MF_UNKNOWN },
+      { 0, NULL, NULL, MF_UNKNOWN }


The MF_UNKNOWN is 0 so if we would skip this initializer, it would be the same, isn't it? It would also make it clear what entry is already filled in.

I'm not getting your suggestion here, are you saying you want:

{ 0, NULL, NULL, 0 }

The existing code has this as a NULL terminated array, not sure it really needs to be NULL term since size is known.

This was comment for the whole block, not only for the terminator. My suggestion was to drop the MF_UNKNOWN from all the "incomplete" entries that are not yet filled out with sensible flags. But on second though, I am not sure if it would be better.

This was comment for the whole block, not only for the terminator. My suggestion was to drop the MF_UNKNOWN from all the "incomplete" entries that are not yet filled out with sensible flags. But on second though, I am not sure if it would be better.

You have to put an initializer there do to the default compiler flags (-Werror=missing-field-initializers):

pkcs11-tool.c:7247:7: error: missing initializer for field ‘mf_flags’ of ‘struct mech_info’ [-Werror=missing-field-initializers] 7247 | { CKM_AES_KEY_WRAP, "AES-KEY-WRAP", NULL },

So I thought something descriptive, like UNKNOWN would be better than a 0.

Oh, right. My bad. So then it is fine, I guess.

Jakuje · 2021-08-24T09:50:04Z

Thank you for your contribution!

Jakuje reviewed Aug 16, 2021

View reviewed changes

dengert reviewed Aug 16, 2021

View reviewed changes

williamcroberts force-pushed the support-hmac-signatures branch from 7cb9e54 to 83c7651 Compare August 17, 2021 17:27

williamcroberts force-pushed the support-hmac-signatures branch from 4c86326 to a771ac1 Compare August 17, 2021 18:57

williamcroberts force-pushed the support-hmac-signatures branch from a771ac1 to 40d34aa Compare August 17, 2021 19:23

Jakuje approved these changes Aug 18, 2021

View reviewed changes

Jakuje merged commit d102449 into OpenSC:master Aug 24, 2021

xhanulik mentioned this pull request Jul 12, 2022

Prepare for release 0.23.0 #2426

Closed

Support HMAC signatures and verification #2385

Support HMAC signatures and verification #2385

Uh oh!

Conversation

williamcroberts commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williamcroberts Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dengert left a comment

Choose a reason for hiding this comment

Uh oh!

mouse07410 commented Aug 16, 2021

Uh oh!

williamcroberts commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dengert commented Aug 16, 2021

Uh oh!

williamcroberts commented Aug 16, 2021

Uh oh!

williamcroberts commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dengert commented Aug 17, 2021

Uh oh!

williamcroberts commented Aug 17, 2021

Uh oh!

lgtm-com bot commented Aug 17, 2021

Uh oh!

lgtm-com bot commented Aug 17, 2021

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakuje commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

williamcroberts commented Aug 13, 2021 •

edited

Loading

williamcroberts Aug 16, 2021 •

edited

Loading

williamcroberts commented Aug 16, 2021 •

edited

Loading

williamcroberts commented Aug 16, 2021 •

edited

Loading