-
Notifications
You must be signed in to change notification settings - Fork 711
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
PIV Secure Messaging as defined in NIST 800-73-4 #2053
Conversation
Some windows builds seems to fail with the following errors:
|
Yes, I am still looking at the pid issue. The pid was chosen as it is POSIX andand should work on Windows. But searching shows diferent windows compilers do thing differently. I will use GetCurrentProcessId which OpenSC log.c uses. |
That are quite some changes... I think it will take some time until I have the capacity of doing a deeper review. Just some quick notes:
|
Thanks for the review. Not the code is not complete, and there are a number of TODO comments. I have been trying to address these. NIST SP 800-73-4 Part 2 Section 1.1 Purpose says: "SP 800-73-4 enumerates requirements where the international integrated circuit card (ICC) standards [ISO7816] include options and branches. The specifications go further by constraining implementers’ interpretations of the normative standards. Such restrictions are designed to ease implementation, facilitate interoperability, and ensure performance, in a manner tailored for PIV applications." So I based the implementation on what is in SP 800-73-4. One of the restrictions is to not say anything about extended APDU. The examples and figures in section 4.2 Secure Messaging, show using command chaining, and 255 bytes to be sent in each, as well as 256 bytes responses. Even if card says it supports extended, the applet may not. Thus PIV When testing the code, The unsecure "plain" apdu would have command chaining applied before the SM code was called. Thus only the first APDU in the chain with 255 bytes of data was passed to card->sm_ctx.ops.get_sm_apdu = piv_get_sm_apdu and piv_encode_apdu. So the SC_APDU_FLAGS_SM_CHAINING flag was added to pass "plain" apdu, without doing command chaining. Then after piv_encode_apdu created the "sm_apdu" command chaining would be applied. The original code for existing cards, must have been using extended APDUs and never run into this problem, so this could be a bug. If so the patch could be simplified. You asked: "How exactly is SM switched on? That should be documented somewhere." This is one of the "TODO" items. The PR is experimental, so for now, it is turned on from piv_init. If there are any errors and it gets turned off, it does not get turned back on. PIV SM could be used to secure everything, or maybe just pins and crypto operations. Some objects should read using SM, such as iris images, facial image and fingerprints. (They require PIN access too.) Or all objects including certificates. So the use could depend on if NFC is being used, or user want everything under SM. opensc.conf could be used. So not clear what approach take. The Virtual Contact Interface (VCI) is designed to be used over NFC (phone or tablet) and so everything is done via SM. (VCI is an additional optional feature). Could be used over RDC too. I had one older IDEMIA card that supported SM, but not VCI or pairing code. Since then, As IDEMIA said in a note: "Let us know if we can help spread the news about your open source implementation of PIV Secure Messaging. That could help other developers of applications using the Virtual contact interface, and make terminals that support VCI ubiquitous. ;-)" |
d5a9be5
to
d9461e9
Compare
7205316
to
9887c39
Compare
This pull request introduces 1 alert when merging 9887c39 into ad81126 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging e74c794 into f0b157b - view on LGTM.com new alerts:
|
e74c794
to
ac276f0
Compare
Can anyone explain this failure?
This appears to have nothing to do with this PR. |
I think I saw something similar before. It should not be anything in your code. It fails tests in jcardsim for some reason. I think it is non-deterministic test and when I restarted the job, it worked (I just restarted yours). Edit: Now, it is green |
I would like to see this PR included in the next release. (This is not what I expressed in a recent note to @frankmorgner and @Jakuje.) There is further interest in support of the PIV extensions in this PR. Based on how often OpenSC releases are done, it would be more timely to have it in the next release so it can be further tested in release candidates in the near future. In the original comment above I referred to a number of TODOs in the code. These have now been addressed:
The code has also been tested using PivKey and Yubikey (which do not support SM). The code has also been configured with The SM code has been tested using a set of IDEMIA provided test cards. Although the SM code works with the OpenSC minidriver (with some hand editing of the registry), the card vendor provides their own minidriver and is installed via Microsoft plug-and-play. Microsoft still provides their own built in minidriver for PIV, but has not been update since 2006. So at the moment, the vendor or Microsoft drivers are used in most situation. |
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 is getting huge. I put there just couple of comments while I reviewed it. I would like certainly to test it at least for compatibility with other piv cards I have around when it will be ready.
OpenSC#2053 (review) On branch PIV-4-extensions Changes to be committed: modified: src/libopensc/card-piv.c
@Jakuje Thanks for the quick review. In regards to testing, note that the The response to SELECT_AID for PIV cards returns if a card supports SM or not, The discovery object says if VCI is implemented and if pairing code is needed or not. So older cards should work as before. I have also compiled with --disable-SM, and older cards continue to function as expected. |
OpenSC#2053 (review) On branch PIV-4-extensions Changes to be committed: modified: src/libopensc/card-piv.c
974b874
to
7925fc5
Compare
I just tried to compile your branch and I am getting the following errors:
I believe that the bit-wise OR should be AND as in other cases. Not sure why this check did not pop up in our CI, but it pops up with my gcc 10. |
Additionally, I am getting crash with old PIV card:
because |
Will fix later today.
…On Wed, Sep 2, 2020, 9:05 AM Jakub Jelen ***@***.***> wrote:
Additionally, I am getting crash with old PIV card:
Thread 1 "lt-pkcs11-tool" received signal SIGSEGV, Segmentation fault.
sc_hex_to_bin (in=0x3200000008 <error: Cannot access memory at address 0x3200000008>,
***@***.***=0x7fffffffc658 "", ***@***.***=0x7fffffffbbf8) at sc.c:73
73 size_t left = *outlen;
#0 sc_hex_to_bin
(in=0x3200000008 <error: Cannot access memory at address 0x3200000008>, ***@***.***=0x7fffffffc658 "", ***@***.***=0x7fffffffbbf8) at sc.c:73
#1 0x00007ffff7e0a530 in sc_pkcs15_format_id (str=<optimized out>, ***@***.***=0x7fffffffc658) at pkcs15.c:2461
#2 0x00007ffff7ed67f2 in sc_pkcs15emu_piv_init (p15card=0x451020) at pkcs15-piv.c:1034
#3 0x00007ffff7e1d5c3 in sc_pkcs15_bind_synthetic ***@***.***=0x451020, ***@***.***=0x0)
at pkcs15-syn.c:125
#4 0x00007ffff7e0c3cb in sc_pkcs15_bind (card=0x455360, ***@***.***=0x0, ***@***.***=0x4568a0)
at pkcs15.c:1258
#5 0x00007ffff72ff25a in pkcs15_bind (p11card=0x450ee0, app_info=<optimized out>) at framework-pkcs15.c:324
#6 0x00007ffff72f65c9 in card_detect ***@***.***=0x454780) at slot.c:314
#7 0x00007ffff72f6c28 in card_detect_all () at slot.c:420
#8 0x00007ffff72f1104 in C_Initialize (pInitArgs=<optimized out>) at pkcs11-global.c:323
#9 0x0000000000404302 in main (argc=4, argv=0x7fffffffdef8) at pkcs11-tool.c:975
(gdb) f 2
#2 0x00007ffff7ed67f2 in sc_pkcs15emu_piv_init (p15card=0x451020) at pkcs15-piv.c:1034
1034 sc_pkcs15_format_id(pubkeys[i].auth_id, &pubkey_obj.auth_id);
because sc_pkcs15emu_piv_init, tries to access behind the pubkeys array
bounds (because i goes up to PIV_NUM_PUB_KEYS (25), while pubkeys is only
PIV_NUM_KEYS (24) long.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2053 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGTIMPQPW5BNEMXDN7DZSDSDZGK3ANCNFSM4NZWB5NA>
.
|
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.
Th ere are few more (mostly minor) nits and comments. Tested with old NIST Test cards and they seem to work fine.
Changes as sujested in: OpenSC#2053 (review) an err: return was added to piv_decode_cvc as suggested by calling piv_clear_cvc_content(cvc); if there was an error. It was nt really needed, as the two CVC structures would be cleared in finish. On branch PIV-4-extensions Changes to be committed: modified: card-piv.c modified: pkcs15-piv.c
OpenSC#2053 (review) On branch PIV-4-extensions Changes to be committed: modified: src/libopensc/card-piv.c
Changes as sujested in: OpenSC#2053 (review) an err: return was added to piv_decode_cvc as suggested by calling piv_clear_cvc_content(cvc); if there was an error. It was not really needed, as the two CVC structures would be cleared in finish. On branch PIV-4-extensions Changes to be committed: modified: card-piv.c modified: pkcs15-piv.c
4f22d1c
to
1420a3c
Compare
a26c15e
to
efd53ca
Compare
Looking for other users who have cards that support PIV SM and VCI for testing. Changes to PIV code for SM as defined in NIST 800-73-4. Section 4.1 The Key Establishment Protocol is done in piv_sm_open. Step names and variable names were chosen to match the names used used in 800-73-4. piv_get_sm_apdu, piv_free_sm_apdu, and piv_sm_close use the builtin SM apdu handling. This version calls piv_sm_open once from piv_init. and card->sm_ctx.sm_mode is set. See TODO below. PR has been tested with pkcs11-tool -O and --test --login using a "IDEMIA ID-One PIV 2.4 on Cosmo V8.1" with vendor provided certificates (about 25 certificates and keys) and other data objects. The test card does not have a "pairing code object" need for VCI for use over a contactless interface (NFC), But code has been added to support pairing to allow testing. The PIV SM code is only enabled if ENABLE_SM, ENABLE_OPENSSL and OPENSSL_NO_EC is not defined. It was tested with --disable_sm A card indicates it can suport SM in the response to SELECT_AID. If card can support SM, but OpenSC was built without ENABLE_SM a sc_log message will say so. card-piv.c use SC_APDU_FLAGS_SM_CHAINING from previous commit. This allows the PIV to pass a plain ADPU which needs command chaining, to SM and piv_get_sm_apdu and will encrypt and MAC the data before command chaining is done in apdu.c NIST sp800-73-4 3.3.2 extends pin policy usage flags for optional VCI and OCC are defined. Checked with valgrind, pkcs11 -O and pkcs11-tool --test --login If card supports SM it is turned on in piv_init, so if card is reset or interfered with from other process, SM will not restart. Signer certificates and CVC certificates are verified. If interfered with by other processes, and SM session is lost, it is restarted. TODO Need a way to give user paring code from card over usb after login if it is not printed on card or distributed in some other way. MD_MAX_KEY_CONTAINERS 32 add piv_logout PIV test card have more the 12 keys. "card_driver PIV_II {" block in opensc.conf See: etc/opensc.conf.example.in piv_max_object_size - removes the code to read first 8 bytes to get object size and use piv_max_object_size as read buffer size. default is 16K, max is 65K piv_use_sm - default, never, always default - use it for PIN, crypto and reading objects that are PIN protected Other objects are read in the clear for performance. never - Don't use SM, even if card supports it. Can help is situations were problems ith SM, and to debug other PIN or cryto problems. always -Like default, but read all objects using SM. piv_pairing_code - Card may require user to enter 8 digit pairing code to use VCI so card can be used over contactless as if using contact reader. VCI requires SM, and encrypts everything. All can be set via env. PIV_USE_SM= PIV_PAIRING_CODE= Used of a contactless reader is identified by the ATR 3B 8X 80 01 .... Rework PIV card matching and init for less overhead. piv_match_card_continued was committed in 4222036 2018-02-28 to handle limitations in card.c on not allowing *_match_card to pass anything other the card->type to *_init routines. These restrictions were removed in 2c0d1b9 2018-07-05. piv_match_card_continued is only called once, from piv_match_card does some checks, sets card->type, allocates piv_private_data_t, saves it in card->drv_data, calls sc_lock. If piv_match_card_continued fails, piv_match_card will call sc_lock, piv_finish and return 0 (failed to match). And just in case piv_match_card is not called, piv_init will call piv_match_card_continued. And if it fails will call sc_unlock, piv_finish and return SC_ERROR_INVALID_CARD. The card lock is finally released at end of piv_init. This allows no interference from other process during piv_match_card and piv_init. If CSAI 0xAC tag is found in the response to a SELECT AID and is used to say the card supports SM. It will still do this even if built without SM so it will show up in debug logs. PIV specs are vague and some PIV applets and a 0xAC tag for every algorithm and not just for SM. PIV Secure Messaging requires at least OpenSSL-1.1.1 or OpenSSL-3.0.0 Added equivelent code from PR 2366. Pairing code is optional, and only used when creating a VCI over contetless reader. It can be provided via env PIV_PAIRING_CODE or in opensc.conf. In any case the paring code, if provided, must be 8 ASCII digits. There is no not easy way to tell the user the code is invalid. "piv_parse_pairing_code" is added to check the length and digits. The caller will add a debug log entry if it is invalid so there is a record of the failure. With 800-73-4 Secure Messaging the SELECT AID response specifies which cryptographic algorithms under tag 0xAC are supported for Secure Messaging. The code was using the discovery object to test if the PIV applet is active as some cards have a card issue of losing the login state if the SELECT AID is used instead. (None of these cards support SM so reading the discovey object was as good as doing SELECT_AID.) The problme was found while running in contactless mode, card would work the first time becaus the discovery object would not find the PIV applet so a SELECT AID was done and it would also update the the cryptographic algorithms. When run a second time, reading the discovery object would work but the SELECT AID would only be done near the end of match routine for card types the may support 800-73-4. The duplicate "sc_atr" was not listed as one the need to have SELECT AID done. PIV change processing of CVC certificates At the request of others va github comment, the method to used to extract an optional intermediate CVC certificate was changed. Unlike other certificate objects in 800-73-4, "Table 42. Secure Messaging Certificate Signer" the "Intermediate CVC (Conditional)" does not have an enclosing tag, but uses the 0x7F21 tag. Later the 0x7F21 tag is considered part of the certificate and a hash of the the full certificate is sent to the card as part of SM. OpenSC has a number of asn1 routines such as "sc_asn1_find" to find tags but once found, they only return the address of the value(V) and its length(L) but do not return the address of the found tag(T). The previous code reconstructed the address of the found tag be calculating the number of bytes it took to encode (L) and known tag(T). 800-73-4 says the "Intermediate CVC (Conditional)" immediately follows the "CertInfo" so the address of the following byte is saved to locate where the "Intermediate CVC (Conditional)" could start. Rename dec_counter to resp_enc_counter as name was misleading 800-74-4 says: "(i.e., the IV used to encrypt the first response after successful completion of the key establishment protocol shall be generated by encrypting '80 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01' with SKENC)." Use the same (encrypted) IV the card used to encrypt the response when decrypting the response. Explain how SM APDU case is derived Added comment and used defines to show how the APDU for SM is derived from the plain APDU. and how it will allow for extended APDUs if NIST allows them or card vendor in known to support them. Changes to be committed: modified: etc/opensc.conf.example.in modified: src/libopensc/card-piv.c modified: src/libopensc/cards.h modified: src/libopensc/pkcs15-piv.c modified: src/libopensc/types.h
Update PIV conf and env in opensc.conf.5.xml.in Improved card match and testing for SM cards Allow force of SC_CARD_TYPE_PIV_II_BASE, which will test for all posible type of cards tested including 800-74-4 supported features. Tested with ID-One with SM, Older NIST beta cards: Gemalto and Oberthur, YubiKey: 4 and 5 NFC and PIVKey C910. Allow testing PIV SM with or without github.com/OpenSC/pull/2712 Clear CVC contents if CVC fails to encode In responses to OpenSC#2053 (comment) For example, if the CVC can not be parsed, clear it by calling piv_clear_cvc_content Add PIV SM functions prototypes as static PIV Use piv_free_sm_apdu to cleanup if piv_encode_apdu fails This is in response to: OpenSC#2053 (comment) and OpenSC#2053 (comment) PIV Improve testing of AuthCryptogram This is in response to: OpenSC#2053 (comment) PIV goto err if AuthCryptogram check fails PIV Add check for plain->resp == NULL Handle case where apdu resp == NULL and resplen > 0 which would be a programming error. card-piv.c With SM and no data returned set plain->resplen=0 Fixes OpenSC#2053 (comment) PIV fix checking of padding Fixes: OpenSC#2053 (comment) PIV SM - Unzip SM Certificate Signer Certificate With SM, the Cert Signer certificate may be ziped. card-piv.c needs to extract the public key before pkcs15 emulation is setup. Call sc_decompress_alloc. Changes to be committed: modified: doc/files/opensc.conf.5.xml.in modified: src/libopensc/card-piv.c
This is in response to: OpenSC#2053 (comment) Configure OpenSC by setting CPPFLAGS="-DENABLE_PIV_SM" to enable the PIV SM code in card-piv.c doc/files/opensc.conf.5.xml.in and etc/opensc.conf.example.in still have the text for PIV SM and could be modified if needed. On branch PIV-4-extensions Changes to be committed: modified: card-piv.c
48f802f
to
570f3e0
Compare
As suggested in #2053 (review) the 53 commits in this PR have been reduced to 19. |
Update PIV conf and env in opensc.conf.5.xml.in Improved card match and testing for SM cards Allow force of SC_CARD_TYPE_PIV_II_BASE, which will test for all posible type of cards tested including 800-74-4 supported features. Tested with ID-One with SM, Older NIST beta cards: Gemalto and Oberthur, YubiKey: 4 and 5 NFC and PIVKey C910. Allow testing PIV SM with or without github.com/OpenSC/pull/2712 Clear CVC contents if CVC fails to encode In responses to OpenSC#2053 (comment) For example, if the CVC can not be parsed, clear it by calling piv_clear_cvc_content Add PIV SM functions prototypes as static PIV Use piv_free_sm_apdu to cleanup if piv_encode_apdu fails This is in response to: OpenSC#2053 (comment) and OpenSC#2053 (comment) PIV Improve testing of AuthCryptogram This is in response to: OpenSC#2053 (comment) PIV goto err if AuthCryptogram check fails PIV Add check for plain->resp == NULL Handle case where apdu resp == NULL and resplen > 0 which would be a programming error. card-piv.c With SM and no data returned set plain->resplen=0 Fixes OpenSC#2053 (comment) PIV fix checking of padding Fixes: OpenSC#2053 (comment) PIV SM - Unzip SM Certificate Signer Certificate With SM, the Cert Signer certificate may be ziped. card-piv.c needs to extract the public key before pkcs15 emulation is setup. Call sc_decompress_alloc. Changes to be committed: modified: doc/files/opensc.conf.5.xml.in modified: src/libopensc/card-piv.c
This is in response to: OpenSC#2053 (comment) configure.ac add --enable-piv-sm option with default disabled Changes to be committed: modified: configure.ac modified: doc/files/opensc.conf.5.xml.in modified: etc/opensc.conf.example.in modified: src/libopensc/card-piv.c
See: https://github.com/OpenSC/OpenSC/pull/2053/files#r1267420364 On branch PIV-4-extensions Changes to be committed: modified: card-piv.c
The Secure Messaging Certificate Signer does not have a private key on the card. The public key was extracted from the certificate but never freed later while creating private key entries. On branch PIV-4-extensions Changes to be committed: modified: pkcs15-piv.c
simplify code and configuration options
This fixes an erroneous call of sc_unlock in piv_match_card_continued in case of an error, which causes sc_unlock to be called more often than sc_lock.
Changes to be committed: modified: src/libopensc/card-piv.c
The use of priv->max_object_size = MAX_FILE_SIZE; causes SM to exceed 65K when creating SM apdu from plain apdu. The plain apdu will have 65K, and SM apdu will add 40 bytes. f05eb3e "replace PIV_MAX_OBJECT_SIZE with MAX_FILE_SIZE" introduced the problem. pcsc internally will allocate another buffer the size of resplen. SCardTransmit will get a 0x80100008 error. Remove some TODO comments Use cipher vs cypher https://english.stackexchange.com/questions/147965/cipher-vs-cypher Remove a nit and combined two "#if"... #endif" sections into one. Remove piv_is_expected_tag and replace with inline code in 3 places. Changes to be committed: modified: src/libopensc/card-piv.c
On branch PIV-4-extensions Changes to be committed: modified: card-piv.c
570f3e0
to
91283f3
Compare
As requested, The PR was rebased: Built with --enable-piv-sm and the run Built without --enable-piv-sm and the run Then using a ACS ACR122U contactless reader: With --enable-piv-sm built code ran Without --enable-piv-sm built code ran ./pkcs11-tool --test --login` as expected it returns:
PIV card does not allow login over contactless without SM. |
Thanks! I think we are good to go. @frankmorgner any other comments or good to merge? |
Fine with me, thank you everyone! |
Thank you! I will merge it and I think we can follow-up with the rc after that in #2792. |
Update PIV conf and env in opensc.conf.5.xml.in Improved card match and testing for SM cards Allow force of SC_CARD_TYPE_PIV_II_BASE, which will test for all posible type of cards tested including 800-74-4 supported features. Tested with ID-One with SM, Older NIST beta cards: Gemalto and Oberthur, YubiKey: 4 and 5 NFC and PIVKey C910. Allow testing PIV SM with or without github.com//pull/2712 Clear CVC contents if CVC fails to encode In responses to #2053 (comment) For example, if the CVC can not be parsed, clear it by calling piv_clear_cvc_content Add PIV SM functions prototypes as static PIV Use piv_free_sm_apdu to cleanup if piv_encode_apdu fails This is in response to: #2053 (comment) and #2053 (comment) PIV Improve testing of AuthCryptogram This is in response to: #2053 (comment) PIV goto err if AuthCryptogram check fails PIV Add check for plain->resp == NULL Handle case where apdu resp == NULL and resplen > 0 which would be a programming error. card-piv.c With SM and no data returned set plain->resplen=0 Fixes #2053 (comment) PIV fix checking of padding Fixes: #2053 (comment) PIV SM - Unzip SM Certificate Signer Certificate With SM, the Cert Signer certificate may be ziped. card-piv.c needs to extract the public key before pkcs15 emulation is setup. Call sc_decompress_alloc. Changes to be committed: modified: doc/files/opensc.conf.5.xml.in modified: src/libopensc/card-piv.c
This is in response to: #2053 (comment) configure.ac add --enable-piv-sm option with default disabled Changes to be committed: modified: configure.ac modified: doc/files/opensc.conf.5.xml.in modified: etc/opensc.conf.example.in modified: src/libopensc/card-piv.c
This still under development. I am looking for PIV users who have cards which have implemented NIST 800-73-4 optional features, Secure Messaging, Pairing code and VCI.
Code has been tested with "IDEMIA ID-One PIV 2.4 on Cosmo V8.1" with vendor/U.S. gov certs, keys and other objects. It has NFC capabilities, but not a pairing code, and does not say it supports VCI.
There are 4 commits,
A general PIV update based on DOD memo from 2019 dealing with dual CAC/PIV cards, and added the ATR for the IDEMIA test card.
Allows APDU that would require command chaining to be passed to SM routines before command chaining is applied. This is done with new flag SC_APDU_FLAGS_SM_CHAINING that controls the actions of the commit. This lets PIV card that uses short APDUs to encrypt data and apply a MAC to data before command chaining is done.
A minor debugging aid to list the APDU flags in the OpenSC log.
Main change to card-piv.c pkcs15-piv.c and types.h to add SC_CARD_TYPE_PIV_II_800_73_4
The first 3 could be added today, as they should have no effect on on any other cards.
The last is still under development and has a number of TODO issues, listed in the commit and in the code.
https://github.com/arekinath and https://github.com/makinako you may have some interest in this too.