Implement C_CreateObject for EC keys #2420

llogar · 2021-10-17T18:49:59Z

Most of the code was already there, only a little glue code was needed...

Checklist

Documentation is added or updated
New files have a LGPL 2.1 license statement
PKCS#11 module is tested
Windows minidriver is tested
macOS tokend is tested

Jakuje · 2021-10-18T10:00:56Z

What card did you use to test the functionality?

llogar · 2021-10-18T10:12:16Z

A somewhat modified version of IsoApplet, but I think it should work with the stock one too...

Jakuje · 2021-10-18T10:21:42Z

We have already couple of tests with isoapplet in https://github.com/OpenSC/OpenSC/blob/master/.github/test-isoapplet.sh#L59 using pkcs15-tool.

Can you extend it with the test for ec key generation through pkcs11 tool if it will work?

llogar · 2021-10-18T10:27:05Z

Sure, will do, both for key generation via the C_GenerateKeyPair() and key "import" via the C_CreateObject() (altough I think the key import in IsoApplet is disabled by default)?

llogar · 2021-10-19T08:19:33Z

I've tried to prepare some tests with the IsoApplet. There are several issues:

As the simulated card ATR is a generic one 3B80800101, it is recognized as one of the SC-HSM cards and marked as readonly, so the C_CreateObject() fails with CKR_TOKEN_WRITE_PROTECTED. I've noticed that already in my PR Fix C_Login() crashing when using uninitialized card #2045 and suggested a change (alas for a different reason), but was rejected. In order to be able to use C_CreateObject() opensc.conf (or ATR) has to be changed...
Stock IsoApplet has key import disabled by default. It should be enabled (by setting DEF_PRIVATE_KEY_IMPORT_ALLOWED = true) and applet recomplied for a C_CreateObject() to succeed.
It seems that the stock IsoApplet requires EC_POINT_NO_ASN1_OCTET_STRING workaround, so pkcs11-tool should be changed & recompiled...
Stock IsoApplet does not support raw CKM_ECDSA signing, only CKM_ECDSA_SHA256, so I don't think much can be tested either with the pkcs11-tool and/or openssl pkcs11 engine? The best I could come up with is
- key generation, and signing
  pkcs11-tool --keypairgen --key-type EC:brainpoolP256r1 ...
  pkcs11-tool --sign --mechanism ECDSA-SHA256 ...
- key import and signing
  openssl ecparam -name brainpoolP256r1 -genkey -noout -out ec_priv01.pem
  openssl ec -in private.ec.key -pubout -out ec_pub01.pem
  pkcs11-tool --write-object ec_priv01.pem --type privkey ...
  pkcs11-tool --write-object ec_pub01.pem --type pubkey ...
  pkcs11-tool --sign --mechanism ECDSA-SHA256 ...
but I couldn't find a simple way to verify the raw signature that is produced with the pkcs11-tool --sign

dengert · 2021-10-19T13:39:23Z

Do you have a debug log for point 1 "recognized as one of the SC-HSM cards"?

3B80800101 is the simplest contactless card, i.e. ISO 14443 Type B and card-sc-hsm.c tries to select the SC-SC_HSM AID to verify it is a SC-HSM card.

https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-sc-hsm.c#L250-L258 test for https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-sc-hsm.c#L70
and returns something other then "1", so https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card.c#L353-L354 should continue to search for other drivers.

Other drivers may also look at this. So it is not clear without a debug log where a read only bit might have been set.

dengert · 2021-10-19T14:01:42Z

With regard to point 4 "Stock IsoApplet does not support raw CKM_ECDSA signing, only CKM_ECDSA_SHA256"

https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-isoApplet.c#L237-L238
claims card supports ECDSA, and ECDSA_SHA1 both on the card.

You may want to try replacing these 2 lines with flags |= SC_ALGORITHM_ECDSA_HASH_SHA256;
which says the card can only support ECDSA with SHA256 on the card.

If isoApplet is modified to support ECDSA with hash done in software, then add flags |= SC_ALGORITHM_ECDSA_RAW;

llogar · 2021-10-19T19:10:38Z

Other drivers may also look at this. So it is not clear without a debug log where a read only bit might have been set.

Well, It almost certainly is set in the function pkcs15_init_slot():

 	atrblock = _sc_match_atr_block(p15card->card->ctx, NULL, &p15card->card->atr);
 	if (atrblock) {
 		write_protected = scconf_get_bool(atrblock, "read_only", write_protected);
 	}
 	if (write_protected) {
 		slot->token_info.flags |= CKF_WRITE_PROTECTED;
 	}

I can certainly attach a debug log, but it will show nothing. Both of the logs are identical apart from the line
[opensc-pkcs11] framework-pkcs15.c:582:C_GetTokenInfo: C_GetTokenInfo() auth. object 0x55626096d2c0, token-info flags 0x40F
and
[opensc-pkcs11] framework-pkcs15.c:582:C_GetTokenInfo: C_GetTokenInfo() auth. object 0x561b2051e2c0, token-info flags 0x40D

We are getting slightly off-topic here, as this PR is about the C_CreateObject support for CKK_EC keys, which is not specific to IsoApplet or related to CKF_WRITE_PROTECTED issue...

llogar · 2021-10-19T20:20:39Z

With regard to point 4 "Stock IsoApplet does not support raw CKM_ECDSA signing, only CKM_ECDSA_SHA256"

https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-isoApplet.c#L237-L238 claims card supports ECDSA, and ECDSA_SHA1 both on the card.

You may want to try replacing these 2 lines with flags |= SC_ALGORITHM_ECDSA_HASH_SHA256; which says the card can only support ECDSA with SHA256 on the card.

If isoApplet is modified to support ECDSA with hash done in software, then add flags |= SC_ALGORITHM_ECDSA_RAW;

Sorry, my mistake. Yes, it only supports ECDSA_SHA1, not ECDSA_SHA256 (and also not plain ECDSA). IsoApplet was written for Javacard 2.2.2 which supports ALG_ECDSA_SHA only. So hashing has to be done on-card, which severly limits the size of the data that can be signed.

Furthermore I belive there is a bug in card-isoApplet.c, which currently breaks isoApplet ECDSA signing:

--- a/src/libopensc/card-isoApplet.c
+++ b/src/libopensc/card-isoApplet.c
@@ -234,7 +234,6 @@ isoApplet_init(sc_card_t *card)
                 * should be kept in sync with the explicit parameters in the pkcs15-init
                 * driver. */
                flags = 0;
-               flags |= SC_ALGORITHM_ECDSA_RAW;
                flags |= SC_ALGORITHM_ECDSA_HASH_SHA1;
                flags |= SC_ALGORITHM_ONBOARD_KEY_GEN;
                ext_flags = SC_ALGORITHM_EXT_EC_UNCOMPRESES;
@@ -1116,14 +1115,14 @@ isoApplet_set_security_env(sc_card_t *card,
                        break;
 
                case SC_ALGORITHM_EC:
-                       if( env->algorithm_flags & SC_ALGORITHM_ECDSA_RAW )
+                       if( env->algorithm_flags & SC_ALGORITHM_ECDSA_HASH_SHA1 )
                        {
                                drvdata->sec_env_alg_ref = ISOAPPLET_ALG_REF_ECDSA;
                                drvdata->sec_env_ec_field_length = env->algorithm_ref;
                        }
                        else
                        {
-                               LOG_TEST_RET(card->ctx, SC_ERROR_NOT_SUPPORTED, "IsoApplet only supports raw ECDSA.");
+                               LOG_TEST_RET(card->ctx, SC_ERROR_NOT_SUPPORTED, "IsoApplet only supports ECDSA with on-card SHA1.");
                        }
                        break;

I will make a separate PR for this...

dengert · 2021-10-19T21:13:02Z

You said: "but I couldn't find a simple way to verify the raw signature that is produced with the pkcs11-tool --sign." Have you looked at option: "--signature-format Format for ECDSA signature : 'rs' (default), 'sequence', 'openssl'

One of my tests uses:

openssl sha1 -binary -out /tmp/test.hash < $2

pkcs11-tool --slot 1 --module $OPENSC_PATH/opensc-pkcs11.so \
 -v \
 --sign -m $MECH --id $1 -i /tmp/test.hash \
 --signature-format openssl \
 -o /tmp/test.signature

pkcs15-tool --read-public-key $1 -o /tmp/pubkey
openssl dgst -sha1 -verify /tmp/pubkey -signature /tmp/test.signature $2

Jakuje · 2021-10-20T09:54:32Z

I've tried to prepare some tests with the IsoApplet. There are several issues:

As the simulated card ATR is a generic one 3B80800101, it is recognized as one of the SC-HSM cards and marked as readonly, so the C_CreateObject() fails with CKR_TOKEN_WRITE_PROTECTED. I've noticed that already in my PR Fix C_Login() crashing when using uninitialized card #2045 and suggested a change (alas for a different reason), but was rejected. In order to be able to use C_CreateObject() opensc.conf (or ATR) has to be changed...

I think this change was not rejected but implemented by Frankn in a more systematic way.

Would using the OPNESC_DRIVER=isoapplet environment variable address the detection issue too?

Stock IsoApplet has key import disabled by default. It should be enabled (by setting DEF_PRIVATE_KEY_IMPORT_ALLOWED = true) and applet recomplied for a C_CreateObject() to succeed.

Yeah, we compile the isoapplet as part of the ci so modification of this is fine for testing.

It seems that the stock IsoApplet requires EC_POINT_NO_ASN1_OCTET_STRING workaround, so pkcs11-tool should be changed & recompiled...

Hmm ... this is stupid. I thought we had this workaround only for reading of the ec keys, but apparently also for writing. it would be great if this could be handled somehow without recompiling. Either with some command-line switch or better with some compat flags. I can think of CKF_VENDOR_DEFINED flag in the CK_SESSION_INFO structure or custom profile object from PKCS#11 3.0 signalized by the card driver.

Stock IsoApplet does not support raw CKM_ECDSA signing, only CKM_ECDSA_SHA256, so I don't think much can be tested either with the pkcs11-tool and/or openssl pkcs11 engine? The best I could come up with is

key generation, and signing
pkcs11-tool --keypairgen --key-type EC:brainpoolP256r1 ...
pkcs11-tool --sign --mechanism ECDSA-SHA256 ...

key import and signing
openssl ecparam -name brainpoolP256r1 -genkey -noout -out ec_priv01.pem
openssl ec -in private.ec.key -pubout -out ec_pub01.pem
pkcs11-tool --write-object ec_priv01.pem --type privkey ...
pkcs11-tool --write-object ec_pub01.pem --type pubkey ...
pkcs11-tool --sign --mechanism ECDSA-SHA256 ...

but I couldn't find a simple way to verify the raw signature that is produced with the pkcs11-tool --sign

I think this is certainly better than nothing. I think the --signature-format option proposed by Doug should do the job to verify the signatures.

So if this would work, it would be great. If some non-trivial amount of work would be needed to get through some hops, I am happy to accept the change as it is now.

Jakuje · 2021-10-20T10:05:09Z

It seems that the stock IsoApplet requires EC_POINT_NO_ASN1_OCTET_STRING workaround, so pkcs11-tool should be changed & recompiled...

Hmm ... this is stupid. I thought we had this workaround only for reading of the ec keys, but apparently also for writing. it would be great if this could be handled somehow without recompiling. Either with some command-line switch or better with some compat flags. I can think of CKF_VENDOR_DEFINED flag in the CK_SESSION_INFO structure or custom profile object from PKCS#11 3.0 signalized by the card driver.

I am actually not sure what is the specification for this in the PKCS#15 layer. The PKCS#11 says:

CKA_EC_POINT1,4 | Byte array | DER-encoding of ANSI X9.62 ECPoint value Q

so if isoapplet applet requires stripped value, it can actually be handled inside of the isoapplet driver, keeping the PKCS#11 specification intact, isn't it?

dengert · 2021-10-20T12:18:53Z

An example of converting from EC signature read from card in a driver: https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-piv.c#L2411-L2461

llogar · 2021-10-20T19:45:15Z

You said: "but I couldn't find a simple way to verify the raw signature that is produced with the pkcs11-tool --sign." Have you looked at option: "--signature-format Format for ECDSA signature : 'rs' (default), 'sequence', 'openssl'

Thanks for the tip.
I've add an ECDSA_SHAxxx support to pkcs11-tool --verify ... in this PR, so the signatures can be verified even without 3rd party tools. An ECDSA signature and verification test was also added to IsoApplet test suite (both for on-card generated and imported ECC keys)

llogar · 2021-10-20T19:57:33Z

It seems that the stock IsoApplet requires EC_POINT_NO_ASN1_OCTET_STRING workaround, so pkcs11-tool should be changed & recompiled...

Hmm ... this is stupid. I thought we had this workaround only for reading of the ec keys, but apparently also for writing. it would be great if this could be handled somehow without recompiling. Either with some command-line switch or better with some compat flags. I can think of CKF_VENDOR_DEFINED flag in the CK_SESSION_INFO structure or custom profile object from PKCS#11 3.0 signalized by the card driver.

Hmm, EC_POINT_NO_ASN1_OCTET_STRING workaround was used for writing objects only? It had no influence on reading objects? In any case, I've "replaced" it with pkcs11-tool --no-asn1-encoding ... option. So IsoApplet test suite could be expanded with some more ECDSA test cases...

llogar · 2021-10-20T20:00:09Z

so if isoapplet applet requires stripped value, it can actually be handled inside of the isoapplet driver, keeping the PKCS#11 specification intact, isn't it?

I'll have to check how the card-piv code does it (from dengert's comment above), as at first sight I don't see any public key handling code in IsoApplet driver.

llogar · 2021-10-20T20:05:16Z

I've added 3 more commits to this PR, so the ECDSA key import, signing and verification could be added to PKCS#11 IsoApplet test suite.

dengert · 2021-10-20T20:34:43Z

It looks like d46a539 is trying to replace a compile option with a command line option.

Do you have a specific card that has a problem with EC keys?

It looks like the `#ifndef EC_POINT_NO_ASN1_OCTET_STRING // workaround for non-compliant cards not expecting DER encoding" was added when parse_ec_pkey was added by copying parts of parse_gost_key by 4441efa then ea4baf5 changed "#ifndef" to "#ifdef" it does not look like it should ever been added at all.

PKCS11 3.0 says for GOST pubkeys:
"GOST CKA_VALUE 1,4 |Byte array | 64 bytes for public key; 32 bytes for each coordinates X and Y of elliptic curve point P(X, Y) in little endian order"

PKCS11 3.0 says for EC pubkeys:
"CKA_EC_PARAMS 1,3 | Byte array | DER-encoding of an ANSI X9.62 Parameters value"
"CKA_EC_POINT 1,4 | Byte array DER-encoding of ANSI X9.62 ECPoint value Q"

I am questioning if "EC_POINT_NO_ASN1_OCTET_STRING" was really ever needed, as only GOST keys do not use the ANS1 encoding.

In any case, pkcs11-tool is not the place to fix this, it should be fixed in card driver or pkcs11 module.

dengert · 2021-10-20T21:09:29Z

In addition, PKCS11 3.0 says "Edwards Elliptic curve public key objects":
"Byte array | DER-encoding of a Parameters value as defined above"
"CKA_EC_POINT 1,4 | Byte array | DER-encoding of the b-bit public key value in little endian order as defined in RFC 8032"
"Montgomery EC public key":
"CKA_EC_PARAMS 1,3 | Byte array | DER-encoding of a Parameters value as defined above"
"CKA_EC_POINT 1,4 | Byte array | DER-encoding of the public key value in little endian order as defined in RFC 7748"

So the format of EC keys depends on the type of key, and parse_ec_pkey should be expanded to handle this. or each type of key should have its own parse routine.

llogar · 2021-10-21T07:24:46Z

In any case, pkcs11-tool is not the place to fix this, it should be fixed in card driver or pkcs11 module.

You are right, I was misled by the mere existance of EC_POINT_NO_ASN1_OCTET_STRING and the comment
in sc_pkcs15_pubkey_ec definition saying it's value is not DER encoded.
I've moved EC public key asn1 decoding to CKA_EC_POINT parsing and omited any changes to pkcs11-tool

.github/test-isoapplet.sh

src/pkcs11/openssl.c

Most of the code was already there, only a little glue code was needed...

src/pkcs11/openssl.c

Cards that support CKM_ECDSA_SHAx only don't do the hashing in C_VerifyUpdate(). All the processing is done in the final C_VerifyFinal() call instead.

and include some more ECDSA tests in IsoApplet test suite

frankmorgner

I think it looks good now

@philipWendland, do you want to have a look?

frankmorgner · 2021-10-27T10:19:35Z

src/pkcs11/openssl.c

+		    || mech->mechanism == CKM_ECDSA_SHA224
+		    || mech->mechanism == CKM_ECDSA_SHA256
+		    || mech->mechanism == CKM_ECDSA_SHA384
+		    || mech->mechanism == CKM_ECDSA_SHA512)) {


I stumbled across this, because the indenting looks bad. I think the conditions need to be moved one block to the right. Also, else if (...) { looks coherent, but since you've added a longer comment this now looks broken. I suggest you add additional {} paranthesis around the else block even though it is not strictly needed.

We still do not have finalized the coding style in #2017 so I still consider this a minor issue. Currently, I think we are aiming for tab alignment to the start of the block and space alignment to the opening brace, keeping the || on the end of the last line so I think it should look like the following, if I am right (if it would be simple if condition):

if (md == NULL && (mech->mechanism == CKM_ECDSA || mech->mechanism == CKM_ECDSA_SHA1 || mech->mechanism == CKM_ECDSA_SHA224 || mech->mechanism == CKM_ECDSA_SHA256 || mech->mechanism == CKM_ECDSA_SHA384 || mech->mechanism == CKM_ECDSA_SHA512)) {

Given that this is else/if condition, I would keep the comment on separate line before the } else .. line and drop one of the tabs in the indentation of the condition lines.

So, do I change formatting or not, and how? Altough it would look odd to change just the formatting of the new code and leave the existing code (even in the same function) the same...

I am fine with it as it is. We are still counting with some mass change of formatting in case we will settle down on one format ;)

llogar force-pushed the createEC branch from 779aa68 to 72ce1b1 Compare October 21, 2021 07:19

Jakuje reviewed Oct 21, 2021

View reviewed changes

.github/test-isoapplet.sh Outdated Show resolved Hide resolved

src/pkcs11/openssl.c Outdated Show resolved Hide resolved

src/pkcs11/openssl.c Outdated Show resolved Hide resolved

Implement C_CreateObject() for EC keys

046c30f

Most of the code was already there, only a little glue code was needed...

llogar force-pushed the createEC branch from 72ce1b1 to 5c70206 Compare October 22, 2021 07:56

Jakuje reviewed Oct 26, 2021

View reviewed changes

src/pkcs11/openssl.c Show resolved Hide resolved

src/pkcs11/openssl.c Show resolved Hide resolved

src/pkcs11/openssl.c Show resolved Hide resolved

Luka Logar added 2 commits October 27, 2021 10:25

Fix signature verification for CKM_ECDSA_SHAx only cards

d360ec4

Cards that support CKM_ECDSA_SHAx only don't do the hashing in C_VerifyUpdate(). All the processing is done in the final C_VerifyFinal() call instead.

Fix IsoApplet CKM_ECDSA_SHA1 implementation

f5cb10c

and include some more ECDSA tests in IsoApplet test suite

llogar force-pushed the createEC branch from 5c70206 to f5cb10c Compare October 27, 2021 08:36

Jakuje approved these changes Oct 27, 2021

View reviewed changes

frankmorgner approved these changes Oct 27, 2021

View reviewed changes

Jakuje merged commit 88eeb89 into OpenSC:master Nov 19, 2021

xhanulik mentioned this pull request Jul 12, 2022

Prepare for release 0.23.0 #2426

Closed

llogar deleted the createEC branch November 14, 2023 21:02

Implement C_CreateObject for EC keys #2420

Implement C_CreateObject for EC keys #2420

Uh oh!

Conversation

llogar commented Oct 17, 2021

Checklist

Uh oh!

Jakuje commented Oct 18, 2021

Uh oh!

llogar commented Oct 18, 2021

Uh oh!

Jakuje commented Oct 18, 2021

Uh oh!

llogar commented Oct 18, 2021

Uh oh!

llogar commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dengert commented Oct 19, 2021

Uh oh!

dengert commented Oct 19, 2021

Uh oh!

llogar commented Oct 19, 2021

Uh oh!

llogar commented Oct 19, 2021

Uh oh!

dengert commented Oct 19, 2021

Uh oh!

Jakuje commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakuje commented Oct 20, 2021

Uh oh!

dengert commented Oct 20, 2021

Uh oh!

llogar commented Oct 20, 2021

Uh oh!

llogar commented Oct 20, 2021

Uh oh!

llogar commented Oct 20, 2021

Uh oh!

llogar commented Oct 20, 2021

Uh oh!

dengert commented Oct 20, 2021

Uh oh!

dengert commented Oct 20, 2021

Uh oh!

llogar commented Oct 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankmorgner left a comment

Choose a reason for hiding this comment

Uh oh!

frankmorgner Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

Jakuje Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

llogar Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

Jakuje Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llogar commented Oct 19, 2021 •

edited

Loading

Jakuje commented Oct 20, 2021 •

edited

Loading