Skip to content

Commit

Permalink
Fix data race in PDPListener and SecurityManager (#4188) (#4207)
Browse files Browse the repository at this point in the history 
* Refs #20166: Regression Test

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

* Refs #20166: Use the participant proxy data stored the collection in SecurityManager::discovered_participant()

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

* Refs #20166: Rev suggestion

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

* Refs #20166: Test Rev suggestion

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

---------

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
(cherry picked from commit 84334da)

Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>
  • Loading branch information
@mergify @Mario-DL
mergify[bot] and Mario-DL authored Jan 16, 2024
1 parent 438a03b commit 340077d
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 14 deletions.
5 changes: 3 additions & 2 deletions src/cpp/rtps/builtin/discovery/participant/DS/PDPSecurityInitiatorListener.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,16 @@ void PDPSecurityInitiatorListener::process_alive_data(

if (old_data == nullptr)
{
auto callback_data = new_data;
reader->getMutex().unlock();
lock.unlock();

//! notify security manager in order to start handshake
bool ret = parent_pdp_->getRTPSParticipant()->security_manager().discovered_participant(new_data);
bool ret = parent_pdp_->getRTPSParticipant()->security_manager().discovered_participant(callback_data);
//! Reply to the remote participant
if (ret)
{
response_cb_(temp_participant_data_);
response_cb_(callback_data);
}

// Take again the reader lock
Expand Down
27 changes: 15 additions & 12 deletions src/cpp/rtps/security/SecurityManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,8 @@ bool SecurityManager::discovered_participant(
// Create or find information
bool undiscovered = false;
DiscoveredParticipantInfo::AuthUniquePtr remote_participant_info;
// Use the information from the collection
const ParticipantProxyData* remote_participant_data = nullptr;
{
std::lock_guard<shared_mutex> _(mutex_);

Expand All @@ -614,13 +616,14 @@ bool SecurityManager::discovered_participant(

undiscovered = map_ret.second;
remote_participant_info = map_ret.first->second->get_auth();
remote_participant_data = &map_ret.first->second->participant_data();
}

bool notify_part_authorized = false;
if (undiscovered && remote_participant_info)
if (undiscovered && remote_participant_info && remote_participant_data != nullptr)
{
// Configure the timed event but do not start it
const GUID_t guid = participant_data.m_guid;
const GUID_t guid = remote_participant_data->m_guid;
remote_participant_info->event_.reset(new TimedEvent(participant_->getEventResource(),
[&, guid]() -> bool
{
Expand All @@ -634,8 +637,8 @@ bool SecurityManager::discovered_participant(
// Validate remote participant.
ValidationResult_t validation_ret = authentication_plugin_->validate_remote_identity(&remote_identity_handle,
*local_identity_handle_,
participant_data.identity_token_,
participant_data.m_guid, exception);
remote_participant_data->identity_token_,
remote_participant_data->m_guid, exception);

switch (validation_ret)
{
Expand All @@ -655,21 +658,21 @@ bool SecurityManager::discovered_participant(
// TODO(Ricardo) Send event.
default:

on_validation_failed(participant_data, exception);
on_validation_failed(*remote_participant_data, exception);

std::lock_guard<shared_mutex> _(mutex_);

// Remove created element, because authentication failed.
discovered_participants_.erase(participant_data.m_guid);
discovered_participants_.erase(remote_participant_data->m_guid);

//TODO(Ricardo) cryptograhy registration in AUTHENTICAITON_OK
return false;
}

EPROSIMA_LOG_INFO(SECURITY, "Discovered participant " << participant_data.m_guid);
EPROSIMA_LOG_INFO(SECURITY, "Discovered participant " << remote_participant_data->m_guid);

// Match entities
match_builtin_endpoints(participant_data);
match_builtin_endpoints(*remote_participant_data);

// Store new remote handle.
remote_participant_info->auth_status_ = auth_status;
Expand All @@ -681,7 +684,7 @@ bool SecurityManager::discovered_participant(
{
//TODO(Ricardo) Shared secret on this case?
std::shared_ptr<SecretHandle> ss;
notify_part_authorized = participant_authorized(participant_data, remote_participant_info, ss);
notify_part_authorized = participant_authorized(*remote_participant_data, remote_participant_info, ss);
}
}
else
Expand All @@ -704,15 +707,15 @@ bool SecurityManager::discovered_participant(
if (remote_participant_info->auth_status_ == AUTHENTICATION_REQUEST_NOT_SEND)
{
// Maybe send request.
returnedValue = on_process_handshake(participant_data, remote_participant_info,
returnedValue = on_process_handshake(*remote_participant_data, remote_participant_info,
MessageIdentity(), HandshakeMessageToken(), notify_part_authorized);
}

restore_discovered_participant_info(participant_data.m_guid, remote_participant_info);
restore_discovered_participant_info(remote_participant_data->m_guid, remote_participant_info);

if (notify_part_authorized)
{
notify_participant_authorized(participant_data);
notify_participant_authorized(*remote_participant_data);
}

return returnedValue;
Expand Down
50 changes: 50 additions & 0 deletions test/blackbox/common/BlackboxTestsSecurity.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3848,6 +3848,56 @@ TEST(Security, AllowUnauthenticatedParticipants_TwoParticipantsDifferentCertific
ASSERT_FALSE(writer.is_matched());
}

// Regresion test for redmine issue 20166
TEST(Security, InANonSecureParticipantWithTwoSecureParticipantScenario_TheTwoSecureParticipantsCorrectlyCommunicate)
{
// Create
PubSubReader<HelloWorldPubSubType> non_secure_reader("HelloWorldTopic");
PubSubReader<HelloWorldPubSubType> secure_reader("HelloWorldTopic");
PubSubWriter<HelloWorldPubSubType> secure_writer("HelloWorldTopic");

// Configure security
const std::string governance_file("governance_helloworld_all_enable.smime");
const std::string permissions_file("permissions_helloworld.smime");
CommonPermissionsConfigure(secure_reader, secure_writer, governance_file, permissions_file);

secure_writer.history_depth(10).
reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS).init();

ASSERT_TRUE(secure_writer.isInitialized());

non_secure_reader.history_depth(10).
reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS).init();

ASSERT_TRUE(non_secure_reader.isInitialized());

secure_reader.history_depth(10).
reliability(eprosima::fastrtps::RELIABLE_RELIABILITY_QOS).init();

ASSERT_TRUE(secure_reader.isInitialized());

// Wait for the authorization
secure_reader.waitAuthorized();
secure_writer.waitAuthorized();

// Wait for discovery
secure_writer.wait_discovery(std::chrono::seconds(5));
secure_reader.wait_discovery(std::chrono::seconds(5));

// Data is correctly sent and received
auto data = default_helloworld_data_generator();

secure_reader.startReception(data);

secure_writer.send(data);

// In this test all data should be sent.
ASSERT_TRUE(data.empty());

secure_reader.block_for_all();
EXPECT_EQ(non_secure_reader.getReceivedCount(), 0u);
}

// *INDENT-OFF*
TEST_P(Security, BuiltinAuthenticationAndAccessAndCryptoPlugin_PermissionsDisableDiscoveryDisableAccessEncrypt_validation_ok_enable_discovery_enable_access_encrypt)
// *INDENT-ON*
Expand Down

0 comments on commit 340077d

Please sign in to comment.