Diff for fixing login problems in src/sslengines/engine_pkcs11.c

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Diff for fixing login problems in src/sslengines/engine_pkcs11.c

Geoff Elgey
Diff for fixing login problems in src/sslengines/engine_pkcs11.c

G'day,

I've noted before that I think the login section of the pkcs11_load_key function in src/sslengines/engine_pkcs11.c is busted.

In particular, the code assumes that no login is necesary if private keys can be enumerated. However, when using the libmusclepkcs11.so library, private keys can be enumerated without a login, but their contents cannot be revealed. As a result, pkcs11_load_key will not perform a login at all, and no private key will be loaded.

Attached is a diff of engine_pkcs11.c against the opensc-20050712 snapshot, with the following changes:

- do not use key enumeration as a test of login status, as this will
  not work for all PKCS#11 libraries

- replace magic number used for PIN length with a constant

- add documentation for set_pin, as well as testing for NULL input
  and checking for strdup failure

- made the global variable 'pin' static (TODO check if other global
  variables can be declared static)

- if a PIN is allocated, then check for NULL

- if a PIN is to be freed, then whiten the memory first

- if the token has a secure authentication path, then the PIN shoud be
  NULL (as per PKCS#11 v2, p. 126)

- replaced some fprintf statements with 'fail' (TODO all
  fprintf calls should be replaced with log functions)


-- Geoff


_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel

diff.engine_pkcs11.c (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Diff for fixing login problems in src/sslengines/engine_pkcs11.c

Nils Larsch
Geoff Elgey wrote:
...

> - }
> - fail("Login failed\n");
> - }
> - logged_in++;
> - }
> -
> +        /* Perform login to the token if required */
> +        if (tok->loginRequired) {
> +                /* If the token has a secure login (i.e., an external keypad),
> +                   then use a NULL pin. Otherwise, check if a PIN exists. If
> +                   not, allocate and obtain a new PIN. */
> +                if (tok->secureLogin) {
> +                        /* Free the PIN if it has already been
> +                           assigned (i.e, via set_pin */
> +                        if (pin != NULL) {
> +                                memset(pin, 0, strlen(pin));
> +                                free (pin);
> +                                pin = NULL;

the memset here will most likely be optimized away, hence is useless
(see how openssl handles this problem, hint: OPENSSL_cleanse())

> +                        }
> +                }
> +                else if (pin == NULL) {
> +                        pin = (char *) calloc(MAX_PIN_LENGTH, sizeof(char));
> +                        if (pin == NULL) {
> +                                fail("Could not allocate memory for PIN");
> +                        }        
> +                        get_pin(ui_method, callback_data, pin, MAX_PIN_LENGTH);
> +                }
> +                
> +                /* Now login in with the (possibly NULL) pin */
> +                if (PKCS11_login(slot, 0, pin)) {
> +                        /* Login failed, so free the PIN if present */
> +                        if(pin != NULL) {
> +                                memset(pin, 0, strlen(pin));
> +                                free(pin);
> +                                pin = NULL;
> +                        }
> +                        fail("Login failed\n");
> +                }
> +                /* Login successful, PIN retained in case further logins are
> +                   required. This will occur on subsequent calls to the

I would prefer to clear the pin as soon as possible (hence after a
successful login) unless it's explicitly stated somewhere (a config
file ?) to save it internally.

> +                   pkcs11_load_key function. Subsequent login calls should be
> +                   relatively fast (the token should maintain its own login
> +                   state), although there may still be a slight performance
> +                   penalty. We could maintain state noting that successful
> +                   login has been performed, but this state may not be updated
> +                   if the token is removed and reinserted between calls. It
> +                   seems safer to retain the PIN and peform a login on each
> +                   call to pkcs11_load_key, even if this may not be strictly
> +                   necessary. */
> +                /* TODO when does PIN get freed after successful login? */
> +                /* TODO confirm that multiple login attempts do not introduce
> +                        significant performance penalties */

Nils
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Diff for fixing login problems in src/sslengines/engine_pkcs11.c

Nils Larsch
Nils Larsch wrote:

> Geoff Elgey wrote:
> ...
>
>> -            }
>> -            fail("Login failed\n");
>> -        }
>> -        logged_in++;
>> -    }
>> -
>> +        /* Perform login to the token if required */
>> +        if (tok->loginRequired) {
>> +                /* If the token has a secure login (i.e., an external
>> keypad),
>> +                   then use a NULL pin. Otherwise, check if a PIN
>> exists. If
>> +                   not, allocate and obtain a new PIN. */
>> +                if (tok->secureLogin) {
>> +                        /* Free the PIN if it has already been
>> +                           assigned (i.e, via set_pin */
>> +                        if (pin != NULL) {
>> +                                memset(pin, 0, strlen(pin));
>> +                                free (pin);
>> +                                pin = NULL;
>
>
> the memset here will most likely be optimized away, hence is useless
> (see how openssl handles this problem, hint: OPENSSL_cleanse())

as the code already uses openssl one might use OPENSSL_cleanse here
(afaik it is included in openssl >= 0.9.7).

Nils
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Diff for fixing login problems in src/sslengines/engine_pkcs11.c

Nils Larsch
In reply to this post by Geoff Elgey
Geoff Elgey wrote:

> G'day,
>
> I've noted before that I think the login section of the pkcs11_load_key
> function in src/sslengines/engine_pkcs11.c is busted.
>
> In particular, the code assumes that no login is necesary if private
> keys can be enumerated. However, when using the libmusclepkcs11.so
> library, private keys can be enumerated without a login, but their
> contents cannot be revealed. As a result, pkcs11_load_key will not
> perform a login at all, and no private key will be loaded.
>
> Attached is a diff of engine_pkcs11.c against the opensc-20050712
> snapshot, with the following changes:
>
> - do not use key enumeration as a test of login status, as this will
>   not work for all PKCS#11 libraries
>
> - replace magic number used for PIN length with a constant
>
> - add documentation for set_pin, as well as testing for NULL input
>   and checking for strdup failure
>
> - made the global variable 'pin' static (TODO check if other global
>   variables can be declared static)
>
> - if a PIN is allocated, then check for NULL
>
> - if a PIN is to be freed, then whiten the memory first
>
> - if the token has a secure authentication path, then the PIN shoud be
>   NULL (as per PKCS#11 v2, p. 126)
>
> - replaced some fprintf statements with 'fail' (TODO all
>   fprintf calls should be replaced with log functions)

sorry for the delay. I've applied the patch (but I replaced memset
with OPENSSL_cleanse). Please test a recent snapshot.

Thanks,
Nils
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel