isoapplet offcard hash #2642

swissbit-csteuer · 2022-11-14T13:39:22Z

Hi,

currently the IsoApplet and OpenSC only support ECDSA-SHA1 because the hash is computed on-card.
This pull request adds support for ECDSA, ECDSA-SHA224, ECDSA-SHA256, ECDSA-SHA384, ECDSA-SHA512 by computing the hash off-card.

The IsoApplet needs to be changed: philipWendland/IsoApplet@master...swissbit-eis:IsoApplet:master

jcardsim also requires some changes to support raw ECDSA: swissbit-eis/jcardsim@00e646a

The changes have been tested with the following card: Swissbit Secure USB PU-50n SE/PE (99000010EF000279) 01 00

Result of p11test
Result of pkcs11-tool --test

Checklist

Documentation is added or updated
PKCS#11 module is tested

swissbit-csteuer · 2022-11-16T08:46:34Z

Hi @philipWendland , since this PR also affects the ISOApplet, could you take a look at it?

frankmorgner · 2022-11-17T11:09:55Z

In your PR you're replacing on-card-hashing with off-card-hashing. Would it be possible to support both?

Additionally, I suggest you open a pull request with your changes at the ISOApplet repository so it can be reviewed...

swissbit-csteuer · 2022-11-17T12:46:50Z

In your PR you're replacing on-card-hashing with off-card-hashing. Would it be possible to support both?

I guess we could check the version number of the ISOApplet and use SHA1 on-card-hashing for old versions and off-card-hashing for the new ISOApplet version.

Additionally, I suggest you open a pull request with your changes at the ISOApplet repository so it can be reviewed...

Done.

philipWendland · 2022-11-19T17:09:03Z

Hi,
thank you for the contribution.

Currently, the change of the IsoApplet major version number in the card-isoApplet.c breaks all existing deployments because the driver refuses to work with older applet versions (see here). Hence, this PR should currently not be merged.
However, I think we can make this work.

ECC with off-card hashes requires JavaCard 3+ and I want to keep supporting v2.2.2 cards with IsoApplet.
It is likely necessary for me to maintain a "legacy" branch of the IsoApplet for JavaCards v2.2.2 (without off-card-ECC support) and one for newer (v3+) cards. I will report back here once the IsoApplet-side is ready.

philipWendland · 2022-11-19T20:48:58Z

Could you please explain the one-line change in pkcs11-tool to me?

swissbit-csteuer · 2022-11-22T17:01:18Z

Could you please explain the one-line change in pkcs11-tool to me?

This was necessary to compute raw ECDSA signatures with the pkcs11-tool like its defined in the PKCS#11 spec (2.3.6 ECDSA without hashing):

Input the entire raw digest. Internally, this will be truncated to the appropriate number of bits.

popovec · 2022-11-22T19:53:33Z

In my opinion, it is not necessary to change anything in the pkcs11-tool.c code. pkcs11-toll.c code for ECDSA (raw) is fully functional and is tested by github actions on the OsEID simulation.

Testing is done by calling pkcs11-tool approximately as follows:

pkcs11-tool --sign -m ECDSA --signature-format "sequence" --input-file tmp/testfile.txt --output-file tmp/testfile.txt.pkcs11.sig --id $keyID

Result is verified both with pkcs11-tool and with openssl

Example: if we calculate ECDSA (without applying the hash function) for secp521r1, the input string is exactly 66 bytes long -> r=66, sizeof(in_buffer) = 1025 . The condition r < sizeof(in_buffer) will be met. Any test for mechanism makes no sense here.

dengert · 2022-11-22T20:31:03Z

I believe the change is needed.

2.3.6 says: "(This mechanism corresponds only to the part of ECDSA that processes the hash value, which should not be longer than 1024 bits; it does not compute the hash value.)" Which says: "should".

If the input is larger the 1024 bytes, the size of the input buffer + 1, the operation will fail in the pkcs11-tool. PKCS11 also says in
2.3.6 foot note 3 "Input the entire raw digest. Internally, this will be truncated to the appropriate number of bits."

So pkcs11-tool is making some assumptions on the max size of the input data i.e. < 1024 bytes but that that should be up to the PKCS11 module to do the truncation based on the size of the key.

One could argue that there are no ECDSA keys or hashes today larger the 1024 bytes, and hashes and key are measured in bits.

popovec · 2022-11-23T06:20:17Z

For r=1025 and sizeof(in_buffer) = 1025 the condition is false and operation preserves the value rv=CKR_CANCEL. In this case operation continues using new C_SignInit() and several calls to C_SignUpdate(). Result is then retrieved by C_SignFinal() . The ECDSA (raw) operation should also work correctly when using C_Sign, but also when using several C_SignUpdate calls.

The only reason for introducing the mechanism test is that truncation will be performed in pkcs11-tool and not at the pkcs11 module level.

It will be useful to check how the OpenSC pkcs11 module behaves in this situation and possibly compare it with how other pkcs11 implementations behave in this situation, for example softhsm...

swissbit-csteuer · 2022-11-23T09:52:57Z

@popovec is right. I just tested it without the change and the tool still produces valid signatures. I thought that it did not work when we tested it some time ago but seems like that was a mistake. I have removed the change in pkcs11-tool.c.

.github/test-isoapplet.sh

src/tests/p11test/isoapplet.json

Jakuje · 2022-12-08T11:14:31Z

src/libopensc/card-isoApplet.c

-			memcpy(out, p, len);
+			memcpy(seqbuf, p, len);
 			r = len;
 		}

 		free(p);
 	}
+	if ((size_t) r > outlen)
+		LOG_FUNC_RETURN(ctx, SC_ERROR_BUFFER_TOO_SMALL);
+	memcpy(out, seqbuf, r);


I am wondering if we need these two memcpy. It will probably not be large overhead, but copying data back and forth might be at least confusing and error prone.

Since we added an additional branch for RSA-PSS I refactored the code to handle each case (RSA, RSA-PSS and ECDSA) in its own branch. We only need seqbuf in the ECDSA branch and now we do not need additional memcpy's.

src/libopensc/card-isoApplet.c

swissbit-csteuer · 2022-12-08T17:35:10Z

I added some more changes to enable RSA 4096 support with the new v1 version of the IsoApplet. Now every new function of the v1 applet should be covered. See philipWendland/IsoApplet#28

philipWendland · 2022-12-10T12:30:06Z

My plan is to review and test those changes carefully (also the changes to IsoApplet). Depending on the effort, it might take me until the end of this month.

Then, the main branch of IsoApplet will be Version 1.0.0 of the applet, for JavaCards >= 3.0.4, with the following new features:

RSA 4096
RSA PSS
ECDSA off-card hashes

There will be a (maintained) legacy branch of IsoApplet, so that JavaCards v.2.2.2 are still supported. Both versions of the IsoApplet will be supported by the OpenSC driver.

I will report back here as soon as my tests are done and the changes to IsoApplet are ready to be merged.

philipWendland · 2023-01-02T16:40:44Z

From my point of view, this PR can be merged.
I will update the IsoApplet main branch as soon as this PR is merged.

Jakuje

looks good. I put there one minor comment, but we can live without that. Please rebase on top of current master to get rid of the CI failures too.

.github/test-isoapplet.sh

swissbit-csteuer · 2023-01-12T16:51:18Z

looks good. I put there one minor comment, but we can live without that. Please rebase on top of current master to get rid of the CI failures too.

Thanks for the review.

I have rebased onto the current master.
The test-isoapplet.sh script now fails when using the v1 branch.
I'm trying to fix it but can't reproduce it locally... so it might take a bit longer.

Jakuje · 2023-01-12T19:05:26Z

The test-isoapplet.sh script now fails when using the v1 branch.

is it deterministic in ci? Did you try to rerun it? Some tests sometimes fail for no reason. There is already too many moving parts with java, jcardsim, ...

swissbit-csteuer · 2023-01-13T11:48:42Z

