From e8dfbabd0c8a72a421cefd834462100397348e84 Mon Sep 17 00:00:00 2001 From: Dennis Potman Date: Tue, 7 Apr 2020 21:25:12 +0200 Subject: [PATCH] Add locking when intializing remote key material The initialization of remote participant's key material was not protected by the remote_participant_crypto lock, which could result in using partially initialized remote key material. This caused intermittent test failures with assertions on key_size in crypto_cipher_decrypt_data. This commit fixes this issue by adding locking for the remote key material. Signed-off-by: Dennis Potman --- .../cryptographic/src/crypto_key_exchange.c | 2 +- .../cryptographic/src/crypto_key_factory.c | 56 +++++++++++++++---- .../cryptographic/src/crypto_key_factory.h | 1 + .../cryptographic/src/crypto_objects.c | 55 ++++++++++++------ .../cryptographic/src/crypto_objects.h | 19 ++++++- .../cryptographic/src/crypto_transform.c | 12 ++-- 6 files changed, 108 insertions(+), 37 deletions(-) diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_key_exchange.c b/src/security/builtin_plugins/cryptographic/src/crypto_key_exchange.c index 59d19af..9ea8ade 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_key_exchange.c +++ b/src/security/builtin_plugins/cryptographic/src/crypto_key_exchange.c @@ -179,7 +179,7 @@ create_local_participant_crypto_tokens( } factory = cryptography_get_crypto_key_factory(impl->crypto); - if (!crypto_factory_get_participant_crypto_tokens(factory, local_id, remote_id, &pp_key_material, NULL, ex)) + if (!crypto_factory_get_participant_crypto_tokens(factory, local_id, remote_id, &pp_key_material, NULL, NULL, ex)) goto fail_invalid_arg; serialize_master_key_material(pp_key_material->local_P2P_key_material, &buffer, &length); CRYPTO_OBJECT_RELEASE(pp_key_material); diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.c b/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.c index ab75a63..cef6830 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.c +++ b/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.c @@ -20,6 +20,7 @@ #include "dds/ddsrt/heap.h" #include "dds/ddsrt/sync.h" #include "dds/ddsrt/types.h" +#include "dds/ddsi/q_gc.h" #include "dds/security/dds_security_api.h" #include "dds/security/core/dds_security_utils.h" #include "dds/security/core/dds_security_serialize.h" @@ -841,12 +842,14 @@ unregister_participant( if ((rmt_pp_crypto = (remote_participant_crypto *)crypto_object_table_find(implementation->crypto_objects, handles[i])) != NULL) { - if ((keymat = crypto_remote_participant_remove_keymat(rmt_pp_crypto, participant_crypto_handle)) != NULL) + ddsrt_mutex_lock(&rmt_pp_crypto->lock); + if ((keymat = crypto_remote_participant_remove_keymat_locked(rmt_pp_crypto, participant_crypto_handle)) != NULL) { if (keymat->remote_key_material && keymat->remote_key_material->receiver_specific_key_id != 0) - crypto_remove_specific_key_relation(rmt_pp_crypto, keymat->remote_key_material->receiver_specific_key_id); + crypto_remove_specific_key_relation_locked(rmt_pp_crypto, keymat->remote_key_material->receiver_specific_key_id); CRYPTO_OBJECT_RELEASE(keymat); } + ddsrt_mutex_unlock(&rmt_pp_crypto->lock); CRYPTO_OBJECT_RELEASE(rmt_pp_crypto); } } @@ -862,12 +865,14 @@ unregister_participant( num = crypto_remote_participnant_get_matching(rmt_pp_crypto, &handles); for (i = 0; i < num; i++) { - if ((keymat = crypto_remote_participant_remove_keymat(rmt_pp_crypto, handles[i])) != NULL) + ddsrt_mutex_lock(&rmt_pp_crypto->lock); + if ((keymat = crypto_remote_participant_remove_keymat_locked(rmt_pp_crypto, handles[i])) != NULL) { if (keymat->remote_key_material && keymat->remote_key_material->receiver_specific_key_id != 0) - crypto_remove_specific_key_relation(rmt_pp_crypto, keymat->remote_key_material->receiver_specific_key_id); + crypto_remove_specific_key_relation_locked(rmt_pp_crypto, keymat->remote_key_material->receiver_specific_key_id); CRYPTO_OBJECT_RELEASE(keymat); } + ddsrt_mutex_unlock(&rmt_pp_crypto->lock); if ((loc_pp_crypto = (local_participant_crypto *)crypto_object_table_find(implementation->crypto_objects, handles[i])) != NULL) { @@ -1063,8 +1068,8 @@ crypto_factory_get_participant_crypto_tokens( DDS_Security_ParticipantCryptoHandle local_id, DDS_Security_ParticipantCryptoHandle remote_id, participant_key_material **pp_key_material, + master_key_material **remote_key_matarial, DDS_Security_ProtectionKind *protection_kind, - DDS_Security_SecurityException *ex) { assert (pp_key_material != NULL); @@ -1085,14 +1090,19 @@ crypto_factory_get_participant_crypto_tokens( goto err_remote; } - if (!(*pp_key_material = (participant_key_material *)crypto_remote_participant_lookup_keymat(remote_crypto, local_id))) + ddsrt_mutex_lock(&remote_crypto->lock); + if (!(*pp_key_material = (participant_key_material *)crypto_remote_participant_lookup_keymat_locked(remote_crypto, local_id))) { DDS_Security_Exception_set(ex, DDS_CRYPTO_PLUGIN_CONTEXT, DDS_SECURITY_ERR_INVALID_CRYPTO_HANDLE_CODE, 0, DDS_SECURITY_ERR_INVALID_CRYPTO_HANDLE_MESSAGE); + ddsrt_mutex_unlock(&remote_crypto->lock); goto err_remote; } + if (remote_key_matarial != NULL) + *remote_key_matarial = (*pp_key_material)->remote_key_material; if (protection_kind != NULL) *protection_kind = remote_crypto->rtps_protection_kind; + ddsrt_mutex_unlock(&remote_crypto->lock); result = true; err_remote: @@ -1101,6 +1111,12 @@ err_no_remote: return result; } +static void gc_remote_key_material (struct gcreq *gcreq) +{ + CRYPTO_OBJECT_RELEASE (gcreq->arg); + gcreq_free (gcreq); +} + bool crypto_factory_set_participant_crypto_tokens( const dds_security_crypto_key_factory *factory, @@ -1128,30 +1144,46 @@ crypto_factory_set_participant_crypto_tokens( goto err_inv_remote; } - key_material = crypto_remote_participant_lookup_keymat(remote_crypto, local_id); + ddsrt_mutex_lock(&remote_crypto->lock); + key_material = crypto_remote_participant_lookup_keymat_locked(remote_crypto, local_id); if (key_material) { - if (!key_material->remote_key_material) - key_material->remote_key_material = crypto_master_key_material_new(CRYPTO_TRANSFORMATION_KIND_NONE); - crypto_token_copy(key_material->remote_key_material, remote_key_mat); + /* Because setting crypto tokens is not done very often, we're not using an + atomic pointer (which makes the code less readable) and do not introduce an + additional lock for the remote key material. Instead it is protected by the + remote_participant_crypto lock. For cleaning up the old remote key + material, the garbage collector is used so that any pointer to the remote + key material can be safely used until thread state sleep. */ + master_key_material *remote_key_mat_old = key_material->remote_key_material; + master_key_material *remote_key_mat_new = crypto_master_key_material_new(CRYPTO_TRANSFORMATION_KIND_NONE); + crypto_token_copy(remote_key_mat_new, remote_key_mat); + key_material->remote_key_material = remote_key_mat_new; + if (remote_key_mat_old != NULL) + { + struct gcreq *gcreq = gcreq_new(impl->crypto->gv->gcreq_queue, gc_remote_key_material); + gcreq->arg = remote_key_mat_old; + gcreq_enqueue(gcreq); + } uint32_t specific_key = key_material->remote_key_material->receiver_specific_key_id; if (specific_key != 0) { - key_relation *relation = crypto_find_specific_key_relation(remote_crypto, specific_key); + key_relation *relation = crypto_find_specific_key_relation_locked(remote_crypto, specific_key); if (!relation) { local_participant_crypto *local_crypto = (local_participant_crypto *)crypto_object_table_find(impl->crypto_objects, local_id); relation = crypto_key_relation_new(0, specific_key, CRYPTO_OBJECT(local_crypto), CRYPTO_OBJECT(remote_crypto), key_material->remote_key_material); - crypto_insert_specific_key_relation(remote_crypto, relation); + crypto_insert_specific_key_relation_locked(remote_crypto, relation); CRYPTO_OBJECT_RELEASE(local_crypto); } CRYPTO_OBJECT_RELEASE(relation); } + ddsrt_mutex_unlock(&remote_crypto->lock); CRYPTO_OBJECT_RELEASE(key_material); } else { + ddsrt_mutex_unlock(&remote_crypto->lock); DDS_Security_Exception_set(ex, DDS_CRYPTO_PLUGIN_CONTEXT, DDS_SECURITY_ERR_INVALID_CRYPTO_HANDLE_CODE, 0, DDS_SECURITY_ERR_INVALID_CRYPTO_HANDLE_MESSAGE); goto err_inv_remote; diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.h b/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.h index a908e2c..66538f4 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.h +++ b/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.h @@ -40,6 +40,7 @@ bool crypto_factory_get_participant_crypto_tokens( DDS_Security_ParticipantCryptoHandle local_id, DDS_Security_ParticipantCryptoHandle remote_id, participant_key_material **pp_key_material, + master_key_material **remote_key_matarial, DDS_Security_ProtectionKind *protection_kind, DDS_Security_SecurityException *ex); diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_objects.c b/src/security/builtin_plugins/cryptographic/src/crypto_objects.c index 17929af..9a9cf05 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_objects.c +++ b/src/security/builtin_plugins/cryptographic/src/crypto_objects.c @@ -472,28 +472,29 @@ void crypto_remote_participant_add_keymat(remote_participant_crypto *rmt_pp_cryp ddsrt_mutex_unlock(&rmt_pp_crypto->lock); } -participant_key_material * crypto_remote_participant_remove_keymat(remote_participant_crypto *rmt_pp_crypto, DDS_Security_ParticipantCryptoHandle loc_pp_handle) +participant_key_material * crypto_remote_participant_remove_keymat_locked(remote_participant_crypto *rmt_pp_crypto, DDS_Security_ParticipantCryptoHandle loc_pp_handle) { participant_key_material *keymat; ddsrt_avl_dpath_t dpath; - ddsrt_mutex_lock(&rmt_pp_crypto->lock); keymat = ddsrt_avl_clookup_dpath(&rmt_pp_keymat_treedef, &rmt_pp_crypto->key_material_table, &loc_pp_handle, &dpath); if (keymat) ddsrt_avl_cdelete_dpath(&rmt_pp_keymat_treedef, &rmt_pp_crypto->key_material_table, keymat, &dpath); - ddsrt_mutex_unlock(&rmt_pp_crypto->lock); return keymat; } +participant_key_material * crypto_remote_participant_lookup_keymat_locked(remote_participant_crypto *rmt_pp_crypto, DDS_Security_ParticipantCryptoHandle loc_pp_handle) +{ + return CRYPTO_OBJECT_KEEP(ddsrt_avl_clookup(&rmt_pp_keymat_treedef, &rmt_pp_crypto->key_material_table, &loc_pp_handle)); +} + participant_key_material * crypto_remote_participant_lookup_keymat(remote_participant_crypto *rmt_pp_crypto, DDS_Security_ParticipantCryptoHandle loc_pp_handle) { participant_key_material *keymat; - ddsrt_mutex_lock(&rmt_pp_crypto->lock); - keymat = CRYPTO_OBJECT_KEEP(ddsrt_avl_clookup(&rmt_pp_keymat_treedef, &rmt_pp_crypto->key_material_table, &loc_pp_handle)); + keymat = crypto_remote_participant_lookup_keymat_locked(rmt_pp_crypto, loc_pp_handle); ddsrt_mutex_unlock(&rmt_pp_crypto->lock); - return keymat; } @@ -727,32 +728,52 @@ key_relation * crypto_find_endpoint_relation( return relation; } +void crypto_insert_specific_key_relation_locked( + remote_participant_crypto *rpc, + key_relation *relation) +{ + ddsrt_avl_insert(&specific_key_treedef, &rpc->specific_key_index, CRYPTO_OBJECT_KEEP(relation)); +} + void crypto_insert_specific_key_relation( remote_participant_crypto *rpc, key_relation *relation) { ddsrt_mutex_lock(&rpc->lock); - ddsrt_avl_insert(&specific_key_treedef, &rpc->specific_key_index, CRYPTO_OBJECT_KEEP(relation)); + crypto_insert_specific_key_relation_locked(rpc, relation); ddsrt_mutex_unlock(&rpc->lock); } +void crypto_remove_specific_key_relation_locked( + remote_participant_crypto *rpc, + uint32_t key_id) +{ + key_relation *relation; + ddsrt_avl_dpath_t dpath; + relation = ddsrt_avl_lookup_dpath(&specific_key_treedef, &rpc->specific_key_index, &key_id, &dpath); + if (relation) + { + ddsrt_avl_delete_dpath(&specific_key_treedef, &rpc->specific_key_index, relation, &dpath); + CRYPTO_OBJECT_RELEASE(relation); + } +} + void crypto_remove_specific_key_relation( remote_participant_crypto *rpc, uint32_t key_id) { - key_relation *relation; - ddsrt_avl_dpath_t dpath; - ddsrt_mutex_lock(&rpc->lock); - relation = ddsrt_avl_lookup_dpath(&specific_key_treedef, &rpc->specific_key_index, &key_id, &dpath); - if (relation) - { - ddsrt_avl_delete_dpath(&specific_key_treedef, &rpc->specific_key_index, relation, &dpath); - CRYPTO_OBJECT_RELEASE(relation); - } + crypto_remove_specific_key_relation_locked(rpc, key_id); ddsrt_mutex_unlock(&rpc->lock); } +key_relation * crypto_find_specific_key_relation_locked( + remote_participant_crypto *rpc, + uint32_t key_id) +{ + return CRYPTO_OBJECT_KEEP(ddsrt_avl_lookup(&specific_key_treedef, &rpc->specific_key_index, &key_id)); +} + key_relation * crypto_find_specific_key_relation( remote_participant_crypto *rpc, uint32_t key_id) @@ -760,7 +781,7 @@ key_relation * crypto_find_specific_key_relation( key_relation *relation; ddsrt_mutex_lock(&rpc->lock); - relation = CRYPTO_OBJECT_KEEP(ddsrt_avl_lookup(&specific_key_treedef, &rpc->specific_key_index, &key_id)); + relation = crypto_find_specific_key_relation_locked(rpc, key_id); ddsrt_mutex_unlock(&rpc->lock); return relation; diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_objects.h b/src/security/builtin_plugins/cryptographic/src/crypto_objects.h index 0ef700b..ebb34ed 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_objects.h +++ b/src/security/builtin_plugins/cryptographic/src/crypto_objects.h @@ -261,16 +261,28 @@ crypto_find_endpoint_relation( CryptoObject *lch, uint32_t key_id); +void crypto_insert_specific_key_relation_locked( + remote_participant_crypto *rpc, + key_relation *relation); + void crypto_insert_specific_key_relation( remote_participant_crypto *rpc, key_relation *relation); +void crypto_remove_specific_key_relation_locked( + remote_participant_crypto *rpc, + uint32_t key_id); + void crypto_remove_specific_key_relation( remote_participant_crypto *rpc, uint32_t key_id); +key_relation * crypto_find_specific_key_relation_locked( + remote_participant_crypto *rpc, + uint32_t key_id); + key_relation * crypto_find_specific_key_relation( remote_participant_crypto *rpc, @@ -403,10 +415,15 @@ crypto_remote_participant_add_keymat( participant_key_material *keymat); participant_key_material * -crypto_remote_participant_remove_keymat( +crypto_remote_participant_remove_keymat_locked( remote_participant_crypto *rmt_pp_crypte, DDS_Security_ParticipantCryptoHandle loc_pp_handle); +participant_key_material * +crypto_remote_participant_lookup_keymat_locked( + remote_participant_crypto *rmt_pp_crypto, + DDS_Security_ParticipantCryptoHandle loc_pp_handle); + participant_key_material * crypto_remote_participant_lookup_keymat( remote_participant_crypto *rmt_pp_crypte, diff --git a/src/security/builtin_plugins/cryptographic/src/crypto_transform.c b/src/security/builtin_plugins/cryptographic/src/crypto_transform.c index 21c3e1c..a2f6893 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_transform.c +++ b/src/security/builtin_plugins/cryptographic/src/crypto_transform.c @@ -996,7 +996,7 @@ add_receiver_specific_mac( return false; /* get remote crypto tokens */ - if (!crypto_factory_get_participant_crypto_tokens(factory, sending_participant_crypto, receiving_participant_crypto, &pp_key_material, &remote_protection_kind, ex)) + if (!crypto_factory_get_participant_crypto_tokens(factory, sending_participant_crypto, receiving_participant_crypto, &pp_key_material, NULL, &remote_protection_kind, ex)) { CRYPTO_OBJECT_RELEASE(session); return false; @@ -1841,6 +1841,7 @@ decode_rtps_message(dds_security_crypto_transform *instance, uint32_t decoded_body_size; static const char *context = "decode_rtps_message"; participant_key_material *pp_key_material; + master_key_material *remote_key_material; DDS_Security_ProtectionKind remote_protection_kind; bool result = false; @@ -1872,10 +1873,9 @@ decode_rtps_message(dds_security_crypto_transform *instance, transform_kind = CRYPTO_TRANSFORM_KIND(header.transform_identifier.transformation_kind); /* Retrieve key material from sending_participant_crypto and receiving_participant_crypto from factory */ - if (!crypto_factory_get_participant_crypto_tokens(factory, receiving_participant_crypto, sending_participant_crypto, &pp_key_material, &remote_protection_kind, ex)) + if (!crypto_factory_get_participant_crypto_tokens(factory, receiving_participant_crypto, sending_participant_crypto, &pp_key_material, &remote_key_material, &remote_protection_kind, ex)) goto fail_tokens; - - if (!pp_key_material->remote_key_material) + if (remote_key_material == NULL) goto fail_remote_keys_not_ready; if (has_origin_authentication(remote_protection_kind)) @@ -1886,8 +1886,8 @@ decode_rtps_message(dds_security_crypto_transform *instance, /* calculate the session key */ decoded_body = DDS_Security_OctetSeq_allocbuf(contents._length); - if (!initialize_remote_session_info(&remote_session, &header, pp_key_material->remote_key_material->master_salt, - pp_key_material->remote_key_material->master_sender_key, pp_key_material->remote_key_material->transformation_kind, ex)) + if (!initialize_remote_session_info(&remote_session, &header, remote_key_material->master_salt, + remote_key_material->master_sender_key, remote_key_material->transformation_kind, ex)) { DDS_Security_Exception_set(ex, DDS_CRYPTO_PLUGIN_CONTEXT, DDS_SECURITY_ERR_INVALID_CRYPTO_ARGUMENT_CODE, 0, "decode_rtps_message: " DDS_SECURITY_ERR_INVALID_CRYPTO_ARGUMENT_MESSAGE);