jpki/iso7816: explicitly check for PIN length != 0 on VERIFY

[frankmorgner]

If we want to verify the PIN, we don't want the status request, which results in an almost identical APDU (VERIFY APDU with no data). However, as the request for verifying the PIN results in a CASE 3 APDU, sc_check_apdu() will check for missing data for the verification so that an error is returned and no PIN bypass is possible.

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

[Jakuje]

I think this should solve the original issue we tried to solve with the pin bypass, where the zero-length PIN was accepted as valid when the card was logged in.

[dengert]

@frankmorgner @Jakuje I think the patch I sent both of you yesterday will fix this problem.

All pin commands start with sc_pkcs15_verify_pin which calls _validate_pin

Problem with e6f7373 is it added code before calling _validate_pin it should have been added after calling _validate_pin.

[Jakuje]

@frankmorgner @Jakuje I think the patch I sent both of you yesterday will fix this problem.

I think it does not handle the case when the sc_build_pin() is called from the iso7816_build_pin_apdu() when the pinpad is used, but I will be able to test it only tomorrow.

All pin commands start with sc_pkcs15_verify_pin which calls _validate_pin

Correct.

Problem with e6f7373 is it added code before calling _validate_pin it should have been added after calling _validate_pin.

I do not think you are right. The comment says "if pin cache is disabled, we can get here with no PIN data", which means that if no pin data would be present here, your checks in _validate_pin() would fail (as described in #2903 when this function is called from PKCS#15 code checking ACLs)

[frankmorgner]

We can only make this choice: We either accept that the state of a validated PIN can be captured between concurrently running processes or we force the user to enter the PIN at a PIN pad reader multiple times. e6f7373 has chosen the first path, while we now, after the CVE choose the latter (#2806, already merged in 0.24.0-rc1). Using _validate_pin earlier won't change the outcome: Either _validate_pin accepts a PIN of length 0 or it doesn't - the results are the same as in the two cases above.

This tradeoff is the reason, why Windows people insist that Windows Logon would only be possible without a pin pad reader. Only if PIN caching is possible, you can administrate concurrently running processes without extensive user interaction. Therefor, MS has created the possibility of establishing a session PIN, which may be derived from the user's PIN for caching, even if the user PIN is entered via PIN pad. OpenSC supports this with sc-hsm. However, I am not aware of a similar mechanism for PKCS#11.

That being said, I'd like to stress again, that #2806 and only #2806 fixes the possible PIN bypass due to a concurrently running application. The PR, that you are looking at here, adds a mere sanity check to avoid APDU confusion on the card driver level due to a specification which overloads the 0x20 commands. As written in the commit message, a PIN bypass is not at stake here, because the format checking on the resulting APDU (case 3 rather than case 1) will result in an error before accepting an empty PIN.

[dengert]

I do not think you are right. The comment says "if pin cache is disabled, we can get here with no PIN data", which means that if no pin data would be present here, your checks in _validate() would fail (as described in #2903 when this function is called from PKCS#15 code checking ACLs)

But if _validate_pin was called BEFORE those comments, _validate_pin checks if card driver supports PIN PAD and pinlen = 0:
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-pin.c#L275-L279

If not it https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-pin.c#L281-L284 Plus the mods in the patch I sent you will make sure pinlen != 0

So _validate_pin returns the length of the pin and it will only return 0 if its PIN PAD or SC_CARD_CAP_PROTECTED_AUTHENTICATION_PATH can be used.

Either _validate_pin accepts a PIN of length 0 or it doesn't - the results are the same as in the two cases above.

_validate_pin could be changed to set the SC_PIN_CMD_USE_PINPAD or other

Problem is pkcs11 uses NULL pin and pinlen = 0 to say use pin_pad reader. sc_pkcs15_verify_pin needs to set flags

@frankmorgner Are you saying a pin pad reader may accept a zero length pin via the pin pad?

(I did not look at what code builds the command to the pin pad reader. It may allow the min.pinlen = 0 issue.)

[frankmorgner]

If I understand correctly, then you want to propose an alternate solution for https://github.com/OpenSC/OpenSC/wiki/CVE-2023-40660 (not for this PR). Could you please provide a complete patch set in a seperate pull request? This may clearify if there is some advantage over #2806. I'm loosing track of all the individual changes...

[dengert]

I do not have a complete solution. A complete solution would require OpenSC to know that a screen unlock or login is being requested. OpenSC would have to figure out if by looking at process names and/or run as root or other info which is OS dependent and fraught with errors if OpenSC gets it wrong.

If the OS had a way a simple way to tell OpenSC screen unlock or login is being done it could work. For example login and screen unlock run with a different opensc.conf. This is also fraught with errors but the burden is on the admin if they chose to use it.

The closest thing we have today to force a pin operation regardless of login state is how PIV handles a key with CKA_ALWAYS_AUTHENTICATE. 91812cf because the card enforces it.

And we are only talking about pin pad readers, which as you point out are not used for login on Windows. They could still be used for card administration and the use of a pin pad reader could require an additional opensc.conf. The security burden is the on the admin.

[Jakuje]

I think we do not need that complicated solution. We are looking at use cases where the C_Login, it can not accept a null pin, but when we check the login status from the pkcs15init layer, it should probably not call the login at all.

I know there might still be some other corner cases in between that were not handled. Some are handled in this PR (where the verify could be mistaken for the get info).

[frankmorgner]

I'm fine with postponing this PR after the release. A review might then reveal that it is not needed anymore.

[Jakuje]

Given that we do not have the debug traces from the initial PIN bypass with PIV, it is still hard to guess was happening there. I would be for getting this change into 0.24.0 as I do not believe it will break anything and it might be one of the cases which might happen and make the bypass happen by mistaking the get pin info for the login APDU.

[frankmorgner]

Given that we do not have the debug traces from the initial PIN bypass with PIV, it is still hard to guess was happening there.

I was reproducing and analyzing the issue back then. When the user wanted to unlock the screen, the following happened:

1. login screen was presented
2. user clicked "login" without entering a password.
3. OpenSCToken received query to verify the PIN ""
4. OpenSCToken called sc_pkcs15_verify_pin
5. sc_pkcs15_verify_pin recognized PIN length == 0, checked the login status ("logged in") and returned SC_SUCCESS.
6. Successful login was propagated back to the upper layers.

Additionally, I was able to reproduce the same issue with Firefox and a Yubikey, because the PIV driver did not really logout when a PKCS#11 application terminated.

[Jakuje]

Thanks for clarification! I probably missed that part (or already forgot). I have now installed 0.23.0 and from what you describe, I can reproduce this now with pkcs11-tool --login -O in one process and firefox with empty pin in the other (even with the SIGN key, which should enforce always authenticate?!). In any case, it should be fixed with both #2806 and #2916.

[frankmorgner]

The target of this PR is to detect an APDU confusion, where a request for a PIN verification is downgraded to a status query.

Please move the discussion about the more complex problem of avoiding multiple PIN verifications to either #2916 or #2903

[Jakuje]

Merging. Thanks!