-
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
Add support for Slovenian eID card (eOI) #2646
Conversation
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.
just first fast review. Good job, but it will need some cleanup yet.
@@ -1207,7 +1207,8 @@ pkcs15_init_slot(struct sc_pkcs15_card *p15card, struct sc_pkcs11_slot *slot, | |||
p15card->tokeninfo ? p15card->tokeninfo->label : "", | |||
32); | |||
} | |||
slot->token_info.flags |= CKF_LOGIN_REQUIRED; | |||
if (p15card->tokeninfo->flags & SC_PKCS15_TOKEN_LOGIN_REQUIRED) |
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.
This may affect other cards. Is it verified that SC_PKCS15_TOKEN_LOGIN_REQUIRED will be set correctly for other cards?
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 don't know. But assuming that the card needs a logon if PIN object is present is at least in this case wrong (especially since the card says otherwise). And I think if the card erroneously reports that it doesn't need a logon, then it's for the card driver to fix this?
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.
This commit breaks skeid and possibly other cards as no driver ever sets SC_PKCS15_TOKEN_LOGIN_REQUIRED
. I also refrained from using the flag, the following line told me so:
Line 603 in aedf3e7
#define SC_PKCS15_TOKEN_LOGIN_REQUIRED 0x02 /* Don't use */ |
assuming that the card needs a logon if PIN object is present is at least in this case wrong
Not within the context of PKCS #11, where the flag CKF_LOGIN_REQUIRED
has the following meaning:
True if there are some cryptographic functions that a user MUST be logged in to perform
The naming might be somewhat unfortunate, but the prior behaviour was very much in line with the specification.
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.
Well, at least in the case of eOI low-level/pin-less app there are no cryptographic functions that a user MUST be logged in to perform, so omitting the CKF_LOGIN_REQUIRED
is in place.
Perhaps other drivers should set SC_PKCS15_TOKEN_LOGIN_REQUIRED
(why is there a Don't use
comment)?
Reverting this commit would break eOI pin-less operation, so I would prefer not to do it?
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.
there are no cryptographic functions that a user MUST be logged in to perform
Why does that particular token have PINs associated with it then?
eOI pin-less operation
What can the card do without a PIN?
Reverting this commit
That is not what I am suggesting. Let us simply aim for all the drivers working.
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.
From your PKCS#15 profile above,
Private EC Key [{bfeeb6fb-1677-f82b-5134-992d9ddcbcb5}]
does not have anAuth ID
associated, which means that it doesn't require a PIN validation before use. This seems to be what you want and the private key should be registered as public object in a second seperate slot which does not set theCKF_LOGIN_REQUIRED
flag. Could you please check why this is not the case for you?
Well, since a PIN object exists, a slot is created (although without any objects) and thus the !slot (in your code excerpt) is false and your code never executes, the public objects are added in the next step (to the previously created slot)...
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.
In case I was not clear, a slot for objects that require "Norm PIN" is created here:
OpenSC/src/pkcs11/framework-pkcs15.c
Line 1671 in 7b782d5
rv = pkcs15_create_slot(p11card, fw_data, auths[i], app_info, &islot); |
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.
Thank you for clearifying. since I don't have such a token, I didn't know what exactly what was happening.
You're right that the current logic does not create a second slot for all the public objects (I was previously wrong about this). Instead, all public objects are added to the PIN-slot (even though they don't require a PIN validation):
OpenSC/src/pkcs11/framework-pkcs15.c
Lines 1716 to 1719 in 7b782d5
if (slot) { | |
sc_log(context, "Add public objects to slot %p", slot); | |
_add_public_objects(slot, fw_data); | |
} |
Setting CKF_LOGIN_REQUIRED
to this single slot, means that "there are some cryptographic functions that a user MUST be logged in to perform". So a good application would still check the authId to check whether or not a PIN validation is required in order to use the private key.
Since I know that changing the workflow of your application is most likely not possible, you could try to single out the PIN objects from the rest of the objects. You already did that by removing the PIN objects above, which gives a slot that doesn't have CKF_LOGIN_REQUIRED
set. Now, instead of removing them, you can attach the PINs to an additional virtual card application in sc_card_t
, which should then be bound in a separate slot. That would involve creating a virtual AID (sc_aid
), which can be used by sc_pkcs15emu_eoi_init_ex()
to check whether only the PIN objects should be propagated or whether everything but the PIN objects should be propagated to the PKCS#15 profile.
Note that we already have some workarounds in our code built in for PKCS#11 applications, that aren't very nice (NSS 👀). If you clearify on your application/use case we may consider adding more workarounds. However, fixing the application would make sense, imho.
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 don’t have (or work on) a specific application in mind. I think the only applications that most probably use pin-less applet of our eID card are applications for health insurance/medical professionals, where eID card can be used instead of the old health insurance card.
And I think all this software runs on windows and uses the official middleware, anyway.
But I still have an issue with the CKF_LOGIN_REQUIRED
flag being set, as it is now:
a) When eOI pin-less applet is being used, there are clearly NO cryptographic functions that a user MUST be logged in to perform, so CKF_LOGIN_REQUIRED
should not be set even if there are PIN objects on the card. The output of the pkcs11-tool –list-slots
(with official dll, which is my reference here)
Slot 0 (0x0): Gemalto IDBridge CT7xx 0
token label : Prijava brez PIN-a
token manufacturer : NXP
token model : ChipDocLite
token flags : rng, token initialized, PIN initialized, readonly
hardware version : 1.0
firmware version : 1.0
serial num : ****************
pin min/max : 6/6
shows that CKF_LOGIN_REQUIRED
is not set, but CKF_USER_PIN_INITIALIZED
is, which, I guess, tells the calling application that PIN/authentication objects are present and authentication should be used to perform some non-cryptographic functions (change PIN, perhaps alter some data)?
b) loginRequired in PKCS#15 tokenFlags is explained as "if login (i.e., authentication) is required before accessing any data" and I think one could easily translate this to PKCS#11 CKF_LOGIN_REQUIRED
"True if there are some cryptographic functions that a user MUST be logged in to perform" and should be treated similarly. Or at the least have a way to handle it from a card driver and not just assume that if there are PIN/authentication objects, CKF_LOGIN_REQUIRED
should be set, as it is now.
c) and finally, during development and testing of eOI support, I have noticed that if CKF_LOGIN_REQUIRED
flag is set some applications (although I can't remember which ones, maybe even Firefox), want to perform a login (and ask for PIN) in order to search for private objects (keys and/or certificates). And since, I think, all populated slots are processed, the solution with an extra slot that has CKF_LOGIN_REQUIRED
would popup a login window, which is not what would one want…
In any case, if there won't be any change how CKF_LOGIN_REQUIRED
is handled in the common code, I think I'll just go with deleting the PIN objects for pin-less slot, as I've done in my 2nd example/commit. I think it's just not worth the effort to implement a separate slot for PIN management (and also because of issue c) from above), as this can easily be done when QES applet/slot is used, which is probably a more common usage scenario anyway…
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.
OK, let me know when you have some code ready that deletes the PIN object. I'll try to add something on top in order to push the PIN objects into a different card application (i.e. different PKCS#11 slot).
Sorry, I deleted my eOI branch by accident... |
061a069
to
9a38014
Compare
It now also works on Windows. PIN object selection had to be modified here too. It looks to me that in case of ECDSA minidriver (wrongly?) passes RSA padding flags to the |
Can you try https://github.com/OpenSC/OpenSC/releases/tag/0.23.0-rc2 on windows? There are a number of changes for ECDSA. |
It looks like OpenSC/src/minidriver/minidriver.c Lines 4783 to 4796 in 8eda758
And here is an excerpt from the debug log (from my eOI branch, but the relevant code is, I believe, unchanged):
|
The name Starting here:https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/opensc.h#L102 are the flags. Some are overloaded depending on the type of key. The sec flags end up with: The Your debug log did not go on to show if the operation worked or not. So it is not clear if you have a problem in the card Can you try doing the same sign operation using either pkcs11-tool or pkcs15-crypt, to see if these work but not minidriver. In https://github.com/llogar/OpenSC/blob/eOI/src/libopensc/card-eoi.c#L264-L269 |
It works now. I just pasted the part of the log where you can see that the flags in
It worked using the pkcs11 (testing with pkcs11-tool & firefox) as it handles this a little differently than minidriver. That's why I only spotted it when testing the minidriver.
I looked at the init() code for other eID cards, npa and edo, (where ext_flags is 0). And it seems it works just fine. |
d546d17
to
70e34a2
Compare
@llogar thank you for the great work. |
Rebased on top of #2674, so sm-eac hack is not needed any more. |
d8b3dd1
to
fc27f21
Compare
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.
Code looks good overall. Just two more minor comments.
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 now, thank you
src/libopensc/card-eoi.c
Outdated
if (!privdata->can[0]) { | ||
const char *can = scconf_get_str(block, "can", NULL); | ||
if (can) | ||
strncpy(privdata->can, can, sizeof(privdata->can) - 1); |
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.
same here -- missing can
length checks.
@llogar can you rebase the changes on top of current master to resolve the conflict and we can merge the changes? |
Slovenian eID card has gaps between consecutive PKCS#15 objects in xDF (PrKDF, CDF) files. Currently parsing stops when it reaches EOC. With this patch parsing continues until whole file is processed and end-of-file reached. This way, all the objects that are stored on a Slovenian eID card are read properly.
Currently CKF_LOGIN_REQUIRED flag is set if any PIN objects exist. But Slovenian eID card (at least the 'low level' app) has PIN objects and still doesn't require PIN. So, set CKF_LOGIN_REQUIRED only if PIN objects exist AND TokenInfo has TOKEN_LOGIN_REQUIRED set.
Currently the first suitable PIN is used as a PKCS#11 'User PIN'. However Slovenian eID's first PIN object is 'Card CAN' which is not used for accessing on-card files. With this patch per PKCS#15 application 'user_pin' and 'sign_pin' options are implemented, which can be used to precisely specify which PIN objects should be used instead of the ones library automagically picked. The same also applies to the minidriver code.
Data field is absent when P1 is 0x03
Slovenian eID card has ECC curve OID included in the PrKDF key object data. By parsing this data one can get the private key length which will be needed later.
…SC driver name 'eoi') It mostly follows the PKCS#15 model with some quirks which are handled by the provided code. Installed on the eID card are 2 applications (that are of interest to OpenSC): 1. app:E8:28:BD:08:0F:01:4E:58:50:31 (Prijava brez PIN-a) which can be used for Low assurance authentication that does not require PIN entry. It includes 1 private key/certificate and several PIN objects, but it looks like they are not needed for normal operation. 2. app:E8:28:BD:08:0F:01:4E:58:50:30 (Podpis in prijava) which can be used to make QES signatures and High assurance authentication and includes 2 private keys/certificates and several PINs, including 'Norm PIN' & 'Sig PIN' PINs, which should probably be exposed as 2 PKCS#11 slots. Both applications also include full certificate chains needed to verify respective user certificates. 'Norm PIN' is shared between both applications, so 'Norm PIN' change in one also applies to the other. SM with PACE authentication is used for establishing a secure communication with the card. In case of contact reader, CAN is read from the card (where it's stored in an encrypted form), decrypted and used for authentication. In case of contactless reader, CAN has to be provided via the opensc.conf file or EOI_CAN environment variable. The following functionalities have been implemented: - secure login - PIN change - PIN unblock using "pkcs15-tool --unblock ..." and/or "pkcs11-tool --unlock-pin ..." (user_pin_unblock_style must be set to set_pin_in_unlogged_session for the later) - digital signatures, SHA1, SHA256 and raw data are supported Not implemented: - card activation The card has been tested on Ubuntu 22.04 and with both contact (pinpad readers will probably have some issues) and contactless readers.
Do I understand, that this came to a dead end where it was not expected that there are keys on the card that do not need PIN? |
PIV card has a key that is usable without PIN. Look at pkcs15-piv.c
Note absence of Key is used to authenticate the card. For example with a door lock. |
This series of patches adds support for Slovenian eID card.
Some minor modifications to the core/common code were needed in order to make this card work.
See comments in individual commits...
Comments and suggestions are welcome... #2564
Checklist