Flesh out key object access control.

This is more complicated than I'd have liked, because the PKCS #11
semantics are (much) more complicated than just "are you logged in?"

New code passes basic testing with libhal.py and the PKCS #11 unit
tests, but there are still unexplored corner cases to be checked.

Private token objects remain simple.  Code which does not need PKCS
HAL_KEY_FLAG_TOKEN and avoid HAL_KEY_FLAG_PUBLIC.

4 files changed, 112 insertions, 13 deletions

diff --git a/hal.h b/hal.h
index 194948a..db4038d 100644
--- a/hal.h
+++ b/hal.h
@@ -692,6 +692,7 @@ typedef uint32_t hal_key_flags_t;
 #define	HAL_KEY_FLAG_USAGE_KEYENCIPHERMENT      (1 << 1)
 #define	HAL_KEY_FLAG_USAGE_DATAENCIPHERMENT	(1 << 2)
 #define	HAL_KEY_FLAG_TOKEN                      (1 << 3)
+#define HAL_KEY_FLAG_PUBLIC                     (1 << 4)
 
 extern hal_error_t hal_rpc_pkey_load(const hal_client_handle_t client,
                                      const hal_session_handle_t session,
diff --git a/ks_volatile.c b/ks_volatile.c
index e88b871..0f53c11 100644
--- a/ks_volatile.c
+++ b/ks_volatile.c
@@ -106,14 +106,29 @@ static inline ks_t *ks_to_ksv(hal_ks_t *ks)
   return (ks_t *) ks;
 }
 
+/*
+ * Check whether the current session can see a particular key.  One
+ * might expect this to be based on whether the session matches, and
+ * indeed it would be in a sane world, but in the world of PKCS #11,
+ * keys belong to sessions, are visible to other sessions, and may
+ * even be modifiable by other sessions, but softly and silently
+ * vanish away when the original creating session is destroyed.
+ *
+ * In our terms, this means that visibility of session objects is
+ * determined only by the client handle, so taking the session handle
+ * as an argument here isn't really necessary, but we've flipflopped
+ * on that enough times that at least for now I'd prefer to leave the
+ * session handle here and not have to revise all the RPC calls again.
+ * Remove it at some later date and redo the RPC calls if we manage to
+ * avoid revising this yet again.
+ */
+
 static inline int key_visible_to_session(const ks_t * const ksv,
                                          const hal_client_handle_t client,
                                          const hal_session_handle_t session,
                                          const ks_key_t * const k)
 {
-  return (!ksv->per_session || client.handle == HAL_HANDLE_NONE ||
-          (k->client.handle  == client.handle &&
-           k->session.handle == session.handle));
+  return !ksv->per_session || client.handle == HAL_HANDLE_NONE || k->client.handle  == client.handle;
 }
 
 static inline void *gnaw(uint8_t **mem, size_t *len, const size_t size)
diff --git a/libhal.py b/libhal.py
index 8334f12..c35a51a 100644
--- a/libhal.py
+++ b/libhal.py
@@ -224,7 +224,7 @@ HAL_KEY_FLAG_USAGE_DIGITALSIGNATURE     = (1 << 0)
 HAL_KEY_FLAG_USAGE_KEYENCIPHERMENT      = (1 << 1)
 HAL_KEY_FLAG_USAGE_DATAENCIPHERMENT     = (1 << 2)
 HAL_KEY_FLAG_TOKEN                      = (1 << 3)
-
+HAL_KEY_FLAG_PUBLIC                     = (1 << 4)
 
 class Attribute(object):
 
@@ -590,10 +590,17 @@ class HSM(object):
 
 if __name__ == "__main__":
 
+    import argparse
+
     def hexstr(s):
         return "".join("{:02x}".format(ord(c)) for c in s)
 
-    hsm = HSM()
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--device", default = os.getenv("CRYPTECH_RPC_CLIENT_SERIAL_DEVICE", "/dev/ttyUSB0"))
+    parser.add_argument("--pin", default = "fnord")
+    args = parser.parse_args()
+
+    hsm = HSM(device = args.device)
 
     print "Version:", hex(hsm.get_version())
 
@@ -607,6 +614,10 @@ if __name__ == "__main__":
     h.update("Hi, Dad")
     print "HMAC:", hexstr(h.finalize())
 
+    print "Logging in"
+    hsm.login(HAL_USER_NORMAL, args.pin)
+
+    print "Generating key"
     k = hsm.pkey_generate_ec(HAL_CURVE_P256)
     print "PKey: {0.uuid} {0.key_type} {0.key_flags} {1}".format(k, hexstr(k.public_key))
     hsm.pkey_close(k)
@@ -621,3 +632,5 @@ if __name__ == "__main__":
 
     k = hsm.pkey_find(k.uuid)
     hsm.pkey_delete(k)
+
+    hsm.logout()
diff --git a/rpc_pkey.c b/rpc_pkey.c
index 053c8b4..493dec8 100644
--- a/rpc_pkey.c
+++ b/rpc_pkey.c
@@ -101,18 +101,61 @@ static inline hal_pkey_slot_t *find_handle(const hal_pkey_handle_t handle)
   return NULL;
 }
 
-#warning Still need access control on pkey objects based on current login state
 /*
- * This would be simple, except for PKCS #11 non-token objects (CKA_TOKEN = CK_FALSE).
- * Need to check detailed PKCS #11 rules, but, from memory, we may be supposed to allow
- * access to non-token objects even when not logged in.  Maybe.  Rules are complex.
+ * Access rules are a bit complicated, mostly due to PKCS #11.
  *
- * I think the libhal translation of this resolves around HAL_KEY_FLAG_TOKEN.
- * For token objects, we insist on being logged in properly; for non-token
- * objects, we do whatever silly thing PKCS #11 wants us to do, probably
- * defaulting to requiring login if PKCS #11 gives us a choice.
+ * The simple, obvious rule would be that one must be logged in as
+ * HAL_USER_NORMAL to create, see, use, or delete a key, full stop.
+ *
+ * That's almost the rule that PKCS #11 follows for so-called
+ * "private" objects (CKA_PRIVATE = CK_TRUE), but PKCS #11 has a more
+ * model which not only allows wider visibility to "public" objects
+ * (CKA_PRIVATE = CK_FALSE) but also allows write access to "public
+ * session" (CKA_PRIVATE = CK_FALSE, CKA_TOKEN = CK_FALSE) objects
+ * regardless of login state.
+ *
+ * PKCS #11 also has a concept of read-only sessions, which we don't
+ * bother to implement at all on the HSM, since the PIN is required to
+ * be the same as for the corresponding read-write session, so this
+ * would just be additional compexity without adding any security on
+ * the HSM; the PKCS #11 library still has to support read-only
+ * sessions, but that's not our problem here.
+ *
+ * In general, non-PKCS #11 users of this API should probably never
+ * set HAL_KEY_FLAG_PUBLIC, in which case they'll get the simple rule.
+ *
+ * Note that keystore drivers may need to implement additional
+ * additional checks, eg, ks_volatile needs to enforce the rule that
+ * session objects are only visible to the client which created them
+ * (not the session, that would be too simple, thanks PKCS #11).  In
+ * practice, this should not be a serious problem, since such checks
+ * will likely only apply to existing objects.  The thing we really
+ * want to avoid is doing all the work to create a large key only to
+ * have the keystore driver reject access at the end, but since, by
+ * definition, that only occurs when creating new objects, the access
+ * decision doesn't depend on preexisting data, so the rules here
+ * should suffice.  That's the theory, anyway, if this is wrong we may
+ * need to refactor.
  */
 
+static inline hal_error_t check_readable(const hal_client_handle_t client,
+                                         const hal_key_flags_t flags)
+{
+  if ((flags & HAL_KEY_FLAG_PUBLIC) != 0)
+    return HAL_OK;
+
+  return hal_rpc_is_logged_in(client, HAL_USER_NORMAL);
+}
+
+static inline hal_error_t check_writable(const hal_client_handle_t client,
+                                         const hal_key_flags_t flags)
+{
+  if ((flags & (HAL_KEY_FLAG_TOKEN | HAL_KEY_FLAG_PUBLIC)) == HAL_KEY_FLAG_PUBLIC)
+    return HAL_OK;
+
+  return hal_rpc_is_logged_in(client, HAL_USER_NORMAL);
+}
+
 /*
  * Pad an octet string with PKCS #1.5 padding for use with RSA.
  *
@@ -186,6 +229,9 @@ static hal_error_t pkey_local_load(const hal_client_handle_t client,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_writable(client, flags)) != HAL_OK)
+    return err;
+
   if ((slot = alloc_slot(flags)) == NULL)
     return HAL_ERROR_NO_KEY_SLOTS_AVAILABLE;
 
@@ -230,6 +276,9 @@ static hal_error_t pkey_local_find(const hal_client_handle_t client,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_readable(client, flags)) != HAL_OK)
+    return err;
+
   if ((slot = alloc_slot(flags)) == NULL)
     return HAL_ERROR_NO_KEY_SLOTS_AVAILABLE;
 
@@ -272,6 +321,9 @@ static hal_error_t pkey_local_generate_rsa(const hal_client_handle_t client,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_writable(client, flags)) != HAL_OK)
+    return err;
+
   if ((slot = alloc_slot(flags)) == NULL)
     return HAL_ERROR_NO_KEY_SLOTS_AVAILABLE;
 
@@ -333,6 +385,9 @@ static hal_error_t pkey_local_generate_ec(const hal_client_handle_t client,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_writable(client, flags)) != HAL_OK)
+    return err;
+
   if ((slot = alloc_slot(flags)) == NULL)
     return HAL_ERROR_NO_KEY_SLOTS_AVAILABLE;
 
@@ -403,6 +458,9 @@ static hal_error_t pkey_local_delete(const hal_pkey_handle_t pkey)
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_writable(slot->client_handle, slot->flags)) != HAL_OK)
+    return err;
+
   if ((err = ks_open_from_flags(&ks, slot->flags)) == HAL_OK &&
       (err = hal_ks_delete(ks, slot)) == HAL_OK)
     err = hal_ks_close(ks);
@@ -859,6 +917,9 @@ static hal_error_t pkey_local_list(const hal_client_handle_t client,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_readable(client, flags)) != HAL_OK)
+    return err;
+
   if ((err = ks_open_from_flags(&ks, flags)) == HAL_OK &&
       (err = hal_ks_list(ks, client, session, result, result_len, result_max)) == HAL_OK)
     err = hal_ks_close(ks);
@@ -883,6 +944,9 @@ static hal_error_t pkey_local_match(const hal_client_handle_t client,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_readable(client, flags)) != HAL_OK)
+    return err;
+
   if ((err = ks_open_from_flags(&ks, flags)) == HAL_OK &&
       (err = hal_ks_match(ks, client, session, type, curve, flags, attributes, attributes_len,
                           result, result_len, result_max, previous_uuid)) == HAL_OK)
@@ -906,6 +970,9 @@ static hal_error_t pkey_local_set_attribute(const hal_pkey_handle_t pkey,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_writable(slot->client_handle, slot->flags)) != HAL_OK)
+    return err;
+
   if ((err = ks_open_from_flags(&ks, slot->flags)) == HAL_OK &&
       (err = hal_ks_set_attribute(ks, slot, type, value, value_len)) == HAL_OK)
     err = hal_ks_close(ks);
@@ -949,6 +1016,9 @@ static hal_error_t pkey_local_delete_attribute(const hal_pkey_handle_t pkey,
   hal_ks_t *ks = NULL;
   hal_error_t err;
 
+  if ((err = check_writable(slot->client_handle, slot->flags)) != HAL_OK)
+    return err;
+
   if ((err = ks_open_from_flags(&ks, slot->flags)) == HAL_OK &&
       (err = hal_ks_delete_attribute(ks, slot, type)) == HAL_OK)
     err = hal_ks_close(ks);