OpenPGP cleanup & extensions towards better OpenPGP card v3+ support #1407

marschap · 2018-06-24T11:09:43Z

Hi,,

this pull request cleans up some FIXMEs/ToDos and starts the way towards v3+ support.

In particular, it

adds DOs introduced with OpenPGP card specs v2.1, v3.0 and v3.3 resp.
updates and extends comments & references to the specs
improves parsing of the "extended capabilities" DO, including v3.x support
starts parsing the "extended length" DO introduced with v3.0
improves debugging capabilities: "symmetric" calls of LOG_FUNC_...
increases resilience by more/better error checks
recognizes the card in question in more detail [e.g. with version number] on init.

Please merge it into OpenSC master, as it paves the ground for better v3+ support.
Of course: reviews welcome.

Best
PEter

... and make it accessible for v2.1+ cards

For some files spec states CONSTRUCTED, but we treat them as SIMPLE, because we only need parts of their contents.

In addition, prepare for parsing DO 7F74, which I cannot do now, because I do not have a card supporting it.

Use it in pgp_erase_card() to slightly simplify the code.

To help debugging, - replace plain return's after LOG_FUNC_CALLED() has been called with LOG_FUNC_RETURN() - use LOG_FUNC_CALLED() & LOG_FUNC_RETURN() pairs more often

... for "standard" OpenPGP cards. This gives more detailed information to the user on the detailed specs the card adheres to. In addition it fixes a long-standing annoyance that every standard 2.x card matching the v2.0 ATR was announced as CryptoStick 1.2. This ATR is not only used in the CryptoStick 1.2, but also also in ZeitControl cards as well as NitroKeys, ...

Form a correct path instead ofmusising an array of 2 u8's. Perform proper error checking.

* length checks where needed * more & better comments

frankmorgner · 2018-06-26T12:28:53Z

src/libopensc/card-openpgp.c

- card->version.hw_major, card->version.hw_minor);
- card->name = card_name;
+ snprintf(card_name, sizeof(card_name),
+ "OpenPGP card v%u.%u (%04X %08lX)",


Why include the serial number in the card name? All other cards keep this seperately; as it is available via opensc-tool --serial. Also, I doubt that it will have some meaning for the average user...

It might be useful to differentiate between multiple tokens.

frankmorgner · 2018-06-26T12:31:30Z

src/libopensc/card-openpgp.c

- { "3b:fa:13:00:ff:81:31:80:45:00:31:c1:73:c0:01:00:00:90:00:b1", NULL, "OpenPGP card v1.0/1.1", SC_CARD_TYPE_OPENPGP_V1, 0, NULL },
- { "3b:da:18:ff:81:b1:fe:75:1f:03:00:31:c5:73:c0:01:40:00:90:00:0c", NULL, "CryptoStick v1.2 (OpenPGP v2.0)", SC_CARD_TYPE_OPENPGP_V2, 0, NULL },
+ { "3b:fa:13:00:ff:81:31:80:45:00:31:c1:73:c0:01:00:00:90:00:b1", NULL, default_cardname_v1, SC_CARD_TYPE_OPENPGP_V1, 0, NULL },
+ { "3b:da:18:ff:81:b1:fe:75:1f:03:00:31:c5:73:c0:01:40:00:90:00:0c", NULL, default_cardname_v2, SC_CARD_TYPE_OPENPGP_V2, 0, NULL },


This replaces the specific name CryptoStick with an unspecific name. Is this intended?

Yes, this was deliberate.

As mentioned in the comment to commit 20cd0c0, this ATR is the generic ATR for OpenPGP v2 cards, which is not only used in the legacy [not available on the market anymore] CryptoStick, but also in ZeitControl's OpenPGP cards, in the NitroKey devices, and most probably others too.

The Name CryptoStick for this ATR is too narrow and misleading: whenever I put my NitroKey into the USB port, or my OpenPGP card into the reader, OpenSC wrongly announced it as CryptoStick.
The mentioned commit fixes the issue.

thanks for the clearification

frankmorgner · 2018-06-26T12:31:34Z

src/libopensc/card-openpgp.c

+static const char default_cardname[] = "OpenPGP card";
+static const char default_cardname_v1[] = "OpenPGP card v1.x";
+static const char default_cardname_v2[] = "OpenPGP card v2.x";
+static const char default_cardname_v3[] = "OpenPGP card v3.x";


Don't use too much information: please remove the ".x" if you don't have any information about it.

Hmmm, I tend to slightly disagree.
In my interpretation "v3" is equivalent "v3.0"
Hence the ".x", to indicate that we not know exactly whether it is "v3.0", "v3.1", ...

If you really insist, I can remove it. But please give it a 2nd thought.

ok, whatever

frankmorgner · 2018-06-26T12:32:10Z

src/libopensc/card-openpgp.c

+ }
+
+ /* v3.0+: get card features from "general feature management" DO */
+ if ((pgp_get_blob(card, blob6e, 0x7f74, &blob) >= 0) &&


Why read that data if you don't need it?

The same is true for other parsed values or data structures, e.g. max_specialDO_size.

I'd better like to see that included when we're actually doing something useful with it. Currently, that's dead weight!

OK, understandable.

Instead of amending this PR and reverting the chanes in additional commits, I lean to calceling this one, and putting in a new one with the offending passages removed.

What's your preference?

choose as you like, thanks

marschap · 2018-06-30T11:17:09Z

I will address the chanes in a separate pull request.
Hereby closing this one.

marschap added 16 commits June 24, 2018 10:52

OpenPGP: add new DO D5 introduced with OpenPGP card spec v2.1

e4a8fc6

... and make it accessible for v2.1+ cards

OpenPGP: clarify meaning of padding byte in pgp_decipher()

b5ceb84

OpenPGP: update references to specifications

2a5c239

OpenPGP: add new DOs introduced with OpenPGP card spec v3.0 & v3.3

a2d7cca

For some files spec states CONSTRUCTED, but we treat them as SIMPLE, because we only need parts of their contents.

OpenPGP: improve parsing of extended capabilities

9e9c68c

OpenPGP: parse "extended length info" DO 7f66 on init

5a6b0e1

In addition, prepare for parsing DO 7F74, which I cannot do now, because I do not have a card supporting it.

OpenPGP: harmonize some comments

86e296a

OpenPGP: define & set LCS (lifecycle support) as extended capability

b427d03

Use it in pgp_erase_card() to slightly simplify the code.

OpenPGP: update comments on function use: ABI or internal

5898afe

OpenPGP: use LOG_FUNC_CALLED & LOG_FUNC_RETURN symmetrically

a3ab033

To help debugging, - replace plain return's after LOG_FUNC_CALLED() has been called with LOG_FUNC_RETURN() - use LOG_FUNC_CALLED() & LOG_FUNC_RETURN() pairs more often

OpenPGP: limit scope of variable

5efbf3c

OpenPGP: improve get_full_pgp_aid()'s parameter checking

d03227b

OpenPGP: fix FIXME in pgp_new_blob()

452b035

Form a correct path instead ofmusising an array of 2 u8's. Perform proper error checking.

OpenPGP: add serial number to card name

7c6f67b

OpenPGP: slightly re-factor pgp_get_card_features()

58b6d4a

* length checks where needed * more & better comments

frankmorgner requested changes Jun 26, 2018

View reviewed changes

marschap closed this Jun 30, 2018

marschap deleted the openpgp-extensions branch June 30, 2018 11:17

marschap mentioned this pull request Jun 30, 2018

OpenPGP extensions #1410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenPGP cleanup & extensions towards better OpenPGP card v3+ support #1407

OpenPGP cleanup & extensions towards better OpenPGP card v3+ support #1407

marschap commented Jun 24, 2018

frankmorgner Jun 26, 2018

martinpaljak Jun 26, 2018

frankmorgner Jun 26, 2018

marschap Jun 30, 2018

frankmorgner Jun 30, 2018

frankmorgner Jun 26, 2018

marschap Jun 30, 2018

frankmorgner Jun 30, 2018

frankmorgner Jun 26, 2018

marschap Jun 30, 2018

frankmorgner Jun 30, 2018

marschap commented Jun 30, 2018

OpenPGP cleanup & extensions towards better OpenPGP card v3+ support #1407

OpenPGP cleanup & extensions towards better OpenPGP card v3+ support #1407

Conversation

marschap commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marschap commented Jun 30, 2018