Yes, it's deterministic and I can now reproduce it locally.

Before, I used a different opensc.conf where I forced the maximum send and receive size for pcsc to 400 bytes as a workaround for an issue with my test hardware.
When I do not force the maximum, send and receive size, I get the same problem with the simulator. This is what happens:

test-isoapplet.sh calls pkcs11-tool --login --test ...
One of the tests calls iso7816_decipher with in-len 256 and out-len 512
iso7816_decipher calls fixup_transceive_length which sets apdu->flags |= SC_APDU_FLAGS_CHAINING because sc_get_max_send_size returns 255 but we want to send 256 byte
sc_transmit_apdu sends a command chaining APDU (CLA = 0x10)
The IsoApplet v1 rejects that APDU with 0x6884 (SW_COMMAND_CHAINING_NOT_SUPPORTED).

The problem is that sc_get_max_send_size returns 255, so that OpenSC cannot send an extended length APDU and falls back to command chaining, which the new IsoApplet does not support.

The virtual smartcard ifd handler (ifd-vpcd) does not support IFDHControl. IFDHControll is called by SCardControl, which in turn is called by OpenSC to detect the reader properties. Since that fails, OpenSC assumes that the reader supports only short APDUs where the maximum send size is 255.

I'm not sure about the best way to resolve this issue.
The easiest fix is forcing max send/recv size to 65536 with a custom
OpenSC config when testing v1 IsoApplet with the javacard simulator.

What do you think @Jakuje ?

In previous versions of IsoApplet, the applet returned the applet version and feature bitmap upon selection. However, this special select command leads to problems with cards that were until now not tested. In future versions of IsoApplet, the behavior will change: - The Select APDU is a normal case 3 apdu - Applet info is obtained using a seperate GET DATA apdu.

Use IsoApplet version 1.0 which no longer computes the hash on card. The previous version of the IsoApplet only supported ECDSA-SHA1. With supports also ECDSA, ECDSA-SHA224, ECDSA-SHA256, ECDSA-SHA384, ECDSA-SHA512. Co-authored-by: Claus Steuer <[email protected]>

- set correct request and response buffer length for extended APDUs

- adapt to change in iso applet that only signal PSS support if all hash schemes are supported - strip PKCS#1 digest info when using RSA PSS so that only the hash is sent to the applet

swissbit-csteuer · 2023-01-19T10:56:20Z

The IsoApplet CI tests are now running without issues.
I rebased onto the current master and updated the reference JSON (+RSA 4096, +PKCS_PSS).

The last commit also changes the order in which the test result JSON files are passed to the diff command. Now the reference/expected file is the FROM file and the actual file the TO file. If a line is missing in the actual file, it's marked with '-' in the diff output instead of '+' and vice versa.

The only CI task that fails is the AppVeyor build, but the failure does not seem the be related to changes in this PR.

swissbit-csteuer · 2023-01-23T11:49:53Z

Is there more that needs to be done before this can be merged?

Jakuje · 2023-01-23T14:42:32Z

I think we are good to merge, unless there would be some more comments from @frankmorgner . If not, I will try to merge the PR later this week.

philipWendland · 2023-01-27T16:22:47Z

Hi Jakuje,
thank you for merging. I updated the main branch of IsoApplet as a consequence.

This means that the test scripts need to be updated, please see #2688

swissbit-csteuer marked this pull request as draft November 14, 2022 13:40

swissbit-csteuer marked this pull request as ready for review November 16, 2022 08:42

This was referenced Nov 17, 2022

Always use ECDSA with off card hashing philipWendland/IsoApplet#28

Closed

Add support for raw ecdsa without hashing Jakuje/jcardsim#1

Merged

swissbit-csteuer force-pushed the 0.23.0-isoapplet-offcard-hash branch 4 times, most recently from 66b0cf6 to 458419b Compare December 6, 2022 16:10

Jakuje reviewed Dec 8, 2022

View reviewed changes

swissbit-csteuer force-pushed the 0.23.0-isoapplet-offcard-hash branch from c8acd02 to fda52f5 Compare December 8, 2022 17:00

swissbit-csteuer force-pushed the 0.23.0-isoapplet-offcard-hash branch from 0c22722 to f35cb9b Compare December 9, 2022 09:35

swissbit-csteuer requested a review from Jakuje January 11, 2023 21:29

Jakuje approved these changes Jan 12, 2023

View reviewed changes

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

swissbit-csteuer force-pushed the 0.23.0-isoapplet-offcard-hash branch from f35cb9b to 2151ed3 Compare January 12, 2023 14:49

philipWendland and others added 19 commits January 19, 2023 10:50

isoApplet: add RSA PSS features if supported by applet

0990c97

isoApplet: announce PSS capabilities

36b7a38

undo ci build changes

2f65870

undo changes in pkcs11-tool

9936f35

add backwards compatibility for ISOApplet 0.6.x

17fb857

fix isoapplet test script

0204571

adapt to isoapplet-v1 changes

0b16061

- set correct request and response buffer length for extended APDUs

fix rsa-pss

2a20c1b

- adapt to change in iso applet that only signal PSS support if all hash schemes are supported - strip PKCS#1 digest info when using RSA PSS so that only the hash is sent to the applet

fix memory leak when init fails

da9e7ee

replace spaces with tabs

96076b2

remove p11test result file

7a467b9

remove static keyword from temporary signature buffer

e031346

refactor isoApplet_compute_signature

e645812

move decleration of major_version to the top of the init func

e531832

add rsa 4096 support

c1404e3

perform shallow clone of IsoApplet repository for testing

a17ffef

update p11test reference result for v1 isoapplet

3845590

swissbit-csteuer force-pushed the 0.23.0-isoapplet-offcard-hash branch from 2151ed3 to 3845590 Compare January 19, 2023 09:51

swissbit-csteuer requested a review from Jakuje January 19, 2023 10:56

Jakuje approved these changes Jan 19, 2023

View reviewed changes

Jakuje merged commit 9dd5a8b into OpenSC:master Jan 27, 2023

philipWendland mentioned this pull request Jan 27, 2023

Github test: adjust package dir and git branch of IsoApplet versions #2688

Merged

swissbit-csteuer deleted the 0.23.0-isoapplet-offcard-hash branch February 1, 2023 08:11

xhanulik mentioned this pull request Jul 3, 2023

New release 0.24.0 #2792

Closed

isoapplet offcard hash #2642

isoapplet offcard hash #2642

Uh oh!

Conversation

swissbit-csteuer commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

swissbit-csteuer commented Nov 16, 2022

Uh oh!

frankmorgner commented Nov 17, 2022

Uh oh!

swissbit-csteuer commented Nov 17, 2022

Uh oh!

philipWendland commented Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipWendland commented Nov 19, 2022

Uh oh!

swissbit-csteuer commented Nov 22, 2022

Uh oh!

popovec commented Nov 22, 2022

Uh oh!

dengert commented Nov 22, 2022

Uh oh!

popovec commented Nov 23, 2022

Uh oh!

swissbit-csteuer commented Nov 23, 2022

Uh oh!

Uh oh!

Uh oh!

Jakuje Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

swissbit-csteuer Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swissbit-csteuer commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipWendland commented Dec 10, 2022

Uh oh!

philipWendland commented Jan 2, 2023

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swissbit-csteuer commented Jan 12, 2023

Uh oh!

Jakuje commented Jan 12, 2023

Uh oh!

swissbit-csteuer commented Jan 13, 2023

Uh oh!

swissbit-csteuer commented Jan 19, 2023

Uh oh!

swissbit-csteuer commented Jan 23, 2023

Uh oh!

Jakuje commented Jan 23, 2023

Uh oh!

philipWendland commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

swissbit-csteuer commented Nov 14, 2022 •

edited

Loading

philipWendland commented Nov 19, 2022 •

edited

Loading

swissbit-csteuer commented Dec 8, 2022 •

edited

Loading