Hi OpenSC folks, While debugging some issues with pkcs#11 related handling in wpasupplicant, I found two small bugs that I'm sending patches for. Both are related to attempting to reuse pkcs#11 modules in one process, something that wpasupplicant does when switching to and from networks that one wants to authenticate to using smartcard credentials.
The first is for a buffer overrun in engine_pkcs11's pin handling. The overrun occurs after the pin has been created with strdup() via set_pin(), when it is OPENSSL_cleanse() it always cleanses to MAX_PIN_LENGTH, which will cause free() to fail when the pin is short. The patch adds tracking of the pin length in a new static variable and uses it for all calls to OPENSSL_cleanse(). The second patch is to make libp11 swallow CKR_CRYPTOKI_ALREADY_INITIALIZED returns from C_Initialize(). It's an informational message and should not be bubbled up to the caller. Please take a look, - dds
commit 26891c363bc4fa2abcb768cd149c77228cb60d30 Author: David Smith <[email protected]> Date: Tue Oct 20 17:56:36 2009 +0900 Fix pin length handling to prevent segfault from cleansing. When using OPENSSL_cleanse on a strdup()'d pin, the cleansing should only be performed up to the length of the pin. Otherwise, the buffer is overrun. This segfault is reproducible when you have a client that closes and reopens connections to the PKCS#11 provider in the same process (in my case, wpasupplicant). Employing a simple solution by adding a variable to track the length of the pin. Better solutions welcome but this should do fine. Signed-off-by: David Smith <[email protected]> diff --git a/src/engine_pkcs11.c b/src/engine_pkcs11.c index fe60972..6715022 100644 --- a/src/engine_pkcs11.c +++ b/src/engine_pkcs11.c @@ -50,10 +50,9 @@ static PKCS11_CTX *ctx; * The memory for this PIN is always owned internally, * and may be freed as necessary. Before freeing, the PIN * must be whitened, to prevent security holes. - * - * length is always MAX_PIN_LENGTH and possibly not 0 terminated? */ static char *pin = NULL; +static int pin_length = 0; static int verbose = 0; @@ -90,8 +89,9 @@ int set_pin(const char *_pin) /* Copy the PIN. If the string cannot be copied, NULL shall be returned and errno shall be set. */ - pin = malloc(MAX_PIN_LENGTH); - strncpy(pin,_pin,MAX_PIN_LENGTH); + pin = strdup(_pin); + if (pin != NULL) + pin_length = strlen(pin); return (pin != NULL); } @@ -119,6 +119,7 @@ static int get_pin(UI_METHOD * ui_method, void *callback_data) if (!pin) return 0; strncpy(pin,mycb->password,MAX_PIN_LENGTH); + pin_length = MAX_PIN_LENGTH; return 1; } @@ -158,9 +159,10 @@ int pkcs11_finish(ENGINE * engine) ctx = NULL; } if (pin != NULL) { - OPENSSL_cleanse(pin, MAX_PIN_LENGTH); + OPENSSL_cleanse(pin, pin_length); free(pin); pin = NULL; + pin_length = 0; } return 1; } @@ -182,9 +184,10 @@ int pkcs11_init(ENGINE * engine) int pkcs11_rsa_finish(RSA * rsa) { if (pin) { - OPENSSL_cleanse(pin, MAX_PIN_LENGTH); + OPENSSL_cleanse(pin, pin_length); free(pin); pin = NULL; + pin_length = 0; } if (module) { free(module); @@ -688,19 +691,22 @@ static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id, /* Free the PIN if it has already been assigned (i.e, cached by get_pin) */ if (pin != NULL) { - OPENSSL_cleanse(pin, MAX_PIN_LENGTH); + OPENSSL_cleanse(pin, pin_length); free(pin); pin = NULL; + pin_length = 0; } } else if (pin == NULL) { pin = (char *)calloc(MAX_PIN_LENGTH, sizeof(char)); + pin_length = MAX_PIN_LENGTH; if (pin == NULL) { fail("Could not allocate memory for PIN"); } if (!get_pin(ui_method, callback_data) ) { - OPENSSL_cleanse(pin, MAX_PIN_LENGTH); + OPENSSL_cleanse(pin, pin_length); free(pin); pin = NULL; + pin_length = 0; fail("No pin code was entered"); } } @@ -709,9 +715,10 @@ static EVP_PKEY *pkcs11_load_key(ENGINE * e, const char *s_slot_key_id, if (PKCS11_login(slot, 0, pin)) { /* Login failed, so free the PIN if present */ if (pin != NULL) { - OPENSSL_cleanse(pin, MAX_PIN_LENGTH); + OPENSSL_cleanse(pin, pin_length); free(pin); pin = NULL; + pin_length = 0; } fail("Login failed\n"); }
commit e5a8f4157dbc70754f85aae7d0a6f69db02408d0 Author: David Smith <[email protected]> Date: Tue Oct 20 18:11:00 2009 +0900 Ignore CKR_CRYPTOKI_ALREADY_INITIALIZED error from C_Initialize() CKR_CRYPTOKI_ALREADY_INITIALIZED is an informational error message that can occur when a caller reuses a PKCS#11 module. It is not fatal. Given libp11's role as a wrapper for normal PKCS#11 API, it makes sense to swallow the error here and not pass it up to callers. Signed-off-by: David Smith <[email protected]> diff --git a/src/p11_load.c b/src/p11_load.c index c0b2aeb..115f9f6 100644 --- a/src/p11_load.c +++ b/src/p11_load.c @@ -73,7 +73,10 @@ int PKCS11_CTX_load(PKCS11_CTX * ctx, const char *name) memset(&args, 0, sizeof(args)); args.pReserved = priv->init_args; rv = priv->method->C_Initialize(&args); - CRYPTOKI_checkerr(PKCS11_F_PKCS11_CTX_LOAD, rv); + if (rv && rv != CKR_CRYPTOKI_ALREADY_INITIALIZED) { + PKCS11err(PKCS11_F_PKCS11_CTX_LOAD, rv); + return -1; + } /* Get info on the library */ rv = priv->method->C_GetInfo(&ck_info);
_______________________________________________ opensc-devel mailing list [email protected] http://www.opensc-project.org/mailman/listinfo/opensc-devel
