-
Notifications
You must be signed in to change notification settings - Fork 716
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
DNIe looses login after the card is used by another program #596
Comments
There is already https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-sec.c#L497-L499. Have you tried returning |
Yep! It was the original error returned by the card driver. It is directly derived by the APDU response of the card. Then it is transformed into CKR_USER_NOT_LOGGED_IN in the pkcs11 layer. But the problem is that the error is not managed by anybody, the pkcs11 layer (session->slot->login_user) still maintains the user as logged (value CKU_USER). Therefore nobody asks for re-login. |
You're a genius @frankmorgner, with your note I could figure out how to fix this. I'd already thought about caching the login and re-login but I hadn't seen you have already a pin cache (sorry, the code is quite big). The problem with DNIe is that the SC_ERROR_SECURITY_STATUS_NOT_SATISFIED gives before the line you have pointed out. It happens in the previous select_key_file. So I have just replicated your cache thing like this:
And now it works. My branch now is very dirty but I'll try to clean it up and test everything again. Thanks a lot! |
The same changes should be added to sc_pkcs15_decipher and sc_pkcs15_derive |
@rickyepoderi, that workaround only works if the PIN is cached, right? If the reader has a PIN-pad, the user might not notice the requested PIN. If the reader has no PIN-pad and the PIN is not cached, then the original problem remains. @dengert do you know how one could propagate the need for a re-login to the PKCS#11 layer? |
DNIe looses login after the card is used by another program OpenSC#596
@frankmorgner Yes I suppose it only works if the PIN is cached but it is exactly the same in the snipped you linked before, isn't it? I have prepared the changes in my repo. I have added the sc_pkcs15_pincache_revalidate in two places (because in DNIe the SC_ERROR_SECURITY_STATUS_NOT_SATISFIED is returned in both). I have also modified the cwa-dnie.c file because now there is no need to re-establish the secure channel. The pin_verify re-creates it (no matter if it was already created) so there was a useless creation of the channel with the previous code (it is quite slow indeed, so better don't do it twice). @germanblanco Could you test the changes too? Just to have a second opinion. You have to go to the DNIe test page, perform the signature and then reload the initial page (to force the browser to reuse the DNIe). Without my patch an error is displayed. Contact me if you have any doubt. |
@frankmorgner I think you are right, the sc_pkcs15_pincache_revalidate do this:
So if you don't have the pin, you cannot re-login. |
@rickyepoderi I felt that https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-sec.c#L497-L499 is just a workaround for not signalling the correct error to the upper layer. This snippet would not be needed if OpenSC had some mechanism to invalidate the PIN in PKCS#11... |
... that's why I'm asking if the workaround works always or just with caching. |
I don't have a pinpad reader so I cannot confirm it (but it is quite sure it does not work if pinpad). I checked the pkcs11 layer before opening this issue and it never modifies the state of the session (session->slot->login_user is only modified in pkcs11-session.c). I think that, if you want to update it in case of SC_ERROR_SECURITY_STATUS_NOT_SATISFIED, it is needed to do something in the places that sc_pkcs15_decipher, sc_pkcs15_compute_signature and sc_pkcs15_derive are used. At least it seems it is only in framework-pkcs15.c. Another comment, I wouldn't remove the workaround because firefox (at least) shows an error in the request that returns the error (no matter you return CKR_USER_NOT_LOGGED_IN or CKR_PIN_EXPIRED, firefox never retries silently). With the workaround no error is displayed (which is very nice). But updating the state of the session (in case of pinpad or not having the pin in the cache) the second time you access the card a real re-login is requested by firefox. At least this is what I see in my tests. |
I can confirm it does not work if cache is deactivated in opensc.conf (I've just realized there is an option for that):
So it is clear that it only works if the pin is cached. |
As you stated in the original post the original problem may be "one secure channel at a time" The DNIe specs should also say if the security state of the card is reset after a reestablishing https://www.limited-entropy.com/dnie-device-auth/ |
@frankmorgner hasked about hot to tell application that it needs to login again. Looking at PKCS#11 2.40. We could do what we do if the token was removed CKR_DEVICE_REMOVED But thiat is more of a permanent error. PKCS#11 defines a C_Logout, we could simulate that from inside the lib. Crypto operations can return: We could changing all sessions to R/O, and returning CKR_SESSION_READ_ONLY or CKR_USER_NOT_LOGGED_IN might work. The idea is to return something to have the application try C_Login again. |
|
Although the user is not really logged in anymore and calling |
@frankmorgner I think that updating the session information should be something like this:
This should be put in the three methods (pkcs15_prkey_sign, pkcs15_prkey_decrypt and pkcs15_prkey_derive). Tested with DNIe and use_pin_caching = false, firefox gives an error the first time but then relogin is requested. What I'm not sure if all the SC_ERROR_SECURITY_STATUS_NOT_SATISFIED errors should be automatically trigger the logout. I don't know if this error could happen with a proper session (I think there are different types of sessions and, maybe, in some cases you can receive this error but the session is not broken). That's why I thought previously to specify another opensc error (SC_ERROR_RELOGIN_NEEDED for example) that the card can return to unequivocally mark the session is really expired and a relogin is needed. In the case of DNIe is when the secure channel is broken. But this point is up to you. :-) If you want me tp update my branch with something similar to this, please just tell me. |
Another option is to treat the DNIe card as if it enforces CKA_ALWAYS_AUTHENTICATE FireFox knows how to handle CKA_ALWAYS_AUTHENTICATE. Could it be the DNIe does in fact enforce CKA_ALWAYS_AUTHENTICATE on some keys, but Some tests doing multiple sign operations using the same SM session would show if this is the case, |
Please also see #451 #362 These relate to DNIe and problems with FireFox, user_consent and CKA_ALWAYS_AUTHENTICATE. |
@dengert I think that DNIe does not need CKA_ALWAYS_AUTHENTICATE, if you only use one application (for example only firefox) you can manage the card without problems as many times as you want. The problem is when you make two different programs work together (firefox and the signature applet in the demo page for example). The DNIe seems to only maintain one secure channel at a time (I tried to confirm this point looking some documentation but no luck until now, nevertheless I'm quite sure of it right now after checking all available implementations). For that reason one program steals the secure channel to the other. In that circumstances the issue explained here happens (but remember that this test is new because previously issue #480 blocks everything and interaction between two different applications was impossible). Maybe CKA_ALWAYS_AUTHENTICATE would solve the problem, because if you relogin for every operation obviously you are recreating the secure channel everytime. But I'd avoid this configuration if possible because it'd be very slow. On the other hand, you are right that the current implementation has an extra warning to the user when a signature is executed, but this is just a development decision (for example I always compile opensc without setting --enable-dnie-ui and no extra warning is presented). I think this behavior is inherited by the official implementation (I think that official windows drivers also ask for this) but this part can be removed, it is just that, another warning that DNIe displays hardcoded inside the driver. |
@rickyepoderi Also see #362 |
Could we please try with CKA_ALWAYS_AUTHENTICATE and see if whether it does or does not make any noticeable difference in performance? |
Hi, I have added CKA_ALWAYS_AUTHENTICATE to the two keys that the DNIe has (auth and sign). The change is just a few lines in the pkcs15-dnie.c:
But I think the cure is worse than the disease. With the change firefox asks for the pin every https page or link so it is almost unusable. It is clear that the issue is fixed (everytime you re-create the secure channel again) but it is crazy in general. Adding the flag to only the signing key is assumable but does not fix the issue. Besides other browsers like chrome do not use the CKA_ALWAYS_AUTHENTICATE, so the fix is useless in that browser. So, we are back to the beginning. What do you think about (for the moment) just adding the sc_pkcs15_pincache_revalidate calls in framework-pkcs15.c??? Later we can think about a properer modification. |
Thank you for doing the test. |
By the way, apparently the DNIe module doesn't intend to support pinpad at all.
|
@dengert @frankmorgner I think this issue is more or less understood right now but I don't know how to continue... Do you want me to prepare a pull request with only the sc_pkcs15_pincache_revalidate or do you prefer a full solution at pkcs11 layer? If you decide to follow the second path I agree in general with @germanblanco, I would add another opensc return code ("SC_ERROR_PIN_EXPIRED" for example) and another version of sc_to_cryptoki_error in pkcs11 (misc.c) that would also receive the sc_pkcs11_session in order to modify the login_user. This way any function in the framework-pkcs15.c file could call the new version of the function in order to update the login_user if the card returns the SC_ERROR_PIN_EXPIRED error (in the future the function can be used to modify the pkcs11 session status for other possible returned errors). I am not really sure what is the best option. The first solution is partial but make the DNIe pass the governmental demo test with the default opensc configuration (for the first time). The second is more complete but also more dangerous to other cards. So, it is on your hands. I'm ready to help in any case. |
I don't see the danger to other cards, could you please explain? |
If we chose a full PKCS#11 approach we must look at minidriver and tokend. If pin caching is not available or pinpad is being used, we already know how to mark the sesssion internally as needing the pin again. A general PKCS#11 solution would need to return an error that will cause to the application to login again. We should also make the return code an option in opensc.conf, with a fall back of keeping the current code. This would allow a fall back for older applications or older cards. Some return codes are: CKR_USER_NOT_LOGGED_IN, CKR_SESSION_CLOSED, and CKR_FUNCTION_FAILED. Looking closer at PKCS#11 there is a C_ GetSessionInfo that returns a CK_SESSION_INFO The CK_STATE indicates by "R/* Public Session" vs "R/* User Functions" We could also |
You are not understanding what I'm explaining. And because a picture is worth a thousand words I have implemented my idea very quickly. Please take a look to my branch: https://github.com/rickyepoderi/OpenSC/tree/pkcs11-relogin The idea is simple:
I have translated the new error to standard CKR_PIN_EXPIRED because this error means exactly what we are trying to do ("CKR_PIN_EXPIRED: The specified PIN has expired, and the requested operation cannot be carried out unless C_SetPIN is called to change the PIN value. Whether or not the normal user’s PIN on a token ever expires varies from token to token.", check https://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/pkcs11-base-v2.40.html). Please look at my branch and check my proposal for a full change. At the end I think is not very invasive, previous cards are not affected (except if a bug is introduced) and anyone can use the SC_ERROR_PIN_EXPIRED for now on. |
@rickyepoderi I'm not convinced that blindly re-validating the PIN is the correct answer. I think that locking a session (and avoid interfering applications) could be a more general answer to this problem, see https://thread.gmane.org/gmane.comp.encryption.opensc.devel/16363/focus=16376 |
Could you check if #618 fixes your problem? It's similar to the pkcs#15 pin caching, but used on a more general level. |
I have compiled your changes and set the "atomic = true" in opensc.conf and it works (I have done several tests and all of them worked ok). But (for what I understand) with this option your are doing login <-> operation <-> logout all the time, aren't you? I don't know, in Spain we have a saying that is "no hay que ponerse la venda antes de la herida" that can be translated to something like don't bandage it before it gets hurt. I think that with pkcs11 the normal way of working is that only one application uses it (firefox for example) but sometimes another one is needed and interferes with the first (the applet in DNIe test-page), when the second application stops the normal way of working (one app) is again restored. The proposed solution ensures that the card is working well with a lot of interfering but needs modifying the default configuration and pin caching, besides DNIe is slower. It's just my humble opinion but I like more the solution that updates the session state with the return codes from the card that I implemented here (I've cleaned sc_pkcs15_pincache_revalidate part and only the solution 2 commented before is used): master...rickyepoderi:pkcs11-relogin This solution could be complementary to yours. It works with default configuration and avoids DNIe logging in and logging out all the time. Thanks! |
The thing is my approach solves the security problem for all cards. Your solution of invalidating the user login makes things work, but the security problem still persists with non-SM cards. But I agree, both solutions could co-exist. At the moment I'm not sure if your approach is the best, because it requires touching every card driver to add the new error code... |
I agree they could co-exist. On 11/29/2015 10:12 AM, Frank Morgner wrote:
Douglas E. Engert [email protected] |
@rickyepoderi could you check if frankmorgner@7e5390c resets the token information correctly? |
I liked the idea of a function in misc.c that performs the change based in the return code, this way any other value that also meant a change (in a flag or whatever) could be added easily and for all the pkcs11 functions (the invocation would already be there). Do you prefer the function or the current if? I don't like to check it twice. ;-) |
Changing |
Instead of using a simple if in all the needed places:
I like more the idea of having a centralized function in misc.c that can take into account other return codes and other actions, something like (if you check my branch I added it to the return code translation function):
That way the call is already placed in all the needed pkcs11 functions and if another transformation is needed in the future is very easy to add it. For example if the CKR_PIN_EXPIRED is used in opensc sometime you only have to add the change in the function to also expire the session for that return code. And if other return code produces other change (whatever) in the session it can be added easily. But it's just an idea. And no, I haven't tested the change yet, I'll try to do it this week (nonetheless I think it's going to work but let me time to check it). |
well, for two lines, I don't think it's worth creating a new function. Anyway, we could add those two lines to |
I quickly added this on https://github.com/frankmorgner/OpenSC/tree/not-logged-in with frankmorgner@4ddfb8c |
By the way, adding |
Perfect @frankmorgner, I'll check https://github.com/frankmorgner/OpenSC/tree/not-logged-in. I'll try to do it as soon as possible but maybe I need some days. Thanks! |
I have just tested the new branch. Everything works as expected. The summary is the following:
Now when atomic=false the error updates the session, firefox and chrome ask for the password after it and re-logs into the card. @germanblanco Maybe we can add a silent re-login inside the DNIe driver if use_pin_caching=true to avoid the error in the first group of tests. But this another story that just affects DNIe. In my opinion this branch handles the issue quite gently. |
@rickyepoderi , @germanblanco I think this last issue can also be seen at a broader scope. I committed frankmorgner@58575b4 to fix this for all cards (still on https://github.com/frankmorgner/OpenSC/tree/not-logged-in). Could you try if it transparently fixes your problem when caching is active? (I don't have a card that even triggers re-validation of a cached PIN.) |
Please excuse me if I have lost track of the details here. @rickyepoderi if we add a silent re-login inside the DNIe driver if use_pin_caching=true, are we then giving access to the browser with the PIN provided from the pkcs11-tool or the applet? I don't like that idea. |
Silent re-login only works within the current process and it only works after The last problem to be solved is that this silent re-login doesn't work for DNIe, because the PIN is currently only re-validated for the actual cryptographic card command. But it should actually also be re-validated if we get an error during the preparation (selection of the key or the MSE command). This behaviour should be fixed with frankmorgner/OpenSC@58575b4 |
The re-validate only works in a previously logged card, but now it doesn't matter cos with the last @frankmorgner's change the re-login is not needed inside DNIe (it's placed in an upper level). I have just tested everything:
The new case is the second group of tests and they work as expected. It's nice to see DNIe working with default configuration. |
@rickyepoderi I just pushed some refactoring commits on top of https://github.com/frankmorgner/OpenSC/tree/not-logged-in did you test with those? |
@rickyepoderi if you have some more time you maybe want to look #622... |
There is a problem with sc_pkcs15_pincache_revalidate in the last modifications, it seems that the pin is not found, I think the problem is that you're using the wrong object (a sc_pkcs15_prkey_info instead of a sc_pkcs15_object) to call the function. With this change it works again:
|
@rickyepoderi thanks, I rewrote the commits applying your fixes! |
If you want me to test again please tell me where the new modifications are (I don't see them in the same branch). |
Sorry, I gave you a bad pointer. I directly pushed the fixed commits to the already existing PR #618. |
Ok, I didn't even know that there was a pull. I've checked it right now and the three group tests (the same groups presented in a previous comment) run smoothly. Great job! |
ok, thanks! |
Implemented atomic transactions for PKCS#11 See discussion here https://opensc.1086184.n5.nabble.com/State-of-stateless-support-td15513.html fixes https://www.opensc-project.org/opensc/ticket/186 fixes #596
Hi again,
Once the another issue is fixed (issue #480 ) I've been trying to continue testing the DNIe. There is another problem but this time (I think) it is related to the card internals.
First I'm going to describe the problem:
1.- Go to the DNIe test page: https://av-dnie.cert.fnmt.es/compruebacert/compruebacert
2.- The browser requests the password and the https process is completed successfully.
3.- Inside the page the information of the certificate is shown. There is a second link that let you test a signature process.
4.- Click the signature link. The new page starts a Java applet (a separate program) that performs a signature sample (it again requests the password and so on).
5.- The signature process also completes successfully.
6.- Reload again the https page.
7.- Here an error is displayed: SC_ERROR_SECURITY_STATUS_NOT_SATISFIED => CKR_USER_NOT_LOGGED_IN. This error continues until you unplug the card from the reader.
I've been looking the code for a long time, and I think the issue can be described like this:
1.- It seems that the DNIe can only maintain one secure channel at a time (I don't know if this is a typical behavior or not). The DNIe implementation take this fact into account and the secure channel is re-established if broken, this re-connection is done in the dnie_wrap_apdu function inside cwa-dnie.c:
2.- But the current problem is that (although the channel is correctly re-established) the card has also lost the pin (the user is not logged in). Therefore the card return SC_ERROR_SECURITY_STATUS_NOT_SATISFIED. the card also needs to pin_verify again if the secure channel is broken. I think the behavior is intrinsic to DNIe (if you re-create the secure channel, you have to pin_verify also).
3.- Nevertheless inside the pkcs11 layer the slot is still marked as logged in (session->slot->login_user is still set to CKU_USER). So C_GetSessionInfo continues informing the browser that the session is logged in.
You can workaround the situation manually logging out or unplugging the card. Firefox logs the user again and everything starts working.
I've been thinking how to overcome the situation but (I think) it isn't going to be easy. I tried two different approaches. The first one doesn't work and the second solution is very invasive inside opensc code:
1.- The first solution added a new opensc error that was translated to standard CKR_PIN_EXPIRED pkcs11 error (this error means that the PIN has expired and you need to re-login). But firefox does not act like I wanted and it does not re-login (because C_GetSessionInfo still returns the session is logged, it refuses to re-login although the previous error).
2.- I added a simple change inside framework-pkcs15.c in order to clean the state (session->slot->login_user = -1) in the pkcs15_prkey_sign function:
As you see, if the new error SC_ERROR_PIN_EXPIRED is returned the session is cleaned. Firefox returns an error the first time but then it realizes that the session is not logged in and requests for the password again. But this change (or a similar one) should be added to a lot of functions in the framework-pkcs15.c file.
I don't know how to continue. I'm just opening this issue to request an advise from you. Do you know any other card that expires the PIN? How can I inform the pkcs11 layer that the pin is expired and a re-login is necessary?
Sorry for being so stubborn but this DNIe is a pain in the ass.
Thanks!
The text was updated successfully, but these errors were encountered: