Support split keypairs, where private key is a token object and public

key is a session object.  Doesn't actually save us anything, but Jakob
tells us that this makes a difference on some HSMs so we people use
this kind of setup and we need to support it.

Explicitly disallow private keys as session objects, since we have no
way to protect them.  Update unit-tests now that we return the correct
error code for this case.

2 files changed, 58 insertions, 37 deletions

diff --git a/pkcs11.c b/pkcs11.c
index dcb418b..dc9bb5b 100644
--- a/pkcs11.c
+++ b/pkcs11.c
@@ -1428,7 +1428,8 @@ static CK_OBJECT_HANDLE p11_object_create(const p11_session_t *session,
  */
 
 static int p11_object_bind_pkey(const p11_session_t * const session,
-                                const hal_key_type_t pkey_type,
+                                const hal_key_type_t pkey_type_1,
+                                const hal_key_type_t pkey_type_2,
                                 const CK_OBJECT_HANDLE object_handle_1,
                                 const CK_OBJECT_HANDLE object_handle_2,
                                 const uint8_t * const der, const size_t der_len,
@@ -1437,7 +1438,7 @@ static int p11_object_bind_pkey(const p11_session_t * const session,
   assert(session != NULL && der != NULL && ski != NULL);
 
   static const char update_pkey_ski[] =
-    " UPDATE object SET hal_pkey_type = ?1, hal_pkey_ski = ?2 WHERE object_handle = ?3 OR object_handle = ?4";
+    " UPDATE object SET hal_pkey_type = ?1, hal_pkey_ski = ?2 WHERE object_handle = ?3";
 
   hal_hash_handle_t hash = {HAL_HANDLE_NONE};
 
@@ -1456,12 +1457,17 @@ static int p11_object_bind_pkey(const p11_session_t * const session,
   sqlite3_stmt *q = NULL;
 
   ok = (sql_check_ok(sql_prepare(&q, update_pkey_ski))                  &&
-        sql_check_ok(sqlite3_bind_int64(q, 1, pkey_type))               &&
+        sql_check_ok(sqlite3_bind_int64(q, 1, pkey_type_1))             &&
         sql_check_ok(sqlite3_bind_blob( q, 2, ski, ski_len, NULL))      &&
         sql_check_ok(sqlite3_bind_int64(q, 3, object_handle_1))         &&
-        sql_check_ok(sqlite3_bind_int64(q, 4, object_handle_2))         &&
         sql_check_done(sqlite3_step(q)));
 
+  if (ok && object_handle_2 != CK_INVALID_HANDLE)
+    ok = (sql_check_ok(sqlite3_reset(q))                                &&
+          sql_check_ok(sqlite3_bind_int64(q, 1, pkey_type_2))           &&
+          sql_check_ok(sqlite3_bind_int64(q, 3, object_handle_2))       &&
+          sql_check_done(sqlite3_step(q)));
+
   sqlite3_finalize(q);
   return ok;
 }
@@ -1503,7 +1509,7 @@ static inline int p11_object_create_rsa_public_key(const p11_session_t * const s
   if (ok) {
     uint8_t der[hal_rsa_public_key_to_der_len(key)], ski[ski_len];
     ok = (hal_check(hal_rsa_public_key_to_der(key, der, NULL, sizeof(der)))                     &&
-          p11_object_bind_pkey(session, HAL_KEY_TYPE_RSA_PUBLIC,
+          p11_object_bind_pkey(session, HAL_KEY_TYPE_RSA_PUBLIC, HAL_KEY_TYPE_NONE,
                                object_handle, CK_INVALID_HANDLE,
                                der, sizeof(der), ski, sizeof(ski))                              &&
           hal_check(hal_rpc_pkey_load(p11_session_hal_client(session),
@@ -1551,7 +1557,7 @@ static inline int p11_object_create_ec_public_key(const p11_session_t * const se
   if (ok) {
     uint8_t der[hal_ecdsa_public_key_to_der_len(key)], ski[ski_len];
     ok = (hal_check(hal_ecdsa_public_key_to_der(key, der, NULL, sizeof(der)))                   &&
-          p11_object_bind_pkey(session, HAL_KEY_TYPE_EC_PUBLIC,
+          p11_object_bind_pkey(session, HAL_KEY_TYPE_EC_PUBLIC, HAL_KEY_TYPE_NONE,
                                object_handle, CK_INVALID_HANDLE,
                                der, sizeof(der), ski, sizeof(ski))                              &&
           hal_check(hal_rpc_pkey_load(p11_session_hal_client(session),
@@ -2051,13 +2057,13 @@ static CK_RV generate_keypair_rsa_pkcs(p11_session_t *session,
 {
   const uint8_t *public_exponent = const_0x010001;
   size_t public_exponent_len = sizeof(const_0x010001);
-  hal_pkey_handle_t pkey = {HAL_HANDLE_NONE};
+  hal_pkey_handle_t pkey1 = {HAL_HANDLE_NONE}, pkey2 = {HAL_HANDLE_NONE};
   CK_ULONG keysize = 0;
   size_t ski_len = 0;
   CK_RV rv;
   int i;
 
-  assert(pPublicKeyTemplate  != NULL && pPrivateKeyTemplate);
+  assert(session != NULL && pPublicKeyTemplate != NULL && pPrivateKeyTemplate != NULL && is_token_handle(private_handle));
 
   for (i = 0; i < ulPublicKeyAttributeCount; i++) {
     const CK_ATTRIBUTE_TYPE type = pPublicKeyTemplate[i].type;
@@ -2087,20 +2093,23 @@ static CK_RV generate_keypair_rsa_pkcs(p11_session_t *session,
 
   if (!hal_check(hal_rpc_pkey_generate_rsa(p11_session_hal_client(session),
                                            p11_session_hal_session(session),
-                                           &pkey, (const uint8_t *) "", 0, keysize,
+                                           &pkey1, (const uint8_t *) "", 0, keysize,
                                            public_exponent, public_exponent_len,
                                            private_flags)))
     lose(CKR_FUNCTION_FAILED);
 
   {
-    uint8_t der[hal_rpc_pkey_get_public_key_len(pkey)], keybuf[hal_rsa_key_t_size], ski[ski_len];
+    uint8_t der[hal_rpc_pkey_get_public_key_len(pkey1)], keybuf[hal_rsa_key_t_size], ski[ski_len];
     size_t der_len, modulus_len;
     hal_rsa_key_t *key = NULL;
 
-    if (!hal_check(hal_rpc_pkey_get_public_key(pkey, der, &der_len, sizeof(der)))               ||
-        !p11_object_bind_pkey(session, HAL_KEY_TYPE_RSA_PRIVATE, private_handle, public_handle,
+    const hal_key_type_t pkey2_type = is_token_handle(public_handle) ? HAL_KEY_TYPE_RSA_PRIVATE : HAL_KEY_TYPE_RSA_PUBLIC;
+
+    if (!hal_check(hal_rpc_pkey_get_public_key(pkey1, der, &der_len, sizeof(der)))              ||
+        !p11_object_bind_pkey(session, HAL_KEY_TYPE_RSA_PRIVATE, pkey2_type,
+                              private_handle, public_handle,
                               der, sizeof(der), ski, sizeof(ski))                               ||
-        !hal_check(hal_rpc_pkey_rename(pkey, ski, sizeof(ski)))                                 ||
+        !hal_check(hal_rpc_pkey_rename(pkey1, ski, sizeof(ski)))                                ||
         !hal_check(hal_rsa_public_key_from_der(&key, keybuf, sizeof(keybuf), der, der_len))     ||
         !hal_check(hal_rsa_key_get_modulus(key, NULL, &modulus_len, 0)))
       lose(CKR_FUNCTION_FAILED);
@@ -2111,12 +2120,20 @@ static CK_RV generate_keypair_rsa_pkcs(p11_session_t *session,
         !p11_attribute_set(public_handle,  CKA_MODULUS, modulus, modulus_len)           ||
         !p11_attribute_set(private_handle, CKA_MODULUS, modulus, modulus_len))
       lose(CKR_FUNCTION_FAILED);
+
+    if (!is_token_handle(public_handle) &&
+        !hal_check(hal_rpc_pkey_load(p11_session_hal_client(session),
+                                     p11_session_hal_session(session),
+                                     &pkey2, HAL_KEY_TYPE_RSA_PUBLIC, HAL_CURVE_NONE,
+                                     ski, sizeof(ski), der, der_len, public_flags)))
+      lose(CKR_FUNCTION_FAILED);
   }
 
   rv = CKR_OK;
 
  fail:
-  hal_rpc_pkey_close(pkey);
+  hal_rpc_pkey_close(pkey1);
+  hal_rpc_pkey_close(pkey2);
   return rv;
 }
 
@@ -2134,7 +2151,7 @@ static CK_RV generate_keypair_ec(p11_session_t *session,
                                  const CK_OBJECT_HANDLE private_handle,
                                  const hal_key_flags_t private_flags)
 {
-  hal_pkey_handle_t pkey = {HAL_HANDLE_NONE};
+  hal_pkey_handle_t pkey1 = {HAL_HANDLE_NONE}, pkey2 = {HAL_HANDLE_NONE};
   const CK_BYTE *params = NULL;
   hal_curve_name_t curve;
   size_t params_len;
@@ -2142,7 +2159,7 @@ static CK_RV generate_keypair_ec(p11_session_t *session,
   CK_RV rv;
   int i;
 
-  assert(session != NULL && pPublicKeyTemplate != NULL && pPrivateKeyTemplate != NULL);
+  assert(session != NULL && pPublicKeyTemplate != NULL && pPrivateKeyTemplate != NULL && is_token_handle(private_handle));
 
   for (i = 0; i < ulPublicKeyAttributeCount; i++) {
     const CK_ATTRIBUTE_TYPE type = pPublicKeyTemplate[i].type;
@@ -2166,21 +2183,24 @@ static CK_RV generate_keypair_ec(p11_session_t *session,
 
   if (!hal_check(hal_rpc_pkey_generate_ec(p11_session_hal_client(session),
                                           p11_session_hal_session(session),
-                                          &pkey, (const uint8_t *) "", 0, curve,
-                                          private_flags))                       ||
+                                          &pkey1, (const uint8_t *) "", 0,
+                                          curve, private_flags))                ||
       !p11_attribute_set(public_handle,  CKA_EC_PARAMS, params, params_len)     ||
       !p11_attribute_set(private_handle, CKA_EC_PARAMS, params, params_len))
     lose(CKR_FUNCTION_FAILED);
 
   {
-    uint8_t der[hal_rpc_pkey_get_public_key_len(pkey)], keybuf[hal_ecdsa_key_t_size], ski[ski_len];
+    uint8_t der[hal_rpc_pkey_get_public_key_len(pkey1)], keybuf[hal_ecdsa_key_t_size], ski[ski_len];
     hal_ecdsa_key_t *key = NULL;
     size_t der_len;
 
-    if (!hal_check(hal_rpc_pkey_get_public_key(pkey, der, &der_len, sizeof(der)))               ||
-        !p11_object_bind_pkey(session, HAL_KEY_TYPE_EC_PRIVATE, private_handle, public_handle,
+    const hal_key_type_t pkey2_type = is_token_handle(public_handle) ? HAL_KEY_TYPE_EC_PRIVATE : HAL_KEY_TYPE_EC_PUBLIC;
+
+    if (!hal_check(hal_rpc_pkey_get_public_key(pkey1, der, &der_len, sizeof(der)))              ||
+        !p11_object_bind_pkey(session, HAL_KEY_TYPE_EC_PRIVATE, pkey2_type,
+                              private_handle, public_handle,
                               der, sizeof(der), ski, sizeof(ski))                               ||
-        !hal_check(hal_rpc_pkey_rename(pkey, ski, sizeof(ski)))                                 ||
+        !hal_check(hal_rpc_pkey_rename(pkey1, ski, sizeof(ski)))                                ||
         !hal_check(hal_ecdsa_public_key_from_der(&key, keybuf, sizeof(keybuf), der, der_len)))
       lose(CKR_FUNCTION_FAILED);
 
@@ -2189,18 +2209,30 @@ static CK_RV generate_keypair_ec(p11_session_t *session,
     if (!hal_check(hal_ecdsa_key_to_ecpoint(key, point, NULL, sizeof(point)))   ||
         !p11_attribute_set(public_handle,  CKA_EC_POINT, point, sizeof(point)))
       lose(CKR_FUNCTION_FAILED);
+
+    if (!is_token_handle(public_handle) &&
+        !hal_check(hal_rpc_pkey_load(p11_session_hal_client(session),
+                                     p11_session_hal_session(session),
+                                     &pkey2, HAL_KEY_TYPE_EC_PUBLIC, curve,
+                                     ski, sizeof(ski), der, der_len, public_flags)))
+      lose(CKR_FUNCTION_FAILED);
   }
 
   rv = CKR_OK;
 
  fail:
-  hal_rpc_pkey_close(pkey);
+  hal_rpc_pkey_close(pkey1);
+  hal_rpc_pkey_close(pkey2);
   return rv;
 }
 
 /*
  * Key pair generation.  This needs a mechanism-specific function to
  * do the inner bits, but there's a lot of boilerplate.
+ *
+ * We have no real way to protect session objects, so we don't allow
+ * them to hold private keys.  PKCS #11 wants us to report this kind
+ * of restriction as a template inconsistency.
  */
 
 static CK_RV generate_keypair(p11_session_t *session,
@@ -2256,7 +2288,7 @@ static CK_RV generate_keypair(p11_session_t *session,
     public_flags |= HAL_KEY_FLAG_PROXIMATE;
 
   if (private_handle_flavor == handle_flavor_session_object)
-    private_flags |= HAL_KEY_FLAG_PROXIMATE;
+    return CKR_TEMPLATE_INCONSISTENT;
 
   if (!sql_exec("BEGIN"))
     lose(CKR_FUNCTION_FAILED);
diff --git a/unit_tests.py b/unit_tests.py
index 4d09b6a..e218b42 100644
--- a/unit_tests.py
+++ b/unit_tests.py
@@ -234,18 +234,7 @@ class TestKeys(unittest.TestCase):
 
     def test_keygen_token_vs_session(self):
 
-        # XXX pkcs11.c currently generates the wrong error code if the
-        #     user tries to generate a keypair with the private key as
-        #     a session object.  Refusing to allow this is deliberate
-        #     (we have no way to protect such private keys), but
-        #     returning CKR_FUNCTION_FAILED is wrong.  Fixing this
-        #     will require minor work in pkcs11.c and perhaps in libhal.
-        #
-        #     For the moment, I'm just testing for the (known) wrong
-        #     exception while I make sure that the library is in fact
-        #     behaving as I expect it to behave.
-
-        with self.assertRaises(CKR_FUNCTION_FAILED):
+        with self.assertRaises(CKR_TEMPLATE_INCONSISTENT):
             p11.C_GenerateKeyPair(self.session, CKM_EC_KEY_PAIR_GEN, CKA_TOKEN = False,
                                   CKA_ID = "EC-P256", CKA_EC_PARAMS = self.oid_p256,
                                   CKA_SIGN = True, CKA_VERIFY = True)
@@ -261,7 +250,7 @@ class TestKeys(unittest.TestCase):
                                   CKA_ID = "EC-P256", CKA_EC_PARAMS = self.oid_p256,
                                   CKA_SIGN = True, CKA_VERIFY = True))
 
-        with self.assertRaises(CKR_FUNCTION_FAILED):
+        with self.assertRaises(CKR_TEMPLATE_INCONSISTENT):
             p11.C_GenerateKeyPair(self.session, CKM_EC_KEY_PAIR_GEN,
                                   public_CKA_TOKEN = True, private_CKA_TOKEN = False,
                                   CKA_ID = "EC-P256", CKA_EC_PARAMS = self.oid_p256,