From 41bc63d2ee629610de41c793e1eb00e1571d38d4 Mon Sep 17 00:00:00 2001 From: Rob Austein Date: Mon, 24 Oct 2016 17:57:35 -0400 Subject: 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. --- hal.h | 1 + ks_volatile.c | 21 ++++++++++++--- libhal.py | 17 ++++++++++-- rpc_pkey.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++------ 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); -- cgit v1.2.3