DNIe looses login after the card is used by another program

[rickyepoderi]

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:

            if ( res == SC_ERROR_SM ) {
                    sc_log(ctx,"Detected SM error/collision. Try %d",retries);
                    switch(provider->status.session.state) {
                            /* No SM or creating: collision with other process
                               just retry as SM error reset ICC SM state */
                            case CWA_SM_NONE:
                            case CWA_SM_INPROGRESS:
                                    continue;
                            /* SM was active: force restart SM and retry */
                            case CWA_SM_ACTIVE:
                                    res=cwa_create_secure_channel(card, provider, CWA_SM_COLD);
                                    LOG_TEST_RET(ctx,res,"Cannot re-enable SM");
                                    continue;
                    }
            }

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:

*** framework-pkcs15.c.ORIG 2015-10-30 21:40:17.501635616 +0100
--- framework-pkcs15.c  2015-10-30 22:11:01.794737610 +0100
*************** pkcs15_prkey_sign(struct sc_pkcs11_sessi
*** 3590,3595 ****
--- 3590,3600 ----
                    pData, ulDataLen, pSignature, *pulDataLen);
    }

+   if (rv == SC_ERROR_PIN_EXPIRED) {
+       sc_log(context, "RICKY, setting the slot as not logged in...");
+       session->slot->login_user = -1;
+   }
+ 
    sc_unlock(p11card->card);

    sc_log(context, "Sign complete. Result %d.", rv);

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!

[frankmorgner]

There is already https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-sec.c#L497-L499. Have you tried returning SC_ERROR_SECURITY_STATUS_NOT_SATISFIED instead?

[rickyepoderi]

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.

[rickyepoderi]

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:

*** pkcs15-sec.c.ORIG   2015-10-31 10:32:40.746585343 +0100
--- pkcs15-sec.c    2015-10-31 10:46:52.707074358 +0100
*************** int sc_pkcs15_compute_signature(struct s
*** 481,486 ****
--- 481,488 ----
    sc_log(ctx, "Private key path '%s'", sc_print_path(&prkey->path));
    if (prkey->path.len != 0 || prkey->path.aid.len != 0) {
        r = select_key_file(p15card, prkey, &senv);
+       if (r == SC_ERROR_SECURITY_STATUS_NOT_SATISFIED && sc_pkcs15_pincache_revalidate(p15card, obj) == SC_SUCCESS)
+           r = select_key_file(p15card, prkey, &senv);
        if (r < 0) {
            sc_unlock(p15card->card);
            LOG_TEST_RET(ctx, r,"Unable to select private key file");

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!

[dengert]

The same changes should be added to sc_pkcs15_decipher and sc_pkcs15_derive
They both call select_key_file

[frankmorgner]

@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?

[rickyepoderi]

@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.

[rickyepoderi]

@frankmorgner I think you are right, the sc_pkcs15_pincache_revalidate do this:

    if (p15card->card->reader->capabilities & SC_READER_CAP_PIN_PAD)
            return SC_ERROR_SECURITY_STATUS_NOT_SATISFIED;

    r = sc_pkcs15_find_pin_by_auth_id(p15card, &obj->auth_id, &pin_obj);
    if (r != SC_SUCCESS) {
            sc_log(ctx, "Could not find pin object for auth_id %s", sc_pkcs15_print_id(&obj->auth_id));
            return SC_ERROR_SECURITY_STATUS_NOT_SATISFIED;
    }

So if you don't have the pin, you cannot re-login.

[frankmorgner]

@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...

[frankmorgner]

... that's why I'm asking if the workaround works always or just with caching.

[rickyepoderi]

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.

[rickyepoderi]

I can confirm it does not work if cache is deactivated in opensc.conf (I've just realized there is an option for that):

            use_pin_caching = false;

So it is clear that it only works if the pin is cached.

[dengert]

As you stated in the original post the original problem may be "one secure channel at a time"
ISO 7816 supports Logical channels, that may or may not apply to secure channels. It is not clear if
the DNie supports more then one secure channels. Only the DNIe specs may something.
If it can then it might be possible to have multiple secure channels, but this might require
PCSC to keep track of channels in use.

The DNIe specs should also say if the security state of the card is reset after a reestablishing
a secure channel.

https://www.limited-entropy.com/dnie-device-auth/
might help.

[dengert]

@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.
CKR_TOKEN_NOT_PRESENT is also possible.

PKCS#11 defines a C_Logout, we could simulate that from inside the lib.

Crypto operations can return:
CKR_USER_NOT_LOGGED_IN, CKR_SESSION_CLOSED and many others.
If we did an internal C_Logout, we would return CKR_SESSION_CLOSED because C_Logout should have closed all the sessions.

We could changing all sessions to R/O, and returning CKR_SESSION_READ_ONLY or CKR_USER_NOT_LOGGED_IN might work.
It is not clear what the calling application will do if it receives CKR_USER_NOT_LOGGED_IN.

The idea is to return something to have the application try C_Login again.

[frankmorgner]

SC_ERROR_SECURITY_STATUS_NOT_SATISFIED is already translated to CKR_USER_NOT_LOGGED_IN in sc_to_cryptoki_error. So I guess only resetting login_user or calling C_Logout internally is missing.

[frankmorgner]

Although the user is not really logged in anymore and calling C_Logout would interfere with the other process. So I think resetting login_user only would be usefull...

[rickyepoderi]

@frankmorgner I think that updating the session information should be something like this:

diff --git a/src/pkcs11/framework-pkcs15.c b/src/pkcs11/framework-pkcs15.c
index e4962bf..4ff8536 100644
--- a/src/pkcs11/framework-pkcs15.c
+++ b/src/pkcs11/framework-pkcs15.c
@@ -3595,6 +3595,11 @@ pkcs15_prkey_sign(struct sc_pkcs11_session *session, void *obj,
                                        pData, ulDataLen, pSignature, *pulDataLen);
        }

+       if (rv == SC_ERROR_SECURITY_STATUS_NOT_SATISFIED && session->slot->login_user >= 0) {
+               sc_log(context, "The pin is expired. Updating session info.");
+               session->slot->login_user = -1;
+        }
+
        sc_unlock(p11card->card);

        sc_log(context, "Sign complete. Result %d.", rv);

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.

[dengert]

Another option is to treat the DNIe card as if it enforces CKA_ALWAYS_AUTHENTICATE
on all its keys. This would force a reentry of the PIN (with flags in the opensc.conf set pin cache could be used so user does not have to renter PIN)

FireFox knows how to handle CKA_ALWAYS_AUTHENTICATE.

Could it be the DNIe does in fact enforce CKA_ALWAYS_AUTHENTICATE on some keys, but
OpenSC has not been setting it?

Some tests doing multiple sign operations using the same SM session would show if this is the case,
or if reestablishing the SM resets the security state that causes the problem.

[dengert]

Please also see #451 #362 These relate to DNIe and problems with FireFox, user_consent and CKA_ALWAYS_AUTHENTICATE.
https://github.com/rickyepoderi
https://github.com/germanblanco

[rickyepoderi]

@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.

[dengert]

@rickyepoderi Also see #362
and talk to @germanblanco

[germanblanco]

Could we please try with CKA_ALWAYS_AUTHENTICATE and see if whether it does or does not make any noticeable difference in performance?
Once we see it working correctly, we could check how to improve the speed.

[rickyepoderi]

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:

diff --git a/src/libopensc/pkcs15-dnie.c b/src/libopensc/pkcs15-dnie.c
index c14f667..d3526dd 100644
--- a/src/libopensc/pkcs15-dnie.c
+++ b/src/libopensc/pkcs15-dnie.c
@@ -243,6 +243,13 @@ static int sc_pkcs15emu_dnie_init(sc_pkcs15_card_t * p15card)
                     p15_info = (struct sc_pkcs15_cert_info *) p15_obj ->data;
                    p15_info ->path.count = -1;
                }
+               /* Set CKA_ALWAYS_AUTHENTICATE to all the private keys in
+                  order to avoid errors with secure channel when two
+                  applications run simultaneusly */
+               if ( p15_obj->df && (p15_obj->df->type == SC_PKCS15_PRKDF) ) {
+                       sc_log(ctx, "RICKY: setting user_consent to %s", sc_print_path(&p15_obj->df->path));
+                       p15_obj->user_consent = 1;
+               }
                /* Remove found public keys as cannot be read_binary()'d */
                if ( p15_obj->df && (p15_obj->df->type == SC_PKCS15_PUKDF) ) {
                        sc_pkcs15_object_t *puk = p15_obj;

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.

[germanblanco]

Thank you for doing the test.
So let's go back to the previous point in the thread.
I actually think now that the safest bet is the new error code that @rickyepoderi proposed originally ("SC_ERROR_PIN_EXPIRED" or whatever name fits better). It avoids the risk of this behaviour impacting any cards that don't use the new error code. And the behaviour of resetting the user and forcing a new log-in for this error condition seems reasonable at least as an option.
The only question then would be if the applications handle this condition correctly, but according to the first post of @rickyepoderi, it seems that at least Firefox works ok.
Does that make sense?

[germanblanco]

By the way, apparently the DNIe module doesn't intend to support pinpad at all.
At least I found this in card-dnie.c, that seems to indicate that:

         /*
         * some flags and settings from documentation
         * No (easy) way to handle pinpad throught SM, so disable it
         */
         data->flags &= ~SC_PIN_CMD_NEED_PADDING; /* no pin padding */
         data->flags &= ~SC_PIN_CMD_USE_PINPAD;   /* cannot handle pinpad */

[rickyepoderi]

@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.

[germanblanco]

I don't see the danger to other cards, could you please explain?
If there is a new error code, and it is returned only by DNIe, then there is no danger, right?
In the code that you pointed out in the first post, it seems possible to return a new code in dnie_select_file that would be seen here.

[dengert]

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.
AND we need to test some popular applications to see if they can respond as expected.

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
that has the CK_STATE. If CKR_FUNCTION_FAILED is returned PKCS#11 says more info can be found by using C_GetSessionInfo. So CKR_FUNCTION_FAILED is a posibble return code.

The CK_STATE indicates by "R/* Public Session" vs "R/* User Functions"

We could also

[rickyepoderi]

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:

• There is a new error SC_ERROR_PIN_EXPIRED in opensc. Any card can return the error when (for whatever reason) the pin is expired.
• DNIe is modified to return that error when the secure channel is broken.
• pkcs15-sec is modified to relogin (sc_pkcs15_pincache_revalidate) if the previous error is returned. To not interfere with previous implementation, in the old case that revalidate was called both errors are taken into acount.
• There is a new method sc_session_to_cryptoki_error that, besides translating the opensc error to pkcs11 error, receives the session. This way if relogin is necessary the session is updated. I prefer this way because future modifications of the session can be handled here for any other errors. No lock is necessary because all pkcs11 methods use sc_pkcs11_lock and sc_pkcs11_unlock to protect the session.
• framework-pkcs15 calls this new function instead of the previous one . I only changed the three affected methods.

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.

[frankmorgner]

@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

[frankmorgner]

Could you check if #618 fixes your problem? It's similar to the pkcs#15 pin caching, but used on a more general level.

[rickyepoderi]

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!

[frankmorgner]

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...

[dengert]

I agree they could co-exist.
If a card driver does not have a check_status routine, the code works much like it does today except a sc_lock, sc_unlock is preformed
for the C_GetSessionInfo. If the lock count is zero, pcsc_lock will be called and the return from the SCardBeginTransaction may return
that the card has been reset or removed. The sc_lock will also do some SM checking which I don't have any way to test.
The main problem with the current code is C_GetSessionInfo returns outdated information.

On 11/29/2015 10:12 AM, Frank Morgner wrote:

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...

—
Reply to this email directly or view it on GitHub #596 (comment).

Douglas E. Engert [email protected]

[frankmorgner]

@rickyepoderi could you check if frankmorgner@7e5390c resets the token information correctly?

[rickyepoderi]

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. ;-)

[frankmorgner]

Changing login_user based upon the return code should also work for other cards and even for DNIe without any modifications (it does return SC_ERROR_SECURITY_STATUS_NOT_SATISFIED, right?). Have you tried out the changes?

[rickyepoderi]

Instead of using a simple if in all the needed places:

    if (rv == CKR_USER_NOT_LOGGED_IN)
        session->slot->login_user = -1; 

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):

    check_session_return_code(session, rv);

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).

[frankmorgner]

well, for two lines, I don't think it's worth creating a new function. Anyway, we could add those two lines to reset_login_state if you want.

[frankmorgner]

I quickly added this on https://github.com/frankmorgner/OpenSC/tree/not-logged-in with frankmorgner@4ddfb8c

[frankmorgner]

By the way, adding sc_pkcs15_pincache_revalidate if sc_set_security_env/ select_key_file return SC_ERROR_SECURITY_STATUS_NOT_SATISFIED is still useful. Because it's clear that the card may return this error also during preparation and not only for the actual signature creation/decryption

[rickyepoderi]

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!

[rickyepoderi]

I have just tested the new branch. Everything works as expected. The summary is the following:

• atomic=false
  1. pkcs11-tool -d XXXXXXXXXXXXXX -s => OK
  2. firefox + pkcs11-tool -l -I (interference) => page=OK tool=OK page=KO retry-page=OK
  3. firefox + applet (DNIe test page) => page=OK applet=OK page=KO retry-page=OK
  4. chrome + pkcs11-tool -l -I (interference) => page=OK tool=OK page=KO retry-page=OK
• atomic=true
  1. pkcs11-tool -d XXXXXXXXXXXXXX -s => OK
  2. firefox + pkcs11-tool -l -I (interference) => page=OK tool=OK page=OK
  3. firefox + applet (DNIe test page) => page=OK applet=OK page=OK
  4. chrome + pkcs11-tool -l -I (interference) => page=OK tool=OK page=OK

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.

