-
Notifications
You must be signed in to change notification settings - Fork 716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
isoapplet offcard hash #2642
isoapplet offcard hash #2642
Conversation
Hi @philipWendland , since this PR also affects the ISOApplet, could you take a look at it? |
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... |
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.
Done. |
Hi, 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. ECC with off-card hashes requires JavaCard 3+ and I want to keep supporting v2.2.2 cards with IsoApplet. |
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):
|
In my opinion, it is not necessary to change anything in the Testing is done by calling
Result is verified both with Example: if we calculate ECDSA (without applying the hash function) for |
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 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. |
For The only reason for introducing the 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... |
@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 |
66b0cf6
to
458419b
Compare
src/libopensc/card-isoApplet.c
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c8acd02
to
fda52f5
Compare
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 |
0c22722
to
f35cb9b
Compare
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:
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. |
From my point of view, this PR can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f35cb9b
to
2151ed3
Compare
Thanks for the review. I have rebased onto the current master. |
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, ... |
Yes, it's deterministic and I can now reproduce it locally. Before, I used a different
The problem is that The virtual smartcard ifd handler (ifd-vpcd) does not support I'm not sure about the best way to resolve this issue. 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
2151ed3
to
3845590
Compare
The IsoApplet CI tests are now running without issues. 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. |
Is there more that needs to be done before this can be merged? |
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. |
Hi Jakuje, This means that the test scripts need to be updated, please see #2688 |
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