Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[20166] Fix data race in PDPListener and SecurityManager (backport #4188) #4208

Merged
merged 1 commit into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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
Loading