[frankmorgner]

@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.)

[germanblanco]

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.
@frankmorgner, I will try this as soon as I have a moment (this weekend).

[frankmorgner]

Silent re-login only works within the current process and it only works after C_Login and before C_Logout. This hopefully appeases your concerns.

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

[rickyepoderi]

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:

• atomic=false && use_pin_caching=false
  1. pkcs11-tool -d XXXXXXXXXXXXXX -s => OK
  2. firefox + pkcs11-tool -l -I (interference) => page=OK tool=OK page=KO retry-page=OK
  3. firefox + applet (DNIe test page) => page=OK applet=OK page=KO retry-page=OK
  4. chrome + pkcs11-tool -l -I (interference) => page=OK tool=OK page=KO retry-page=OK
• atomic=false && use_pin_caching=true (default config)
  1. pkcs11-tool -d XXXXXXXXXXXXXX -s => OK
  2. firefox + pkcs11-tool -l -I (interference) => page=OK tool=OK page=OK
  3. firefox + applet (DNIe test page) => page=OK applet=OK page=OK
  4. chrome + pkcs11-tool -l -I (interference) => page=OK tool=OK page=OK
• atomic=true && use_pin_caching=false (use_pin_caching does not matter here but just to be sure)
  1. pkcs11-tool -d XXXXXXXXXXXXXX -s => OK
  2. firefox + pkcs11-tool -l -I (interference) => page=OK tool=OK page=OK
  3. firefox + applet (DNIe test page) => page=OK applet=OK page=OK
  4. chrome + pkcs11-tool -l -I (interference) => page=OK tool=OK page=OK

The new case is the second group of tests and they work as expected. It's nice to see DNIe working with default configuration.

[frankmorgner]

@rickyepoderi I just pushed some refactoring commits on top of https://github.com/frankmorgner/OpenSC/tree/not-logged-in did you test with those?

[frankmorgner]

@rickyepoderi if you have some more time you maybe want to look #622...

[rickyepoderi]

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:

diff --git a/src/libopensc/pkcs15-sec.c b/src/libopensc/pkcs15-sec.c
index 5953fa5..8db083d 100644
--- a/src/libopensc/pkcs15-sec.c
+++ b/src/libopensc/pkcs15-sec.c
@@ -81,7 +81,7 @@ static int select_key_file(struct sc_pkcs15_card *p15card,
 }

 static int use_key(struct sc_pkcs15_card *p15card,
-               const struct sc_pkcs15_prkey_info *prkey,
+               const struct sc_pkcs15_object *obj,
                sc_security_env_t *senv,
                int (*card_command)(sc_card_t *card,
                         const u8 * in, size_t inlen,
@@ -90,6 +90,7 @@ static int use_key(struct sc_pkcs15_card *p15card,
 {
        int r = SC_SUCCESS;
        int revalidated_cached_pin = 0;
+       const struct sc_pkcs15_prkey_info *prkey = (const struct sc_pkcs15_prkey_info *) obj->data;

        r = sc_lock(p15card->card);
        LOG_TEST_RET(p15card->card->ctx, r, "sc_lock() failed");
@@ -112,8 +113,7 @@ static int use_key(struct sc_pkcs15_card *p15card,
                        /* only re-validate once */
                        break;
                if (r == SC_ERROR_SECURITY_STATUS_NOT_SATISFIED) {
-                       r = sc_pkcs15_pincache_revalidate(p15card,
-                                       (const struct sc_pkcs15_object *) prkey);
+                       r = sc_pkcs15_pincache_revalidate(p15card, obj);
                        if (r < 0)
                                break;
                        revalidated_cached_pin = 1;
@@ -216,8 +216,7 @@ int sc_pkcs15_decipher(struct sc_pkcs15_card *p15card,
        LOG_TEST_RET(ctx, r, "cannot encode security operation flags");
        senv.algorithm_flags = sec_flags;

-       r = use_key(p15card, prkey, &senv, sc_decipher, in, inlen, out,
-                       outlen);
+       r = use_key(p15card, obj, &senv, sc_decipher, in, inlen, out, outlen);
        LOG_TEST_RET(ctx, r, "use_key() failed");

        /* Strip any padding */
@@ -274,8 +273,7 @@ int sc_pkcs15_derive(struct sc_pkcs15_card *p15card,
        LOG_TEST_RET(ctx, r, "cannot encode security operation flags");
        senv.algorithm_flags = sec_flags;

-       r = use_key(p15card, prkey, &senv, sc_decipher, in, inlen, out,
-                       *poutlen);
+       r = use_key(p15card, obj, &senv, sc_decipher, in, inlen, out, *poutlen);
        LOG_TEST_RET(ctx, r, "use_key() failed");

        /* Strip any padding */
@@ -431,8 +429,7 @@ int sc_pkcs15_compute_signature(struct sc_pkcs15_card *p15card,
                inlen = modlen;
        }

-       r = use_key(p15card, prkey, &senv, sc_compute_signature, tmp, inlen,
-                       out, outlen);
+       r = use_key(p15card, obj, &senv, sc_compute_signature, tmp, inlen, out, outlen);
        LOG_TEST_RET(ctx, r, "use_key() failed");
        sc_mem_clear(buf, sizeof(buf));

[frankmorgner]

@rickyepoderi thanks, I rewrote the commits applying your fixes!

[rickyepoderi]

If you want me to test again please tell me where the new modifications are (I don't see them in the same branch).

[frankmorgner]

Sorry, I gave you a bad pointer. I directly pushed the fixed commits to the already existing PR #618.

[rickyepoderi]

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!

[frankmorgner]

ok, thanks!