From e1899df62d7c09d452543eada4bdf3d7f79ee4fc Mon Sep 17 00:00:00 2001 From: Marcel Jordense Date: Fri, 8 May 2020 17:09:01 +0200 Subject: [PATCH] Fix incorrect type of include_optional_fields config parameter Signed-off-by: Marcel Jordense --- src/core/ddsi/include/dds/ddsi/q_config.h | 2 +- src/core/ddsi/src/ddsi_security_omg.c | 2 +- .../authentication/src/authentication.c | 62 ++++++++++--------- .../cryptographic/src/crypto_key_exchange.c | 17 ++--- .../cryptographic/src/crypto_key_factory.c | 4 +- 5 files changed, 46 insertions(+), 41 deletions(-) diff --git a/src/core/ddsi/include/dds/ddsi/q_config.h b/src/core/ddsi/include/dds/ddsi/q_config.h index d9628e2..3c78e28 100644 --- a/src/core/ddsi/include/dds/ddsi/q_config.h +++ b/src/core/ddsi/include/dds/ddsi/q_config.h @@ -174,7 +174,7 @@ typedef struct authentication_properties_type{ char *private_key; char *password; char *trusted_ca_dir; - bool include_optional_fields; + int include_optional_fields; } authentication_properties_type; typedef struct access_control_properties_type{ diff --git a/src/core/ddsi/src/ddsi_security_omg.c b/src/core/ddsi/src/ddsi_security_omg.c index 36d5a3c..7824239 100644 --- a/src/core/ddsi/src/ddsi_security_omg.c +++ b/src/core/ddsi/src/ddsi_security_omg.c @@ -736,7 +736,7 @@ void q_omg_security_init (struct ddsi_domaingv *gv) gv->security_context = sc; if (gv->config.omg_security_configuration) - gv->handshake_include_optional = gv->config.omg_security_configuration->cfg.authentication_properties.include_optional_fields; + gv->handshake_include_optional = gv->config.omg_security_configuration->cfg.authentication_properties.include_optional_fields != 0; else gv->handshake_include_optional = false; diff --git a/src/security/builtin_plugins/authentication/src/authentication.c b/src/security/builtin_plugins/authentication/src/authentication.c index 490654b..316c910 100644 --- a/src/security/builtin_plugins/authentication/src/authentication.c +++ b/src/security/builtin_plugins/authentication/src/authentication.c @@ -1126,7 +1126,7 @@ DDS_Security_ValidationResult_t begin_handshake_request(dds_security_authenticat EVP_PKEY *dhkey; unsigned char *certData, *dhPubKeyData = NULL; uint32_t certDataSize, dhPubKeyDataSize; - uint32_t tsz = impl->include_optional ? 8 : 7; + uint32_t tokcount = impl->include_optional ? 8 : 7; int created = 0; if (!instance || !handshake_handle || !handshake_message || !serialized_local_participant_data) @@ -1184,39 +1184,41 @@ DDS_Security_ValidationResult_t begin_handshake_request(dds_security_authenticat if (localIdent->pdata._length == 0) DDS_Security_OctetSeq_copy(&localIdent->pdata, serialized_local_participant_data); - DDS_Security_BinaryProperty_t *tokens = DDS_Security_BinaryPropertySeq_allocbuf(tsz); - uint32_t idx = 0; + DDS_Security_BinaryProperty_t *tokens = DDS_Security_BinaryPropertySeq_allocbuf(tokcount); + uint32_t tokidx = 0; - DDS_Security_BinaryProperty_set_by_ref(&tokens[idx++], "c.id", certData, certDataSize); - DDS_Security_BinaryProperty_set_by_string(&tokens[idx++], "c.perm", localIdent->permissionsDocument ? localIdent->permissionsDocument : ""); - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "c.pdata", serialized_local_participant_data->_buffer, serialized_local_participant_data->_length); - DDS_Security_BinaryProperty_set_by_string(&tokens[idx++], "c.dsign_algo", get_dsign_algo(localIdent->dsignAlgoKind)); - DDS_Security_BinaryProperty_set_by_string(&tokens[idx++], "c.kagree_algo", get_kagree_algo(localIdent->kagreeAlgoKind)); + DDS_Security_BinaryProperty_set_by_ref(&tokens[tokidx++], "c.id", certData, certDataSize); + DDS_Security_BinaryProperty_set_by_string(&tokens[tokidx++], "c.perm", localIdent->permissionsDocument ? localIdent->permissionsDocument : ""); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "c.pdata", serialized_local_participant_data->_buffer, serialized_local_participant_data->_length); + DDS_Security_BinaryProperty_set_by_string(&tokens[tokidx++], "c.dsign_algo", get_dsign_algo(localIdent->dsignAlgoKind)); + DDS_Security_BinaryProperty_set_by_string(&tokens[tokidx++], "c.kagree_algo", get_kagree_algo(localIdent->kagreeAlgoKind)); /* Todo: including hash_c1 is optional (conform spec); add a configuration option to leave it out */ { DDS_Security_BinaryPropertySeq bseq = { ._length = 5, ._buffer = tokens }; get_hash_binary_property_seq(&bseq, handshake->hash_c1); if (impl->include_optional) - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "hash_c1", handshake->hash_c1, sizeof(HashValue_t)); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "hash_c1", handshake->hash_c1, sizeof(HashValue_t)); } /* Set the DH public key associated with the local participant in dh1 property */ assert(dhPubKeyData); assert(dhPubKeyDataSize < 1200); - DDS_Security_BinaryProperty_set_by_ref(&tokens[idx++], "dh1", dhPubKeyData, dhPubKeyDataSize); + DDS_Security_BinaryProperty_set_by_ref(&tokens[tokidx++], "dh1", dhPubKeyData, dhPubKeyDataSize); /* Set the challenge in challenge1 property */ - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "challenge1", relation->lchallenge->value, sizeof(AuthenticationChallenge)); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "challenge1", relation->lchallenge->value, sizeof(AuthenticationChallenge)); (void)ddsrt_hh_add(impl->objectHash, handshake); ddsrt_mutex_unlock(&impl->lock); + assert(tokcount == tokidx); + handshake_message->class_id = ddsrt_strdup(AUTH_HANDSHAKE_REQUEST_TOKEN_ID); handshake_message->properties._length = 0; handshake_message->properties._buffer = NULL; - handshake_message->binary_properties._length = tsz; + handshake_message->binary_properties._length = tokidx; handshake_message->binary_properties._buffer = tokens; *handshake_handle = HANDSHAKE_HANDLE(handshake); @@ -1627,7 +1629,7 @@ DDS_Security_ValidationResult_t begin_handshake_reply(dds_security_authenticatio EVP_PKEY *dhkeyLocal = NULL; unsigned char *certData, *dhPubKeyData; uint32_t certDataSize, dhPubKeyDataSize; - uint32_t tsz = impl->include_optional ? 12 : 9; + uint32_t tokcount = impl->include_optional ? 12 : 9; int created = 0; if (!instance || !handshake_handle || !handshake_message_out || !handshake_message_in || !serialized_local_participant_data) @@ -1694,23 +1696,23 @@ DDS_Security_ValidationResult_t begin_handshake_reply(dds_security_authenticatio if (localIdent->pdata._length == 0) DDS_Security_OctetSeq_copy(&localIdent->pdata, serialized_local_participant_data); - DDS_Security_BinaryProperty_t *tokens = DDS_Security_BinaryPropertySeq_allocbuf(tsz); - uint32_t idx = 0; + DDS_Security_BinaryProperty_t *tokens = DDS_Security_BinaryPropertySeq_allocbuf(tokcount); + uint32_t tokidx = 0; /* Store the Identity Certificate associated with the local identify in c.id property */ - DDS_Security_BinaryProperty_set_by_ref(&tokens[idx++], "c.id", certData, certDataSize); + DDS_Security_BinaryProperty_set_by_ref(&tokens[tokidx++], "c.id", certData, certDataSize); certData = NULL; - DDS_Security_BinaryProperty_set_by_string(&tokens[idx++], "c.perm", localIdent->permissionsDocument ? localIdent->permissionsDocument : ""); - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "c.pdata", serialized_local_participant_data->_buffer, serialized_local_participant_data->_length); - DDS_Security_BinaryProperty_set_by_string(&tokens[idx++], "c.dsign_algo", get_dsign_algo(localIdent->dsignAlgoKind)); - DDS_Security_BinaryProperty_set_by_string(&tokens[idx++], "c.kagree_algo", get_kagree_algo(remoteIdent->kagreeAlgoKind)); + DDS_Security_BinaryProperty_set_by_string(&tokens[tokidx++], "c.perm", localIdent->permissionsDocument ? localIdent->permissionsDocument : ""); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "c.pdata", serialized_local_participant_data->_buffer, serialized_local_participant_data->_length); + DDS_Security_BinaryProperty_set_by_string(&tokens[tokidx++], "c.dsign_algo", get_dsign_algo(localIdent->dsignAlgoKind)); + DDS_Security_BinaryProperty_set_by_string(&tokens[tokidx++], "c.kagree_algo", get_kagree_algo(remoteIdent->kagreeAlgoKind)); /* Calculate the hash_c2 */ DDS_Security_BinaryPropertySeq bseq = { ._length = 5, ._buffer = tokens }; get_hash_binary_property_seq(&bseq, handshake->hash_c2); /* Set the DH public key associated with the local participant in dh2 property */ - DDS_Security_BinaryProperty_t *dh2 = &tokens[idx++]; + DDS_Security_BinaryProperty_t *dh2 = &tokens[tokidx++]; DDS_Security_BinaryProperty_set_by_ref(dh2, "dh2", dhPubKeyData, dhPubKeyDataSize); /* Find the dh1 property from the received request token */ @@ -1718,18 +1720,18 @@ DDS_Security_ValidationResult_t begin_handshake_reply(dds_security_authenticatio assert(dh1); assert(relation->rchallenge); - DDS_Security_BinaryProperty_t *challenge1 = &tokens[idx++]; + DDS_Security_BinaryProperty_t *challenge1 = &tokens[tokidx++]; DDS_Security_BinaryProperty_set_by_value(challenge1, "challenge1", relation->rchallenge->value, sizeof(AuthenticationChallenge)); assert(relation->lchallenge); - DDS_Security_BinaryProperty_t *challenge2 = &tokens[idx++]; + DDS_Security_BinaryProperty_t *challenge2 = &tokens[tokidx++]; DDS_Security_BinaryProperty_set_by_value(challenge2, "challenge2", relation->lchallenge->value, sizeof(AuthenticationChallenge)); /* THe dh1 and hash_c1 and hash_c2 are optional */ if (impl->include_optional) { - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "dh1", dh1->value._buffer, dh1->value._length); - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "hash_c2", handshake->hash_c2, sizeof(HashValue_t)); - DDS_Security_BinaryProperty_set_by_value(&tokens[idx++], "hash_c1", handshake->hash_c1, sizeof(HashValue_t)); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "dh1", dh1->value._buffer, dh1->value._length); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "hash_c2", handshake->hash_c2, sizeof(HashValue_t)); + DDS_Security_BinaryProperty_set_by_value(&tokens[tokidx++], "hash_c1", handshake->hash_c1, sizeof(HashValue_t)); } /* Calculate the signature */ @@ -1744,12 +1746,14 @@ DDS_Security_ValidationResult_t begin_handshake_reply(dds_security_authenticatio DDS_Security_BinaryProperty_free(hash_c2_val); if (result != DDS_SECURITY_VALIDATION_OK) goto err_signature; - DDS_Security_BinaryProperty_set_by_ref(&tokens[idx++], "signature", sign, (uint32_t)signlen); + DDS_Security_BinaryProperty_set_by_ref(&tokens[tokidx++], "signature", sign, (uint32_t)signlen); } + assert(tokidx == tokcount); + (void)ddsrt_hh_add(impl->objectHash, handshake); handshake_message_out->class_id = ddsrt_strdup(AUTH_HANDSHAKE_REPLY_TOKEN_ID); - handshake_message_out->binary_properties._length = tsz; + handshake_message_out->binary_properties._length = tokidx; handshake_message_out->binary_properties._buffer = tokens; ddsrt_mutex_unlock(&impl->lock); @@ -1758,7 +1762,7 @@ DDS_Security_ValidationResult_t begin_handshake_reply(dds_security_authenticatio return DDS_SECURITY_VALIDATION_PENDING_HANDSHAKE_MESSAGE; err_signature: - free_binary_properties(tokens, tsz); + free_binary_properties(tokens, tokcount); err_get_public_key: err_gen_dh_keys: ddsrt_free(certData); 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 9ea8ade..7cadb64 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_key_exchange.c +++ b/src/security/builtin_plugins/cryptographic/src/crypto_key_exchange.c @@ -78,18 +78,19 @@ static bool check_crypto_keymaterial( { bool status = false; uint32_t transform_kind = CRYPTO_TRANSFORM_KIND(keymat->transformation_kind); - uint32_t key_sz = CRYPTO_KEY_SIZE_BYTES(transform_kind); if (transform_kind != CRYPTO_TRANSFORMATION_KIND_NONE) { - status = (transform_kind <= CRYPTO_TRANSFORMATION_KIND_AES256_GCM && - keymat->master_salt._length == key_sz && keymat->master_salt._buffer != NULL && check_not_data_empty(&keymat->master_salt) && - keymat->master_sender_key._length == key_sz && keymat->master_sender_key._buffer != NULL && check_not_data_empty(&keymat->master_sender_key)); - - if (status && CRYPTO_TRANSFORM_ID(keymat->receiver_specific_key_id)) + if (transform_kind <= CRYPTO_TRANSFORMATION_KIND_AES256_GCM) { - status = (keymat->master_receiver_specific_key._length == key_sz && - keymat->master_receiver_specific_key._buffer != NULL && check_not_data_empty(&keymat->master_receiver_specific_key)); + uint32_t key_sz = CRYPTO_KEY_SIZE_BYTES(transform_kind); + status = (keymat->master_salt._length == key_sz && keymat->master_salt._buffer != NULL && check_not_data_empty(&keymat->master_salt) && + keymat->master_sender_key._length == key_sz && keymat->master_sender_key._buffer != NULL && check_not_data_empty(&keymat->master_sender_key)); + if (status && CRYPTO_TRANSFORM_ID(keymat->receiver_specific_key_id)) + { + status = (keymat->master_receiver_specific_key._length == key_sz && + keymat->master_receiver_specific_key._buffer != NULL && check_not_data_empty(&keymat->master_receiver_specific_key)); + } } } else 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 5416abf..1ce7e49 100644 --- a/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.c +++ b/src/security/builtin_plugins/cryptographic/src/crypto_key_factory.c @@ -37,7 +37,9 @@ */ #define KXKEYCOOKIE "key exchange key" +#define KXKEYCOOKIE_SIZE (sizeof(KXKEYCOOKIE) - 1) #define KXSALTCOOKIE "keyexchange salt" +#define KXSALTCOOKIE_SIZE (sizeof(KXSALTCOOKIE) - 1) typedef struct dds_security_crypto_key_factory_impl { @@ -89,8 +91,6 @@ calculate_kx_keys( unsigned char *kx_master_salt, *kx_master_sender_key; size_t shared_secret_size = get_secret_size_from_secret_handle(shared_secret); unsigned char hash[SHA256_DIGEST_LENGTH]; - size_t KXKEYCOOKIE_SIZE = strlen(KXKEYCOOKIE); - size_t KXSALTCOOKIE_SIZE = strlen(KXSALTCOOKIE); size_t concatenated_bytes1_size = DDS_SECURITY_AUTHENTICATION_CHALLENGE_SIZE * 2 + KXSALTCOOKIE_SIZE; size_t concatenated_bytes2_size = DDS_SECURITY_AUTHENTICATION_CHALLENGE_SIZE * 2 + KXKEYCOOKIE_SIZE; DDS_Security_octet *concatenated_bytes1, *concatenated_bytes2;