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

Workaround Yubikey bug with SELECT DATA #2969

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ya-isakov
Copy link
Contributor

@ya-isakov ya-isakov commented Dec 25, 2023

There is a known issue with Yubikey tokens, that they're requiring another length byte in APDU for SELECT DATA:
Yubico/yubikey-manager#403

Let's workaround this, by checking return value of SELECT DATA, and if error indicates that parameters are wrong, let's try Yubikey-specific ones.

Checklist
  • Documentation is added or updated
  • New files have a LGPL 2.1 license statement
  • PKCS#11 module is tested
  • Windows minidriver is tested
  • macOS tokend is tested

There is a known issue with Yubikey tokens, that they're requiring
another length byte in APDU for SELECT DATA:
Yubico/yubikey-manager#403

Let's workaround this, by checking return value of SELECT DATA,
and if error indicates that parameters are wrong, let's try
Yubikey-specific ones.
@ya-isakov ya-isakov mentioned this pull request Dec 25, 2023
3 tasks
@Jakuje
Copy link
Member

Jakuje commented Dec 28, 2023

Thanks for the PR! Isn't there a way to detect yubikey before we will get the error to send the right APDU right away?

@ya-isakov
Copy link
Contributor Author

Very good question - I tried to find the way. It seems that in yubikey-manager, they use Yubikey-specific command to detect firmware version (https://github.com/Yubico/yubikey-manager/blob/main/yubikit/openpgp.py#L1018), which, I thought, is not the best approach for OpenSC. I cannot find any generic way to determine vendor, unfortunately.

@dengert
Copy link
Member

dengert commented Dec 30, 2023

Using a Yubikey with OpenPGP applet to get ATR and version:

./opensc-tool --card-driver openpgp -a -s "00:f1:00:00:03"
Using reader with a card: Yubico YubiKey CCID 00 00
3b:f8:13:00:00:81:31:fe:15:59:75:62:69:6b:65:79:34:d4
Sending: `00 F1 00 00 03`
Received (SW1=0x90, SW2=0x00):
04 02 06 ...

Paste the ATR into https://smartcard-atr.apdu.fr/
The historic bytes 59 75 62 69 6B 65 79 34 spell "Yubikey4" The "Y" is also "proprietary format"
Then sending 00 F1 00 00 03 returns version number.

With PIV applet can also get version number sending 00 FD 00 00 03

@Jakuje
Copy link
Member

Jakuje commented Jan 2, 2024

Well. We already have a yubico_version as part of the PIV private structure:

int yubico_version; /* 3 byte version number of NEO or Yubikey4 as integer */

Would using that to detect yubikey work?

@dengert
Copy link
Member

dengert commented Jan 2, 2024

I believe it would for OpenPGP applet. Just a different command. Yubikey PIV applet uses 00 FD 00 00 Yubikey OpenPGP applett uses 00 F1 00 00 as shown in example in #2969 (comment)

@ya-isakov
Copy link
Contributor Author

Hmm, is this ATR standard command? I did not want to use vendor-specific commands to detect version etc, to not cause issues with other tokens.

@Jakuje
Copy link
Member

Jakuje commented Jan 4, 2024

Hmm, is this ATR standard command? I did not want to use vendor-specific commands to detect version etc, to not cause issues with other tokens.

I would say it should be safe to assume the token is yubico if it is in the ATR historic bytes.

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Jan 4, 2024

I was thinking, that there is already ATR matching, by pgp_atrs, and there is even flags field - so, I can add flag there, to allow Yubikey-specific commands/workarounds. But, I cannot find where these flags are read or passed.

I'll write PoC, and will push as a separate commit into this PR.

@dengert
Copy link
Member

dengert commented Jan 4, 2024

I was thinking, that there is already ATR matching, by pgp_atrs, and there is even flags field - so, I can add flag there, to allow Yubikey-specific commands/workarounds.

https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-openpgp.c#L305-L315 will match a card by ATR, or in lines https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-openpgp.c#L320-L325 will match a card if it has a OpenPGP applet. In either case, The ATR Historical bytes can be found at: card->reader->atr_info.hist_bytes

The ATR is for the card. A Yubikey card can have both a openpgp applet and a piv applet. (OpenSC can only use one at a time.
based on first driver to find its applet will be used. See OPENSC_DRIVER= env or in opensc.conf)

card-piv.c checks the Historical bytes here:
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-piv.c#L5354-L5385

It the sets a card type. But with OpenPGP the card type is based on version of OpenPGP applet. You could just set a flag saying card is a Yubikey and later call a routing like
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-piv.c#L5481-L5497 to save the Yubikey version.
Note that with PIV applet, line https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-piv.c#L5485 it uses "0xFD"
but with OpenPGP applet you would use "0xF1" to get the version.

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Jan 4, 2024

Please, check new commit - I added new flag, used it to select proper pgp_select_data..., and in pgp_select_data_yubikey I'm checking for Yubikey version, to see if it should try Yubikey-specific ATR. I'm a bit confused, that my APDU seems to be different from what @dengert shown - I'm sending 00 F1 00 00 01 03 03 (this is what sc_format_apdu_ex created, and I cannot find way to put data in sc_format_apdu), not 00 F1 00 00 03, but it seems, this one is working, too - it returns proper versions for both of my Yubikey tokens.

It works on both of my Yubikey tokens, selecting proper data, but I cannot test it on other vendors, unfortunately.

@dengert
Copy link
Member

dengert commented Jan 4, 2024

SC_CARD_FLAG_YUBIKEY_SELECT should not be in opensc.h. It should not be a new card type.

The point of reading the Yubikey version, is that Yubikey as fixed problem in 5.4.3 and OpenSC could then handle old and new cards as needed.

You could simplify the code by saving the version in https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-openpgp.h#L171-L190
In openpgp_init() test ATR historical bytes for Yubikey and call pgp_yubikey_get_version(card, &priv->version);

Then test in pgp_select_data if (priv->yubikey_version > 0 && priv->yubikey_version < 0x050403) change the string to be sent (and the length).

You would not need SC_CARD_FLAG_YUBIKEY_SELECT or special pgp_select_data_yubikey

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Jan 4, 2024

@dengert

It should not be a new card type.

It is not a card type, it's a flag

In openpgp_init() test ATR historical bytes for Yubikey and call pgp_yubikey_get_version(card, &priv->version);

Why to test for historical bytes 2nd time, if there is already historical bytes check via pgp_match_card? select_data seems to be available only in OpenPGP v3, so, there is no point to check for Yubikey version for Yubikey 4 and earlier, right?

It seems that historical bytes for Yubikey 5 are different from Yubikey 4, so, it won't help much, to have historical bytes check, to detect version of firmware on all Yubikeys

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Jan 4, 2024

So, yeah, in Yubikey 5, last bytes of ATR are 59:75:62:69:4b:65:79 instead of 59:75:62:69:6b:65:79:34 - so, YubiKey (with big K), instead of Yubikey4

The whole historical bytes for Yubikey5 looks like this 80:73:c0:21:c0:57:59:75:62:69:4b:65:79 - I'm not sure, if I should compare with this whole line? It might have some settings in it...

@dengert
Copy link
Member

dengert commented Jan 5, 2024

It is not a card type, it's a flag

Then add a flag that is only used in OpenPGP driver in card-openpgp.h

If you did want to have a new card type for OpenPGP card driver, add it in https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/cards.h#L107
But note that the card types refer to the OpenPGP applet versions. The problem is Yubico had a bug in there version
of applet.

Why to test for historical bytes 2nd time,

Because if the ATRs is not matched, the code then tests for OpenPGP applet at
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-openpgp.c#L320-L325
This will then work with newer cards or OpenPGP applets on other cards not listed in the driver.

The whole historical bytes for Yubikey5 looks like this 80:73:c0:21:c0:57:59:75:62:69:4b:65:79 - I'm not sure, if I should compare with this whole line? It might have some settings in it.

You could almost copy this code to test for Yubikey cards:
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-piv.c#L5348-L5385

Also look at https://smartcard-atr.apdu.fr/ and paste in the ATR to see what is in it.

What cards do you have?

@dengert
Copy link
Member

dengert commented Jan 6, 2024

@ya-isakov While looking at card-openpgp.c for #2952 I notice pgp_get_card_features called from pgp_init parses the ATR historical bytes expecting the card to follow the OpenPGP standards and provide some additional information.

But of the 3 Yubikey devices listed in pgp_atrs on the last 3b:fd:13:00:00:81:31:fe:15:80:73:c0:21:c0:57:59:75:62:69:4b:65:79:40 Yubikey 5 NFC does not use the proprietary format, but uses the compact TLV data object which includes some extra features, and card issuer's data of "YubiKey". The code should be able to handle future Yubikey devices with different ATRs even if not listed in pgp_atrs

I would suggest the code be modified here to parse further for Yubikey and to then also read the Yubikey version number to handle the bug you are trying to address.

@frankmorgner
Copy link
Member

@ya-isakov will you have a second look at the suggestions above?

@ya-isakov
Copy link
Contributor Author

@frankmorgner Sorry, I don't have time now to do it as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants