Skip to content
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

Merged
merged 15 commits into from
Sep 12, 2023

Conversation

dengert
Copy link
Member

@dengert dengert commented Jun 9, 2020

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.

  • PKCS#11 module is tested
  • compiled with and without --enable-sm
  • tested with valgrind - no leaks in in PIV code.

There are 4 commits,

  1. 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.

  2. 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.

  3. A minor debugging aid to list the APDU flags in the OpenSC log.

  4. 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.

@Jakuje
Copy link
Member

Jakuje commented Jun 10, 2020

Some windows builds seems to fail with the following errors:

card-piv.c(1414): error C2065: 'pid_t': undeclared identifier
card-piv.c(1414): error C2146: syntax error: missing ';' before identifier 'pid'
card-piv.c(1414): error C2065: 'pid': undeclared identifier
card-piv.c(1516): error C2065: 'pid': undeclared identifier
card-piv.c(1520): error C2065: 'pid': undeclared identifier

@dengert
Copy link
Member Author

dengert commented Jun 10, 2020

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.

@frankmorgner
Copy link
Member

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:

  • I wonder why you didn't use the encoding from iso-sm.h in piv_encode_apdu(), that would have simplified the code at least a little bit...
  • I wonder why you've changed handling chaining/get response for SM. If I remember correct, APDUs are first wrapped into SM and then split up into chains, right?
  • How exactly is SM switched on? That should be documented somewhere.

@dengert
Copy link
Member Author

dengert commented Jul 4, 2020

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
is restricted to short APDUs without anyway to tell if applet supports extended.

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,
IDEMIA sent me a set of 5 test cards that implement these optional features. As far as I know they are the only card vendor that supports these. As noted in original PR, there are two open source versions of PIV applets that could also implement this on the applet side. This PR could be used to test their implementation.

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. ;-)"

@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

This pull request introduces 1 alert when merging 9887c39 into ad81126 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2020

This pull request introduces 1 alert when merging e74c794 into f0b157b - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@dengert
Copy link
Member Author

dengert commented Aug 21, 2020

Can anyone explain this failure?
https://travis-ci.org/github/OpenSC/OpenSC/jobs/719919993#L1645

[ERROR] testGenerateSecretECDH(com.licel.jcardsim.crypto.KeyAgreementImplTest)  Time elapsed: 0.041 s  <<< ERROR!
java.lang.ArrayIndexOutOfBoundsException

This appears to have nothing to do with this PR.

@Jakuje
Copy link
Member

Jakuje commented Aug 21, 2020

[ERROR] testGenerateSecretECDH(com.licel.jcardsim.crypto.KeyAgreementImplTest)  Time elapsed: 0.041 s  <<< ERROR!
java.lang.ArrayIndexOutOfBoundsException

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

@dengert
Copy link
Member Author

dengert commented Aug 21, 2020

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:

  • Code added and tested to handle card-reset and interference from other processes. As each process establishes its own Secure Message session and keys, these replace the other processes session and require piv_sm_open to be called again.

  • Piv_match_card and piv_init have been cleaned up.

  • Opensc.conf or environment are queried for 3 variables. See opensc.conf.example.in and next three points:

  • The use of the trick to read the first few bytes using GET_DATA 0xCB to get the size of the object to be read, has been replaced by using a large buffer to read the whole object. The default is 16K and can be changed in opensc.conf or environment.

  • sp800-73-4 pairing code can be set in environment or opensc.conf

  • Secure messaging can be turned off, or forced to always be used. If card supports SM, the default is to turn it on when using contactless and to use it for pin and crypto operations when using a contact reader.

The code has also been tested using PivKey and Yubikey (which do not support SM). The code has also been configured with --disable-sm and tested with old and new cards. When used with contactless readers true PIV card enforce restrictions on what may be done. Some Yubikey devices to not enforce these restrictions and so the code does not enforce the restrictions leaving it up the device. The SM code also requires OpenSSL with EC support.

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.

Copy link
Member

@Jakuje Jakuje left a 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.

src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Show resolved Hide resolved
src/libopensc/card-piv.c Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Show resolved Hide resolved
dengert added a commit to dengert/OpenSC that referenced this pull request Aug 21, 2020
OpenSC#2053 (review)

 On branch PIV-4-extensions
 Changes to be committed:
	modified:   src/libopensc/card-piv.c
@dengert
Copy link
Member Author

dengert commented Aug 21, 2020

@Jakuje Thanks for the quick review.

In regards to testing, note that .travis.yml was changed so pkcs11-tool --test --login --pin would actually do something when testing the PIV driver. It added self signed certificates. Without a certificate there is no way to find a private key, so there was nothing to test.

the .travis.yml does not test SM, because AFAIK, only the IDEMIA cards have implemented PIV SM. So I am expecting that other applets developers could use this PR to test adding SM in their applets.

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.

dengert added a commit to dengert/OpenSC that referenced this pull request Sep 2, 2020
OpenSC#2053 (review)

 On branch PIV-4-extensions
 Changes to be committed:
	modified:   src/libopensc/card-piv.c
@Jakuje
Copy link
Member

Jakuje commented Sep 2, 2020

I just tried to compile your branch and I am getting the following errors:

card-piv.c: In function ‘piv_decode_apdu’:
card-piv.c:1110:52: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]
 1110 |  if ((asn1_sm_response[1].flags | SC_ASN1_PRESENT) == 0
      |                                                    ^~
card-piv.c:1111:53: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]
 1111 |    || (asn1_sm_response[2].flags | SC_ASN1_PRESENT) == 0) {
      |                                                     ^~

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.

@Jakuje
Copy link
Member

Jakuje commented Sep 2, 2020

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>, 
    out=out@entry=0x7fffffffc658 "", outlen=outlen@entry=0x7fffffffbbf8) at sc.c:73
73		size_t left = *outlen;
#0  sc_hex_to_bin
    (in=0x3200000008 <error: Cannot access memory at address 0x3200000008>, out=out@entry=0x7fffffffc658 "", outlen=outlen@entry=0x7fffffffbbf8) at sc.c:73
#1  0x00007ffff7e0a530 in sc_pkcs15_format_id (str=<optimized out>, id=id@entry=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 (p15card=p15card@entry=0x451020, aid=aid@entry=0x0)
    at pkcs15-syn.c:125
#4  0x00007ffff7e0c3cb in sc_pkcs15_bind (card=0x455360, aid=aid@entry=0x0, p15card_out=p15card_out@entry=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 (reader=reader@entry=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.

@dengert
Copy link
Member Author

dengert commented Sep 2, 2020 via email

Copy link
Member

@Jakuje Jakuje left a 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.

src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/card-piv.c Outdated Show resolved Hide resolved
src/libopensc/pkcs15-piv.c Show resolved Hide resolved
dengert added a commit to dengert/OpenSC that referenced this pull request Sep 9, 2020
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
dengert added a commit to dengert/OpenSC that referenced this pull request Sep 21, 2020
OpenSC#2053 (review)

 On branch PIV-4-extensions
 Changes to be committed:
	modified:   src/libopensc/card-piv.c
dengert added a commit to dengert/OpenSC that referenced this pull request Sep 21, 2020
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
    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
dengert added a commit to dengert/OpenSC that referenced this pull request Sep 9, 2023
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
dengert added a commit to dengert/OpenSC that referenced this pull request Sep 9, 2023
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
@dengert
Copy link
Member Author

dengert commented Sep 9, 2023

As suggested in #2053 (review) the 53 commits in this PR have been reduced to 19.

@Jakuje
Copy link
Member

Jakuje commented Sep 11, 2023

Last nit to commit series:

But I think we are good to merge if there will be no other comments/concerns.

dengert and others added 12 commits September 11, 2023 07:43
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
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
@dengert
Copy link
Member Author

dengert commented Sep 11, 2023

As requested, The PR was rebased:

Built with --enable-piv-sm and the run USE_PIV_SM=always ./pkcs11-tool --test --login and checked debug log. (For testing USE_PIV_SM=always was set to force use of SM over contact reader)

Built without --enable-piv-sm and the run ./pkcs11-tool --test --login and checked debug log.

Then using a ACS ACR122U contactless reader:

With --enable-piv-sm built code ran ./pkcs11-tool --test --login and checked debug log and it used SM as expected

Without --enable-piv-sm built code ran ./pkcs11-tool --test --login` as expected it returns:

./pkcs11-tool --test --login 
Using slot 0 with a present token (0x0)
Logging in to "TEST PIV CONTENT SIGNER FOR O...".
Please enter User PIN: 
error: PKCS11 function C_Login failed: rv = CKR_FUNCTION_NOT_SUPPORTED (0x54)
Aborting.

PIV card does not allow login over contactless without SM.

@Jakuje
Copy link
Member

Jakuje commented Sep 11, 2023

Thanks! I think we are good to go. @frankmorgner any other comments or good to merge?

@frankmorgner
Copy link
Member

Fine with me, thank you everyone!

@Jakuje
Copy link
Member

Jakuje commented Sep 12, 2023

Thank you! I will merge it and I think we can follow-up with the rc after that in #2792.

@Jakuje Jakuje merged commit dfeeac6 into OpenSC:master Sep 12, 2023
36 checks passed
OpenSC 0.24.0 automation moved this from In progress to Done Sep 12, 2023
Jakuje pushed a commit that referenced this pull request Sep 12, 2023
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
Jakuje pushed a commit that referenced this pull request Sep 12, 2023
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
@dengert dengert deleted the PIV-4-extensions branch September 12, 2023 11:21
@xhanulik xhanulik mentioned this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants