6540060 race in pkcs#11 engine in multithreaded environment
authorjp161948
Tue, 15 May 2007 12:40:20 -0700
changeset 4245 daa332a90913
parent 4244 77e7b46e3d5e
child 4246 fd4303222aba
6540060 race in pkcs#11 engine in multithreaded environment 6554248 OpenSSL pkcs#11 engine doesn't strip leading zeros from a computed Diffie-Hellman shared secret
usr/src/common/openssl/crypto/engine/hw_pk11.c
usr/src/common/openssl/crypto/engine/hw_pk11_pub.c
--- a/usr/src/common/openssl/crypto/engine/hw_pk11.c	Tue May 15 12:25:41 2007 -0700
+++ b/usr/src/common/openssl/crypto/engine/hw_pk11.c	Tue May 15 12:40:20 2007 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright 2006 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 
@@ -1516,11 +1516,19 @@
 	a_key_template[5].pValue = (void *) key;
 	a_key_template[5].ulValueLen = (unsigned long) ctx->key_len;
 
+	/*
+	 * As stated in v2.20, 11.7 Object Management Function, in section for
+	 * C_FindObjectsInit(), at most one search operation may be active at
+	 * a given time in a given session. Therefore, we must group these
+	 * three calls in one atomic operation.
+	 */
+	CRYPTO_w_lock(CRYPTO_LOCK_PK11_ENGINE);
 	rv = pFuncList->C_FindObjectsInit(session, a_key_template, 
 		ul_key_attr_count);
 
 	if (rv != CKR_OK)
 		{
+		CRYPTO_w_unlock(CRYPTO_LOCK_PK11_ENGINE);
 		PK11err(PK11_F_GET_CIPHER_KEY, PK11_R_FINDOBJECTSINIT);
 		snprintf(tmp_buf, sizeof (tmp_buf), "%lx", rv);
 		ERR_add_error_data(2, "PK11 CK_RV=0X", tmp_buf);
@@ -1531,6 +1539,7 @@
 
 	if (rv != CKR_OK)
 		{
+		CRYPTO_w_unlock(CRYPTO_LOCK_PK11_ENGINE);
 		PK11err(PK11_F_GET_CIPHER_KEY, PK11_R_FINDOBJECTS);
 		snprintf(tmp_buf, sizeof (tmp_buf), "%lx", rv);
 		ERR_add_error_data(2, "PK11 CK_RV=0X", tmp_buf);
@@ -1538,6 +1547,7 @@
 		}
 
 	rv = pFuncList->C_FindObjectsFinal(session);
+	CRYPTO_w_unlock(CRYPTO_LOCK_PK11_ENGINE);
 
 	if (rv != CKR_OK)
 		{
--- a/usr/src/common/openssl/crypto/engine/hw_pk11_pub.c	Tue May 15 12:25:41 2007 -0700
+++ b/usr/src/common/openssl/crypto/engine/hw_pk11_pub.c	Tue May 15 12:40:20 2007 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright 2006 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 #pragma ident	"%Z%%M%	%I%	%E% SMI"
@@ -1981,19 +1981,18 @@
 
 static int pk11_DH_compute_key(unsigned char *key,const BIGNUM *pub_key,DH *dh)
 	{
-	CK_ULONG i;
+	int i;
 	CK_MECHANISM mechanism = {CKM_DH_PKCS_DERIVE, NULL_PTR, 0};
 	CK_OBJECT_CLASS key_class = CKO_SECRET_KEY;
 	CK_KEY_TYPE key_type = CKK_GENERIC_SECRET;
 	CK_OBJECT_HANDLE h_derived_key = CK_INVALID_HANDLE;
 	CK_OBJECT_HANDLE h_key = CK_INVALID_HANDLE;
 
-	CK_ULONG ul_priv_key_attr_count = 3; 
+	CK_ULONG ul_priv_key_attr_count = 2;
 	CK_ATTRIBUTE priv_key_template[] =
 		{
 		{CKA_CLASS, (void*) NULL, sizeof(key_class)},
 		{CKA_KEY_TYPE, (void*) NULL,  sizeof(key_type)},
-		{CKA_VALUE_LEN, (void*) NULL,  sizeof(i)},
 		};
 
 	CK_ULONG priv_key_attr_result_count = 1;
@@ -2034,8 +2033,6 @@
 		goto err;
 		}
 
-	i = mechanism.ulParameterLen;
-	priv_key_template[2].pValue = &i;
 	rv = pFuncList->C_DeriveKey(sp->session,
 			   &mechanism,
 			   h_key,
@@ -2089,9 +2086,23 @@
 	 * length of the public key. It is long enough for the derived key */
 	if (priv_key_result[0].type == CKA_VALUE)
 		{
-		memcpy(key, priv_key_result[0].pValue, 
-			priv_key_result[0].ulValueLen);
-		ret = priv_key_result[0].ulValueLen;
+		/* CKM_DH_PKCS_DERIVE mechanism is not supposed to strip
+		 * leading zeros from a computed shared secret. However,
+		 * OpenSSL always did it so we must do the same here. The
+		 * vagueness of the spec regarding leading zero bytes was
+		 * finally cleared with TLS 1.1 (RFC 4346) saying that leading
+		 * zeros are stripped before the computed data is used as the
+		 * pre-master secret.
+		 */
+		for (i = 0; i < priv_key_result[0].ulValueLen; ++i)
+			{
+			if (((char *) priv_key_result[0].pValue)[i] != 0)
+				break;
+			}
+
+		memcpy(key, ((char *) priv_key_result[0].pValue) + i, 
+			priv_key_result[0].ulValueLen - i);
+		ret = priv_key_result[0].ulValueLen - i;
 		}
 
 err: