Improve pinpad support and PIN length validation

[nunojpg]

Currently slots without user_consent fail pkcs11-tool test (--test) in the following situations:

• Pinpad is in use. pkcs11-tool is not triggered by CKA_ALWAYS_AUTHENTICATE to reauthenticate, but it is needed since OpenSC will not cache the pin.
• Number of authentications required is above the current (pin_cache configuration + 1). pin_cache is by default 10 and pkcs11-tool --test requires, at least for my test card, 12 authentication attempts.

This pull request fixes the first issue by checking also for CKF_PROTECTED_AUTHENTICATION_PATH.
It doesn't fix the second issue.

I believe OpenSC pkcs11 lib should dynamically report CKA_ALWAYS_AUTHENTICATE as (user_consent || pin_cache_expired).

When the pin_cache is expired (also seen as "expired" if it is always disabled), from the pkcs11 layer a next attempt without login will fail, so it makes no sense to be reporting CKA_ALWAYS_AUTHENTICATE as 0.

[dengert]

The attribute is CKA_ALWAYS_AUTHENTICATE. There is no CKF_ALWAYS_AUTHENTICATE. Please change this in the commit message.

"OpenSC will not cache the pin" implies the code could be changed to cache the pin.
But with a pin pad reader, the pin is never sent to the host, the above should read: "OpenSC can not cached the pin."

I am not sure what you mean by:" so it makes no sense to be reporting CKF(A)_ALWAYS_AUTHENTICATE as 0." CKA_ALWAYS_AUTHENTICATE is an attribute of the private key, and the way I read the specs, it is not expected to change during a session or as long as the key does not change. Some applications may query the key only once, but try and use it multiple times. Having PKCS#11 "dynamically report" it will not work correctly with these applications.

I believe the case is already handled, by this statement in PKCS#11:
"Failing or omitting to reauthenticate when CKA_ALWAYS_AUTHENTICATE is set to CK_TRUE will result in
CKR_USER_NOT_LOGGED_IN to be returned from calls using the key."
I believe this is what the OpenSC pin cache is expired, and CKR_USER_NOT_LOGGED_IN can be returned by many functions for different reasons and applications know how to handle it.

The patches look good, but need some testing. I will try in the next few days with PIV card that enforces CKA_ALWAYS_AUTHENTICATE on one of its keys.

[nunojpg]

Fixed CKF to CKA on the commit message and comment above.

I need to explain better. Problem only exists when using a slot WITHOUT CKA_ALWAYS_AUTHENTICATE and requiring multiple authentication attempts.

Eg, run pkcs11-tool --login --test:

CKA_ALWAYS_AUTHENTICATE	FALSE	TRUE
With pinpad	OK after this pull request	OK
Without pinpad	Fails because pkcs11-test --login --test requires 12 logins and by default pin_cache_counter = 10.	OK

I understand CKA_ALWAYS_AUTHENTICATE does not mean according to the spec "hey, this slot does not enforce user_consent, but since you are going over pin_cache_counter, you really need to authenticate next time or this will fail."

But, afaik, there is no other way to signal this behavior to a pkcs11 application. Also, I don't see any problem on doing that, as CKA_ALWAYS_AUTHENTICATE would only be dynamically asserted if the application is about to trigger a login fail because it consumed the pin_cache completely.

[dengert]

Pin caching is not a PKCS#11 concept, but rather an OpenSC concept. You are trying to solve the problem of what happens when pkcs11-tool does too many operations. I would says that CKR_USER_NOT_LOGGED_IN should be handled by pkcs11-tool. Its not. Every PKCS#11 call in the test routines does:

if (rv != CKR_OK)
   p11_fatal("C_xxx", rv);

I believe CKR_USER_NOT_LOGGED_IN is handled by NSS, that requests the PIN and retries the operation. I am worried that your approach of setting CKA_ALWAYS_AUTHENTICATE for the key could confuse other applications.

For the testing of multiple operations in pkcs11-tool, it could test rv == CKR_USER_NOT_LOGGED_IN and report error, and retry a login again if it has the pin, or also prompt for the pin. A macro could be defined for this. I would only put the retry in the tests not in other parts of pkcs11-tool.

On the other hand I would like to hear what @frankmorgner and @viktorTarasov or others have to say about this.

[nunojpg]

Your solution looks the best. I can implement it if no other comments arrive. I actually considered that option but then got it lost while debugging the pinpad problem and also still learning how all the different layers articulate.

[martinpaljak]

Pin cache can be implemented in the app (pkcs11 tool) or module. I think it is best to leave it a feature of the module. Triggering pin entry on an unexpected not logged in exception should be handled by the tool, and for your test case where pin is needed 12 times, you should get 12 pin prompts from the app without pin cache turned on, the same way you get 12 prompts on the pinpad. The whole pin cache topic is a bit hairy, i find a test run with a pinpad the best design verification - it is not a value to be always passed around and stored for further reference, but something occasionally shared between the cardholder and the card. Martin

On Thu, 26 Jan 2017, 18:18 Nuno Goncalves, ***@***.***> wrote: Your solution looks the best. I can implement it if no other comments arrive. I actually considered that option but then got it lost while debugging the pinpad problem and also still learning all the different layers articulate. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#948 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACztpwSYSrIwDMGhBSn0jGsDbFf3QRAks5rWNVsgaJpZM4LuqNa> .

-- typos expected due to mobile device

[frankmorgner]

1bc09dd disables the login if (sessionInfo.state & CKS_RO_USER_FUNCTIONS) == 0) though the user used --login. I don't see why pkcs11-tool should not follow the user's choice even though this may mean that she is prompted several times at her pin pad reader. I agree with @martinpaljak for not accepting this change.

183c493 doesn't change the logic but makes the code more consistent and readable. This change is good.

[nunojpg]

1bc09dd does nothing. It was just trash I pushed. Don't make me more embarrassed over it.

I tested it badly and it looked to have effects, but it doesn't. I was even touching parts of the code I didn't understand well, so I didn't explain properly the reason for the change, and now I can't also explain well the reason to revert it, except that it doesn't do anything good, in any situation.

If you care to mention a specific case were it does anything, I will be very happy to withdraw the revert request for my own code :)

@frankmorgner, I fail to see where do you agree with @martinpaljak in the sense of not accepting any of this commits.. I also agree with him, but I don't see that it affects any of this commits and also the way forward to solve the case that still fails, as discussed with @dengert.

[dengert]

I would revert 1bc09ddafa7e7c44a62226b2c2380e558bee9360it too. login(sess, CKU_CONTEXT_SPECIFIC) should only be called when already logged in AND the code is about to use a key which has the CKA_ALWAYS_AUTHENTICATE. Some PKCS#11 implementations might complain if it is used at the wrong time.

[frankmorgner]

I agree with martin that if you want to --login doing the --test, then you should get 15 pin prompts instead of skipping tests that require authentication.

[nunojpg]

Hi, this pull request have been updated to also solve some pin length validation issues.

The table at #948 (comment) is still valid.

This pull request solves the failure when CKA_ALWAYS_AUTHENTICATE=FALSE and a Pinpad is present.

It also still doesn't fix the case "Fails because pkcs11-test --login --test requires 12 logins and by default pin_cache_counter = 10.".

I will pull request a fix for that after since it is unrelated. (To be clear: it is not a regression of this pull request).

@frankmorgner

I agree with martin that if you want to --login doing the --test, then you should get 15 pin prompts instead of skipping tests that require authentication.

This pull request doesn't have any impact on that...

[frankmorgner]

To be consistent with the existing PIN checking in sc_pkcs15_change_pin and sc_pkcs15_unblock_pin, this call belongs into sc_pkcs15_verify_pin

[nunojpg]

Ok...have to take a look at it, because _validate_pin appears immediatly broken:

/* if we use pinpad, no more checks are needed */
if (p15card->card->reader->capabilities & SC_READER_CAP_PIN_PAD)
	return SC_SUCCESS;

This is testing if we have a pinpad, and not if we are actually using a pinpad (only if pinlen is 0). Have to check how this impacts the other callers.

[frankmorgner]

why aren't you logging in here anymore?

[nunojpg]

This is a revert commit because this code doesn't have any effect. Checking for a RW session doesn't check if we are currently logged in (if a operation that requires a login will succeed). This test is equivalent to opt_login==true && opt_login_type == CKU_USER. So it only checks that a login was made in the beginning, and so we should run this test. If the current login expires it will still fail with a error. This checks are only here to skip tests that should not be attempted if the user didn't used the option --login.

[nunojpg]

@frankmorgner updated according to your comments.

[frankmorgner]

thanks, good work!