Various PIV changes

[dengert]

Some ActivIdentity CAC/PIV cards lose the login state when selecting
the PIV AID SC_CARD_TYPE_PIV_II_CAC and CI_PIV_AID_LOSE_STATE were added
so piv_card_reader_lock_obtained will try and do a SELECT PIV AID.

card->type is reset to -1 if piv_match_card_continued fails to
match a card as PIV.

pkcs15-piv.c now uses sc_card_ctl which checks card->ops->card_ctl for NULL.

Date: Sat Mar 31 08:46:29 2018 -0500
On branch various-piv-changes
Changes to be committed:
modified: card-piv.c
modified: cards.h
modified: pkcs15-piv.c

Checklist

• Documentation is added or updated
• New files have a LGPL 2.1 license statement
• Tested with the following card:
  • tested PKCS#11
  • tested Windows Minidriver
  • tested macOS Tokend

[dengert]

@dpward @mouse07410, Please test this with your cards.

It should solve:
#1292 #1297

[dpward]

@dengert Thanks for this change — I will test.

However I see you are adding a card type called SC_CARD_TYPE_PIV_II_CAC. I should mention that to more generally determine whether a PIV is also a CAC, the method for doing this is actually described in section 3 of the DoD Implementation Guide for CAC PIV End-Point — there is a flow-chart of the card discovery steps.

I realize this might not be the correct test to determine whether the SELECT PIV AID command should be suppressed; but in the context of the comments in #841, this information might be useful to discover which driver should handle a card.

[dengert]

SC_CARD_TYPE_PIV_II_CAC may be misleading. It means the historical bytes in the ATR match what was in the document you sent and your card. You card appears to not honor the PIV requirement that selecting the AID will not lose the login state. So it not really saying anything about CAC.

Many cards can have a PIV applet and one or more other applets like OpenPGP on the card.
Yours appears to support CAC and PIV.

You said: "...useful to discover which driver should handle a card". That is really up to the user. CAC and PIV may present different partially overlapping views of the same data and keys on the card. But each applet may present unique data and keys too.

Currently OpenSC will select a driver based on the internal list of drivers or from the opensc.conf. First driver that can handle the card is selected. There are standards for the applets, CAC has one and PIV has another. The special case of the SELECT FILE APDU is used to select an AID, i.e. which applet should be active This usually means the security status is reset when an applet is deselected. Each applet may support different APDU commands, with only the SELECT FILE for AID being incommon.

But many people want to use more the one applet at a time or use different applets from different applications. The p11kit can do some of this with pkcs11 using different pkcs11 modules from the vendors. (https://github.com/dengert/OpenSC/tree/pkcs11-multi-application is a first attempt to have OpenSC with pkcs11 to support one card with multiple OpenSC drivers.)

I have seen DoD Implementation Guide for CAC PIV End-Point It is more for the card developer. Once the PIV AID is selected, the card should be following the PIV card edge standards.

My goal in OpenSC is to support the PIV driver, for real PIV cards. And if possible for PIV applets to ruin from the same card as other applets.

What we are seeing with your card is it does not follow all the PIV standards. The change I proposed in
#1307 should make it work the way it did before. Can you test it?

[frankmorgner]

I think you should save the value of card->type before modification, and use the original value here. The user may configure the type by using opensc.conf.

[dengert]

piv_match_card may be called with card->type = -1 or some value set by the user.
If the card->type is not -1 or one of the SC_CARD_TYPE_PIV_II_* it returns 0 and does not change card->type. i.e. it will continue only if uses said it was PIV or unknown type of card.

It then calls piv_match_card_continued which will try to read the discovery object or do a SELECT AID for PIV. Only if if this fails will card->type will be set to -1. But this gets to this point if card->type was -1 or SC_CARD_TYPE_PIV_II_. No other driver should be looking to process a SC_CARD_TYPE_PIV_II_

If piv_match_card_continued succeeds, piv_match_card returns with card->type as one of the
SC_CARD_TYPE_PIV_II_* The card->type is passed along, so the card commands do not have to be repeated in piv_init to try and determine the type of card. So the information of card->type is being passed from piv_match_card to piv_init.

When piv_init is called, it calls piv_match_card_continued again, to complete setting things up.

As per our discussions in #1256 about a piv_match_card passing information to piv_init
you did not like passing a lock and card->data as a pointer to allocated memory. So I changed it to have piv_match_card do nothing and always return a match. piv_init did all the real matching. You did not like that either, so I changed it to what is in the current code, passing only the card->type between piv_match_card to piv_init.

There is no place to store any information from piv_match_card to piv_init other then somewhere in the card structure. So we are back to where we were in #1256. As you pointed out the current code is to complicated. The original #1256 was much cleaner but depended on a successful
piv_match_card would always be followed by a call to piv_init.

[dengert]

Will you accept this 2 line change to card.c to address the card->type issue?
The code already resets the card->driver, it should reset the card->type too in case other drivers may return SC_ERROR_INVALID_CARD and may have modified card->type.

diff --git a/src/libopensc/card.c b/src/libopensc/card.c
index 194634b..5b23b17 100644
--- a/src/libopensc/card.c
+++ b/src/libopensc/card.c
@@ -269,6 +269,7 @@ int sc_connect_card(sc_reader_t *reader, sc_card_t **card_out)
                for (i = 0; ctx->card_drivers[i] != NULL; i++) {
                        struct sc_card_driver *drv = ctx->card_drivers[i];
                        const struct sc_card_operations *ops = drv->ops;
+                       int saved_type = card->type;
 
                        sc_log(ctx, "trying driver '%s'", drv->short_name);
                        if (ops == NULL || ops->match_card == NULL)   {
@@ -292,6 +293,7 @@ int sc_connect_card(sc_reader_t *reader, sc_card_t **card_out)
                                sc_log(ctx, "driver '%s' init() failed: %s", drv->name, sc_strerror(r));
                                if (r == SC_ERROR_INVALID_CARD) {
                                        card->driver = NULL;
+                                       card->type = saved_type;
                                        continue;
                                }
                                goto err;

The above code is the only place where a card structure is reused and passed to more than one driver. In my original version of #1256 I passed the card->data (and the lock) because a successful match_card is always followed by a call to init.

[dengert]

looking at code, card-mcrd.c can also set card->type in *-match_card, and *_init can return with SC_ERROR_INVALID_CARD. The patch to card.c would prevent problems with card-mcrd.c

[dengert]

Commit 81adb58 contains the patch for card.c to reset the card->type.

[frankmorgner]

There is no need to handle this complexity in card.c; please solve the issue with the card's type in card-piv.c. The implicit contract here is to not modify the output (the card object) unless you know that everything is fine. Though these kind of problems have been introduced in the past, instead of replicating we should fix them. Note that the mcrd driver is disabled by default.

[dengert]

The PIV code needs to pass some information between piv_match_card and piv_init to avoid duplicate
code and APDUs to the card.

The original version used the card->drv_data and it also passed a lock after doing some APDUs to the card to test for a PIV applet. The lock kept all the initial ADPUs in the same PCSC transaction. You did not like that.

The next version piv_match_card always matched and let the piv_init do all the checking. You did not like that.

The current version of this PR split up the code so piv_match_card does some checking and sets the card->type but usies multiple PCSC transactions. You do not like that.

So where can I save some information to pass between piv_match_card and piv_init?

If 81adb58 is not acceptable I could go back to card->drv_data without the lock.

Looking closer at other card drivers, it looks like the card-cac.c in cac_match_card passes both card->drv_data and card->type. (look for "ACA found, is CAC-2 without CCC" It also looks like it duplicates some APDUs and code for other CAC cards). So it could also cause #1297. The PIV driver precedes the CAC driver in the list of drivers. I still prefer 81adb58 for both drivers and any other future drivers.

[frankmorgner]

Currently, you shouldn't share information, which is a design decision from the previous authors. If you think that the separation between match_card and init is nonsense (I would understand that), please make a separate PR that covers all cards and reduces the overall complexity of OpenSC. I've written that multiple times before, I don't know how else to put it.

If you choose to only look at PIV and PKCS#11, I suggest you use a SELECT in match_card, this costs you only 100 ms on an applet based card. If you want to be even faster, use the ATR.

Please stick to solving the problems in question (the lack of speed has never been one of them).

[mouse07410]

If you want to be even faster, use the ATR.

As has been mentioned in another PR, unfortunately the ATR is not a reliable way to tell, thanks to multiple applets.

[dengert]

I disagree with your statement: "Currently, you shouldn't share information, which is a design decision from the previous authors."

I know of no documentation that supports "you shouldn't share information". In a project like OpenSC where there is little or no documentation, commit messages and the code are the documentation. On the contrary, commits and code show card->type and card->drv_data are the way a driver can share information among the driver's routines, as intended by the previous authors.

The match_card and init routines are part of the card's sc_card_operations and are part of the card driver. When card.c calls card->ops->match_card(card) and it returns 1, card->ops->init(card) is ALWAYS called so init can clean up after match_card

git blame opensc.h shows these two lines in sc_card_t:

69d2e901 (aet                 2005-02-06 19:40:40 +0000  494)   int type;                       /* Card type, for card driver internal use */
3fa1b277 (jey                 2002-02-24 19:32:14 +0000  514)   void *drv_data;

Scroll over to the the comment on type. /* Card type, for card driver internal use */ type was introduced for the use of the driver. Many cards drivers have multiple types defined in cards.h card->type is where they store them.

3fa1b27 renamed ops_data to drv_data. It does not say much about this but drv_data has been used by all drivers to store specific driver information.

I have never used the word nonsense in regards to this issue. But a believe that the code and commit messages show that the developers expected both card->type and card->drv_data can be used to share information between all a driver's routines. The use of card->type being for the "driver internal use" and card.c setting it based on the config file and expecting it to not be changed is a bug in card.c.

In an attempt to get 0.18.0 out I still suggest the 72a1fc3 is needed to fix this and will work with all card drivers. I can submit 72a1fc3 as separate PR if you insist with most of the above comments, or you can just commit it as is.

I have no intention at this time to try and rewrite the card driver selection process. But look at
pkcs11-multi-application which builds on the current card.c (This would not be for 0.18.0 and is only experimental.)

I understand that you may disagree with the card->type but not with the use of card->drv_data. If needed, I will modify card-piv.c one more time to allocate card->drv_data in piv_match_card to pass to piv_init

[dpward]

SC_CARD_TYPE_PIV_II_CAC may be misleading

Can we please call it something else? The code is comparing historical bytes to determine if the card is made by G+D — not if the card is a CAC. What about SC_CARD_TYPE_PIV_II_GI_DE?

(Everything else makes sense, thanks!)

[dengert]

OK, I can change it. Can you test it the way it is?

[dpward]

This still isn't working for me — I believe because this comparison is off by one byte?
73 66 74 = "sft"
66 74 65 = "fte"

So from the logs, it still finds a SC_CARD_TYPE_PIV_II_GENERIC:

0x7f14c086b880 16:48:59.963 [opensc-pkcs11] card.c:317:sc_connect_card: card info name:'Personal Identity Verification Card', type:14001, flags:0x0, max_send/recv_size:255/256

[dpward]

I edited the code locally, to compare the first 4 historical bytes against 73 66 74 65 or "sfte". Now everything seems to work — pkcs11-tool, OpenSSL + pkcs11_engine, Firefox, and Thunderbird.

[dengert]

I have updated the branch to rename SC_CARD_TYPE_PIV_II_CAC to SC_CARD_TYPE_PIV_II_GI_DE and fix the compare of the historical bytes.

I also added a commit to have card.c save and restore on failure the card->type while searching for a driver.

[mouse07410]

@dengert I tried your latest changes on Mac and virtualized CentOS 7 - and it worked with CAC fine.

72a1fc3 solves the problem.

[dpward]

This still works for me as well.

(I would suggest using git rebase to squash e9dce6a and 4cd66cc into a single commit before the PR is merged, so that these changes are easier to follow.)

[frankmorgner]

saving/resetting the card type should be done in card-piv.c.

Apart from that, the PR looks good.

[frankmorgner]

bad indenting

[frankmorgner]

merged with my requested changes, tested with a default card and yubikey neo

[dengert]

While writing my comment #1307 (comment) all morning, I see you have committed some different changes.
But I stand by what I said.