From 00b2adefccab211bb853c79ac84315dbd40ee05d Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Tue, 17 May 2016 23:07:20 -0400 Subject: Bugfixes to new error handling code, refactor some unreadable nested logic in handle lookup code. The mapping between PKCS #11 objects and libhal handles isn't quite right yet. This is a snapshot of bugfixes accumulated along the way, before refactoring mapping code to deal with the underlying problem. --- GNUmakefile | 13 ++++----- pkcs11.c | 85 ++++++++++++++++++++++++----------------------------------- unit_tests.py | 46 +++++++++++++++++++++++++------- 3 files changed, 78 insertions(+), 66 deletions(-) diff --git a/GNUmakefile b/GNUmakefile index 4a7df7c..dc41be5 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -140,16 +140,17 @@ ifneq "${HSMBULLY}" "" bully: all set -x; \ sudo rm -f ${HSMBULLY_DATABASE} ${HSMBULLY_DATABASE}-journal ${HSMBULLY_KS_CLIENT} ${HSMBULLY_KS_SERVER}; \ - if test -x ${HSMBULLY_SERVER_BIN}; \ + if test -x '${HSMBULLY_SERVER_BIN}'; \ then \ sudo CRYPTECH_KEYSTORE=${HSMBULLY_KS_SERVER} ${HSMBULLY_SERVER_BIN} & \ pid=$$!; \ - fi; \ - (echo fnord; echo fnord) | sudo ./p11util --set-so-pin --set-user-pin --pin-from-stdin; \ - sudo PKCS11_DATABASE=${HSMBULLY_DATABASE} CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ${HSMBULLY} ${HSMBULLY_OPTIONS}; \ - if test -x ${HSMBULLY_SERVER_BIN}; \ - then \ + sleep 5; \ + (echo fnord; echo fnord) | CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ./p11util --set-so-pin --set-user-pin --pin-from-stdin; \ + PKCS11_DATABASE=${HSMBULLY_DATABASE} CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ${HSMBULLY} ${HSMBULLY_OPTIONS}; \ sudo kill $$pid; \ + else \ + (echo fnord; echo fnord) | sudo CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ./p11util --set-so-pin --set-user-pin --pin-from-stdin; \ + sudo PKCS11_DATABASE=${HSMBULLY_DATABASE} CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ${HSMBULLY} ${HSMBULLY_OPTIONS}; \ fi; \ sudo rm -f ${HSMBULLY_DATABASE} ${HSMBULLY_DATABASE}-journal ${HSMBULLY_KS_CLIENT} ${HSMBULLY_KS_SERVER} diff --git a/pkcs11.c b/pkcs11.c index 3e47c67..4fde44f 100644 --- a/pkcs11.c +++ b/pkcs11.c @@ -317,11 +317,11 @@ static inline hal_error_t _hal_whine(const hal_error_t err, int ok = 0; hal_error_t code; - va_start(ap, line) - do { - code = va_arg(ap, hal_error_t); - ok |= (err == code); - } while (code != HAL_OK); + va_start(ap, line); + do { + code = va_arg(ap, hal_error_t); + ok |= (err == code); + } while (code != HAL_OK); va_end(ap); if (!ok) @@ -368,6 +368,9 @@ static CK_RV _p11_error_from_hal(const hal_error_t err, const char * const file, * More here later, first see if this compiles. */ + case HAL_OK: + return CKR_OK; + default: #if DEBUG_PKCS11 || DEBUG_HAL fprintf(stderr, "\n%s:%u: Mapping unhandled HAL error to CKR_FUNCTION_FAILED\n", file, line); @@ -1630,39 +1633,25 @@ static int p11_object_get_pkey_handle(const p11_session_t * const session, !sql_check_row(sqlite3_step(q))) goto fail; - switch (sqlite3_column_type(q, 0)) { + const int column_0_type = sqlite3_column_type(q, 0); + const int column_1_type = sqlite3_column_type(q, 1); - case SQLITE_INTEGER: + if (column_0_type == SQLITE_INTEGER) pkey_type = (hal_key_type_t) sqlite3_column_int64(q, 0); - break; - - case SQLITE_NULL: - if (!p11_object_pkey_type(object_handle, &pkey_type)) - goto fail; - break; - default: + else if (column_0_type != SQLITE_NULL || !p11_object_pkey_type(object_handle, &pkey_type)) goto fail; - } - switch (sqlite3_column_type(q, 1)) { - - case SQLITE_BLOB: - err = hal_whine_allow(hal_rpc_pkey_find(p11_session_hal_client(session), - p11_session_hal_session(session), pkey_handle, - pkey_type, sqlite3_column_blob(q, 1), - sqlite3_column_bytes(q, 1), - flags), + if (column_1_type == SQLITE_BLOB) + err = hal_whine_allow(hal_rpc_pkey_find(p11_session_hal_client(session), p11_session_hal_session(session), pkey_handle, + pkey_type, sqlite3_column_blob(q, 1), sqlite3_column_bytes(q, 1), flags), HAL_ERROR_KEY_NOT_FOUND); - break; - case SQLITE_NULL: - err = HAL_ERROR_KEY_NOT_FOUND; - break; + else if (column_1_type == SQLITE_NULL) + err = hal_whine(HAL_ERROR_KEY_NOT_FOUND); - default: + else goto fail; - } if (err == HAL_OK) ok = 1; @@ -2285,26 +2274,28 @@ static CK_RV generate_keypair_ec(p11_session_t *session, static CK_RV generate_keypair(p11_session_t *session, const CK_MECHANISM_PTR pMechanism, - const CK_ATTRIBUTE_PTR pPublicKeyTemplate, - const CK_ULONG ulPublicKeyAttributeCount, - const CK_ATTRIBUTE_PTR pPrivateKeyTemplate, - const CK_ULONG ulPrivateKeyAttributeCount, - CK_OBJECT_HANDLE_PTR phPublicKey, - CK_OBJECT_HANDLE_PTR phPrivateKey, CK_RV (*mechanism_handler)(p11_session_t *session, + const CK_ATTRIBUTE_PTR pPublicKeyTemplate, const CK_ULONG ulPublicKeyAttributeCount, const CK_OBJECT_HANDLE public_handle, const hal_key_flags_t public_flags, + const CK_ATTRIBUTE_PTR pPrivateKeyTemplate, const CK_ULONG ulPrivateKeyAttributeCount, const CK_OBJECT_HANDLE private_handle, const hal_key_flags_t private_flags), + const CK_ATTRIBUTE_PTR pPublicKeyTemplate, + const CK_ULONG ulPublicKeyAttributeCount, const p11_descriptor_t * const public_descriptor, - const p11_descriptor_t * const private_descriptor) + CK_OBJECT_HANDLE_PTR phPublicKey, + const CK_ATTRIBUTE_PTR pPrivateKeyTemplate, + const CK_ULONG ulPrivateKeyAttributeCount, + const p11_descriptor_t * const private_descriptor, + CK_OBJECT_HANDLE_PTR phPrivateKey) { - CK_OBJECT_HANDLE private_handle = CK_INVALID_HANDLE; CK_OBJECT_HANDLE public_handle = CK_INVALID_HANDLE; + CK_OBJECT_HANDLE private_handle = CK_INVALID_HANDLE; handle_flavor_t public_handle_flavor = handle_flavor_session_object; handle_flavor_t private_handle_flavor = handle_flavor_session_object; hal_key_flags_t public_flags = 0; @@ -4295,23 +4286,15 @@ CK_RV C_GenerateKeyPair(CK_SESSION_HANDLE hSession, switch (pMechanism->mechanism) { case CKM_RSA_PKCS_KEY_PAIR_GEN: - rv = generate_keypair(session, pMechanism, - pPublicKeyTemplate, ulPublicKeyAttributeCount, - pPrivateKeyTemplate, ulPrivateKeyAttributeCount, - phPublicKey, phPrivateKey, - generate_keypair_rsa_pkcs, - &p11_descriptor_rsa_public_key, - &p11_descriptor_rsa_private_key); + rv = generate_keypair(session, pMechanism, generate_keypair_rsa_pkcs, + pPublicKeyTemplate, ulPublicKeyAttributeCount, &p11_descriptor_rsa_public_key, phPublicKey, + pPrivateKeyTemplate, ulPrivateKeyAttributeCount, &p11_descriptor_rsa_private_key, phPrivateKey); break; case CKM_EC_KEY_PAIR_GEN: - rv = generate_keypair(session, pMechanism, - pPublicKeyTemplate, ulPublicKeyAttributeCount, - pPrivateKeyTemplate, ulPrivateKeyAttributeCount, - phPublicKey, phPrivateKey, - generate_keypair_ec, - &p11_descriptor_ec_public_key, - &p11_descriptor_ec_private_key); + rv = generate_keypair(session, pMechanism, generate_keypair_ec, + pPublicKeyTemplate, ulPublicKeyAttributeCount, &p11_descriptor_ec_public_key, phPublicKey, + pPrivateKeyTemplate, ulPrivateKeyAttributeCount, &p11_descriptor_ec_private_key, phPrivateKey); break; default: diff --git a/unit_tests.py b/unit_tests.py index f5553d4..6866a87 100644 --- a/unit_tests.py +++ b/unit_tests.py @@ -272,7 +272,6 @@ class TestKeys(unittest.TestCase): p11.C_VerifyInit(self.session, CKM_ECDSA_SHA384, public_key) p11.C_Verify(self.session, hamster, sig) - def test_gen_sign_verify_ecdsa_p521_sha512(self): #if not args.all_tests: self.skipTest("SHA-512 not available in current build") public_key, private_key = p11.C_GenerateKeyPair(self.session, CKM_EC_KEY_PAIR_GEN, @@ -286,16 +285,45 @@ class TestKeys(unittest.TestCase): p11.C_VerifyInit(self.session, CKM_ECDSA_SHA512, public_key) p11.C_Verify(self.session, hamster, sig) - def test_gen_rsa_1024(self): - self.assertIsKeypair( - p11.C_GenerateKeyPair(self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 1024, - CKA_ID = "RSA-1024", CKA_SIGN = True, CKA_VERIFY = True)) + def test_gen_sign_verify_rsa_1024(self): + public_key, private_key = p11.C_GenerateKeyPair( + self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 1024, + CKA_ID = "RSA-1024", CKA_SIGN = True, CKA_VERIFY = True) + self.assertIsKeypair(public_key, private_key) + hamster = "Your mother was a hamster" + p11.C_SignInit(self.session, CKM_SHA512_RSA_PKCS, private_key) + sig = p11.C_Sign(self.session, hamster) + self.assertIsInstance(sig, str) + p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, public_key) + p11.C_Verify(self.session, hamster, sig) - def test_gen_rsa_2048(self): + if False: + a = p11.C_GetAttributeValue(self.session, public_key, + CKA_CLASS, CKA_KEY_TYPE, CKA_VERIFY, CKA_TOKEN, + CKA_PUBLIC_EXPONENT, CKA_MODULUS) + a[CKA_TOKEN] = not a[CKA_TOKEN] + o = p11.C_CreateObject(self.session, a) + p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, o) + p11.C_Verify(self.session, hamster, sig) + + self.tearDown() + self.setUp() + o = p11.C_CreateObject(self.session, a) + p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, o) + p11.C_Verify(self.session, hamster, sig) + + def test_gen_sign_verify_rsa_2048(self): if not args.all_tests: self.skipTest("RSA key generation is still painfully slow") - self.assertIsKeypair( - p11.C_GenerateKeyPair(self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 2048, - CKA_ID = "RSA-1024", CKA_SIGN = True, CKA_VERIFY = True)) + public_key, private_key = p11.C_GenerateKeyPair( + self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 2048, + CKA_ID = "RSA-2048", CKA_SIGN = True, CKA_VERIFY = True) + self.assertIsKeypair(public_key, private_key) + hamster = "Your mother was a hamster" + p11.C_SignInit(self.session, CKM_SHA512_RSA_PKCS, private_key) + sig = p11.C_Sign(self.session, hamster) + self.assertIsInstance(sig, str) + p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, public_key) + p11.C_Verify(self.session, hamster, sig) @staticmethod def _build_ecpoint(x, y): -- cgit v1.2.3