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 <dennis.potman@adlinktech.com>

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);

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)
+    /* Because setting crypto tokens is not done very often, we're not using an
-      key_material->remote_key_material = crypto_master_key_material_new(CRYPTO_TRANSFORMATION_KIND_NONE);
+       atomic pointer (which makes the code less readable) and do not introduce an
-    crypto_token_copy(key_material->remote_key_material, remote_key_mat);
+       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;

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);
 

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);
+  crypto_remove_specific_key_relation_locked(rpc, key_id);
-  if (relation)
-  {
-    ddsrt_avl_delete_dpath(&specific_key_treedef, &rpc->specific_key_index, relation, &dpath);
-    CRYPTO_OBJECT_RELEASE(relation);
-  }
   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;

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,

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 (remote_key_material == NULL)
-  if (!pp_key_material->remote_key_material)
     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,
+  if (!initialize_remote_session_info(&remote_session, &header, remote_key_material->master_salt,
-        pp_key_material->remote_key_material->master_sender_key, pp_key_material->remote_key_material->transformation_kind, ex))
+        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);