Do not require repeated authentication in the pkcs15init layer when checking ACLs #2916
Conversation
|
I still believe that a clean solution would track the login state on an upper layer. In PKCS#11, one should leverage slot->login_user; in pkcs15-init propably verify_pin should store some global authentication state that is used in the lower layers, and so on. If we apply the suggested solution, we cannot detect whether sc_pkcs15_get_pin_info returned "logged in", because the this process verified the PIN or because some other concurrently running process logged in. That being said, maybe this risk is acceptable during the enrollment. Since I don't have a quick alternative solution, we may need to accept this as is. |
|
How about if a process has not done a verify_pin, a check for login state would always return not logged in. Once a verify pin was successful, a check for login state would do what it does now. This flag would be saved per pin. If logout or disconnect was done, this would reset the flag to force the next verify to A login process with or with or without PIN PAD reader would then always endup having to send a verify pin command. Should also work for screen unlock. |
This sounds to me as a best proposal so far. I had something like that in my mind, but did not manage to put it into the code yet. Do you want to propose a change for this or should I look into that? |
|
I can confirm that my test is working now. |
Why don't you look at it. Question would this be done in PKCS11 layer vs PKCS15 layer. In #2903 script calls multiple pkcs15 tools so each process ends up testing card for login state. If done at pkcs15 layer, user could set environment variable to use the old way in scripts or when calling |
I would say in pkcs15. The pkcs11 itself already has login state tracking as part of session if I am right. And I do not think there is a way to get trough readonly to authentication session without C_Login().
Yes, but this is a setup script without pinpad yet. The logs that we looked at were produced by the following steps, where the keys are generated, for example as provided by Peter, which was single process, single call to pkcs15init and which was prompting for pins needlessly. |
Correct. but PKCS15 can ask a driver for the login state:
No, but for example: pkcs11-tool.c will only prompt user https://github.com/OpenSC/OpenSC/blob/master/src/tools/pkcs11-tool.c#L1816 and then will send a pin from --pin or NULL https://github.com/OpenSC/OpenSC/blob/master/src/tools/pkcs11-tool.c#L1855-L1858 This should then cause PIN PAD reader to prompt for pin. Other applications may have similar code. Problem was in order to cut out PIN PAD prompts e6f7373 would call sc_pkcs15_get_pin_info and some cards would query the card for the login state. Works well if user applications are sharing the login state. But causes login or screen saver to avoid checking if real user is still at the workstation. So user sharing card login state vs is this the real user unlocking screen or logging are different. |
Jakuje
force-pushed
the
bypass-pkcs15init
branch
from
b07ccef
to
d60f58e
Compare
|
Added explicit tracking of the login state in the pkcs15 layer. Needs more testing, I hope I will be able to do tomorrow. |
There was a problem hiding this comment.
my suggestion would be add a list of PINs that have been verified to struct sc_profile. If I read the code correctly, then the profile is available in all calls of the pkcs15init layer and it may be used to carry such volatile information. This could look like this:
struct sc_profile {
....
struct {
unsigned int type;
int reference;
int verified;
} pin_status[SC_PKCS15_MAX_PINS];
...
}
- If
sc_pkcs15init_verify_secret()was successful, then savetypeandreferencein the first unused triple and setverifiedto1. - Before calling the PIN verification with a PIN pad in
sc_pkcs15init_verify_secret()(in both cases,pin_objand!pin_obj), then iterate through the list and check if the secret (type/reference) has beenverifiedbefore and the PIN is still verified. If so, do not request the actual PIN verification at the PIN pad and continue with the rest.
src/pkcs15init/pkcs15-lib.c
Outdated
| if (r == SC_SUCCESS && | ||
| auth_info.logged_in == SC_PIN_STATE_LOGGED_IN && | ||
| auth_info.pin_verified == 1) { | ||
| LOG_FUNC_RETURN(ctx, r); |
There was a problem hiding this comment.
in case of a failure, we should still try to call sc_pkcs15_verify_pin() as below
There was a problem hiding this comment.
afaik that is what is done now. We return only if we got the info, we are logged in according to the card and this PIN was previously provided.
What would be the advantage against tracking this inside of the auth (PIN) structure as I proposed in this PR?
I think that the PIN does not need to be verified through the
The case |
|
Before going into implementation details, I stumbled across your analysis of the log here:
If I understand correctly, then, at least in the case of using pkcs15init via PKCS#11, then the required PIN is always verified before. So why, actually, should we bother of tracking the authentication place in the first place? In pkcs15init, we could add So maybe we don't need any complicated state tracking after all and just add sc_pkcs15_get_pin_info() to pkcs15init directly without modifying anything else... |
|
I have quite limited knowledge of the pkcs15init and its works from inside to the pkcs11, basically limited only to the myeid card's case we are solving now. Likely there will be other ways in other drivers that will bite us later.
That is my understanding. At least after we removed the pin bypass in the pkcs15-pin.c, which is called from the pkcs11, there should not be any other way to bypass from the readonly session to RW session, which is required to access all pkcs15init operations (did not check all but for example
My thinking would be that this would allow pin bypass in the pkcs15init tool, but that might not be an issue as the pkcs15init usually does not enforce the ALC, until the
I am not against. I can drop the top commits and keep only 40f4b72 which was already confirmed that it solves this particular issue, if you agree. I am all in for simpler solutions. |
I only have limited knowledge as well, and would be happy to see more release testing on pkcs15init. Considering that e6f7373 was specifically introduced with myeid in mind, I think that bad side effects for other cards are unlikely if we basically just move the call of
Yes, please do so and I'd be happy if @larssilven could try this out then. |
The original code block from e6f7373 is still needed when pkcs15init layer checks ACLs for PKCS#15 objects, but it should be kept out of the libopensc, which is used for more authentication code paths and can be used for PIN bypass.
Jakuje
force-pushed
the
bypass-pkcs15init
branch
from
d60f58e
to
a0b6223
Compare
|
Done. @larssilven can you double-check? |
|
EDIT: Please consider the text below irrelevant, I did the test in the wrong container.
|
|
I have tested this a0b6223 (jakuje/bypass-pkcs15init). Had some problems to find it. |
|
I modified my test application to not call C_Login before C_GenerateKeyPair. Then if C_GenerateKeyPair fails with CKR_USER_NOT_LOGGED_IN: C_Login will be called and the C_GenerateKeyPair will be called again.
|
|
This "problem" occurs due to the overloaded semantics of PIN length == 0. OpenSC/src/pkcs15init/pkcs15-lib.c Lines 3946 to 3951 in 8fc2c20 This approach dates back at least 11 years and I'm not sure if we should remove it. One could argue that we should remove it, because we now have a better understanding on how to plug things together, also with a PIN pad reader. On the other hand, you could also argue to leave it to keep backward compatibility - since the user needs to authorize the transaction with a PIN (on the PIN pad), she still is in full control. |
|
@frankmorgner |
|
Thanks for verbose testing! It was also my concern that asking for a pin on pinpad from anywhere in the code is bad so I would even consider dropping the call to Regarding the wrong return value, I would assume the OpenSC/src/pkcs11/pkcs11-object.c Line 1168 in 8fc2c20 Unfortunately the debug log does not show much information about this. I can probably try tomorrow. I am wondering if we should put together some test coverage with the pin cache disabled (and interactive PIN prompts) to make sure this works ok in all cases. Regarding to this PR, do we consider this good enough to merge and for a release, or do we have some more concerns that I should address as part of this PR? |
As discussed in the #2903 this basically moves the check out of the libopensc to prevent zero-lenght pin bypass there, but allow checking login status in the pkcs15init layer, which happens usually during card enrollment (key generation, ...).
Fixes: #2903
Checklist