From ae9ca3722fdd7fabdf8ab2d889592f32482b5f87 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Tue, 24 Sep 2024 16:52:46 +0200 Subject: [PATCH 1/9] Refs #21707. Regression test. Signed-off-by: Miguel Company --- .../common/BlackboxTestsTransportUDP.cpp | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index 18907f4bb35..f5dc2b4f259 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -635,6 +636,135 @@ TEST(TransportUDP, MaliciousManipulatedDataOctetsToNextHeaderIgnore) reader.block_for_all(); } +/** + * This is a regression test for redmine issue #21707. + */ +TEST(TransportUDP, KeyOnlyBigPayloadIgnored) +{ + // Force using UDP transport + auto udp_transport = std::make_shared(); + + PubSubWriter writer(TEST_TOPIC_NAME); + PubSubReader reader(TEST_TOPIC_NAME); + + struct KeyOnlyBigPayloadDatagram + { + std::array rtps_id{ {'R', 'T', 'P', 'S'} }; + std::array protocol_version{ {2, 3} }; + std::array vendor_id{ {0x01, 0x0F} }; + GuidPrefix_t sender_prefix{}; + + struct DataSubMsg + { + struct Header + { + uint8_t submessage_id = 0x15; +#if FASTDDS_IS_BIG_ENDIAN_TARGET + uint8_t flags = 0x0A; // Serialized key, inline QoS +#else + uint8_t flags = 0x0B; // Serialized key, inline QoS, endianness +#endif // FASTDDS_IS_BIG_ENDIAN_TARGET + uint16_t octets_to_next_header = 0x48; + uint16_t extra_flags = 0; + uint16_t octets_to_inline_qos = 0x10; + EntityId_t reader_id{}; + EntityId_t writer_id{}; + SequenceNumber_t sn{ 2 }; + }; + + struct InlineQoS + { + // PID_STATUS_INFO (unregistered + disposed) + struct StatusInfo + { + uint16_t pid = 0x0071; + uint16_t length = 0x0004; + std::array status_value{ {0x00, 0x00, 0x00, 0x03} }; + }; + // PID_SENTINEL + struct Sentinel + { + uint16_t pid = 0x0001; + uint16_t length = 0x0000; + }; + + StatusInfo status_info; + Sentinel sentinel; + }; + + struct SerializedData + { + std::array encapsulation {{0}}; + std::array encapsulation_opts {{0}}; + std::array data {{0}}; + }; + + Header header; + InlineQoS qos; + SerializedData payload; + } + data; + }; + + UDPMessageSender fake_msg_sender; + + // Set common QoS + reader.disable_builtin_transport().add_user_transport_to_pparams(udp_transport) + .history_depth(10).reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS); + writer.history_depth(10).reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS); + + // Set custom reader locator so we can send malicious data to a known location + Locator_t reader_locator; + ASSERT_TRUE(IPLocator::setIPv4(reader_locator, "127.0.0.1")); + reader_locator.port = 7000; + reader.add_to_unicast_locator_list("127.0.0.1", 7000); + + // Initialize and wait for discovery + reader.init(); + ASSERT_TRUE(reader.isInitialized()); + writer.init(); + ASSERT_TRUE(writer.isInitialized()); + reader.wait_discovery(); + writer.wait_discovery(); + + // Send one sample + std::list data; + KeyedHelloWorld sample; + sample.key(0); + sample.index(1); + sample.message("KeyedHelloWorld 1 (key = 0)"); + data.push_back(sample); + reader.startReception(data); + writer.send(data); + ASSERT_TRUE(data.empty()); + + // Wait for the reader to receive the sample + reader.block_for_all(); + + // Send unregister disposed without PID_KEY_HASH, and long key-only payload + { + auto writer_guid = writer.datawriter_guid(); + + KeyOnlyBigPayloadDatagram malicious_packet{}; + malicious_packet.sender_prefix = writer_guid.guidPrefix; + malicious_packet.data.header.writer_id = writer_guid.entityId; + malicious_packet.data.header.reader_id = reader.datareader_guid().entityId; + malicious_packet.data.payload.encapsulation[1] = CDR_LE; + malicious_packet.data.payload.data.fill(0x00); + + CDRMessage_t msg(0); + uint32_t msg_len = static_cast(sizeof(malicious_packet)); + msg.init(reinterpret_cast(&malicious_packet), msg_len); + msg.length = msg_len; + msg.pos = msg_len; + fake_msg_sender.send(msg, reader_locator); + } + + // Wait some time to let the message be processed + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + +} + // Test for ==operator UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method // Test for copy UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method From 2ea6b9427af0baa8fb6a01be279bc769d2d9e554 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Tue, 24 Sep 2024 17:14:37 +0200 Subject: [PATCH 2/9] Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company --- .../common/BlackboxTestsTransportUDP.cpp | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index f5dc2b4f259..616cdcba52f 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -639,14 +639,10 @@ TEST(TransportUDP, MaliciousManipulatedDataOctetsToNextHeaderIgnore) /** * This is a regression test for redmine issue #21707. */ -TEST(TransportUDP, KeyOnlyBigPayloadIgnored) +static void KeyOnlyBigPayloadIgnored( + PubSubWriter& writer, + PubSubReader& reader) { - // Force using UDP transport - auto udp_transport = std::make_shared(); - - PubSubWriter writer(TEST_TOPIC_NAME); - PubSubReader reader(TEST_TOPIC_NAME); - struct KeyOnlyBigPayloadDatagram { std::array rtps_id{ {'R', 'T', 'P', 'S'} }; @@ -708,10 +704,11 @@ TEST(TransportUDP, KeyOnlyBigPayloadIgnored) UDPMessageSender fake_msg_sender; + // Force using UDP transport + auto udp_transport = std::make_shared(); + // Set common QoS - reader.disable_builtin_transport().add_user_transport_to_pparams(udp_transport) - .history_depth(10).reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS); - writer.history_depth(10).reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS); + reader.disable_builtin_transport().add_user_transport_to_pparams(udp_transport); // Set custom reader locator so we can send malicious data to a known location Locator_t reader_locator; @@ -765,6 +762,30 @@ TEST(TransportUDP, KeyOnlyBigPayloadIgnored) } +TEST(TransportUDP, KeyOnlyBigPayloadIgnored_Reliable) +{ + PubSubWriter writer(TEST_TOPIC_NAME); + PubSubReader reader(TEST_TOPIC_NAME); + + // Set reliability + reader.reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS); + writer.reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS); + + KeyOnlyBigPayloadIgnored(writer, reader); +} + +TEST(TransportUDP, KeyOnlyBigPayloadIgnored_BestEffort) +{ + PubSubWriter writer(TEST_TOPIC_NAME); + PubSubReader reader(TEST_TOPIC_NAME); + + // Set reliability + reader.reliability(eprosima::fastdds::dds::BEST_EFFORT_RELIABILITY_QOS); + writer.reliability(eprosima::fastdds::dds::BEST_EFFORT_RELIABILITY_QOS); + + KeyOnlyBigPayloadIgnored(writer, reader); +} + // Test for ==operator UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method // Test for copy UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method From 12821f7527d50b0d37eb4ddbce66a7694b2886b7 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Wed, 25 Sep 2024 09:27:53 +0200 Subject: [PATCH 3/9] Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company --- src/cpp/rtps/reader/reader_utils.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/cpp/rtps/reader/reader_utils.cpp b/src/cpp/rtps/reader/reader_utils.cpp index bf08fb3e6b5..c56875e7165 100644 --- a/src/cpp/rtps/reader/reader_utils.cpp +++ b/src/cpp/rtps/reader/reader_utils.cpp @@ -31,6 +31,12 @@ bool change_is_relevant_for_filter( { bool ret = true; + if ((change.serializedPayload.data == nullptr) && + ((change.kind == fastdds::rtps::ALIVE) || !change.instanceHandle.isDefined())) + { + ret = false; + } + // Only evaluate filter on ALIVE changes, as UNREGISTERED and DISPOSED are always relevant if ((nullptr != filter) && (fastdds::rtps::ALIVE == change.kind) && (!filter->is_relevant(change, reader_guid))) { From ffccf15fc415571b0dc67fa1c420920919966da1 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Wed, 25 Sep 2024 09:35:40 +0200 Subject: [PATCH 4/9] Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company --- src/cpp/rtps/messages/MessageReceiver.cpp | 48 +++++++++-------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp index 1cc8916e175..ce6d536548a 100644 --- a/src/cpp/rtps/messages/MessageReceiver.cpp +++ b/src/cpp/rtps/messages/MessageReceiver.cpp @@ -864,46 +864,36 @@ bool MessageReceiver::proc_Submsg_Data( } payload_size = smh->submessageLength - submsg_no_payload_size; - - if (dataFlag) + uint32_t next_pos = msg->pos + payload_size; + if (msg->length >= next_pos && payload_size > 0) { - uint32_t next_pos = msg->pos + payload_size; - if (msg->length >= next_pos && payload_size > 0) + if (dataFlag) { ch.serializedPayload.data = &msg->buffer[msg->pos]; ch.serializedPayload.length = payload_size; ch.serializedPayload.max_size = payload_size; - msg->pos = next_pos; } else { - EPROSIMA_LOG_WARNING(RTPS_MSG_IN, IDSTRING "Serialized Payload value invalid or larger than maximum allowed size" - "(" << payload_size << "/" << (msg->length - msg->pos) << ")"); - ch.serializedPayload.data = nullptr; - ch.inline_qos.data = nullptr; - return false; + if (payload_size <= PARAMETER_KEY_HASH_LENGTH) + { + memcpy(ch.instanceHandle.value, &msg->buffer[msg->pos], payload_size); + } + else + { + EPROSIMA_LOG_WARNING(RTPS_MSG_IN, IDSTRING "Ignoring Serialized Payload for too large key-only data (" << + payload_size << ")"); + } } + msg->pos = next_pos; } - else if (keyFlag) + else { - if (payload_size <= 0) - { - EPROSIMA_LOG_WARNING(RTPS_MSG_IN, IDSTRING "Serialized Payload value invalid (" << payload_size << ")"); - ch.serializedPayload.data = nullptr; - ch.inline_qos.data = nullptr; - return false; - } - - if (payload_size <= PARAMETER_KEY_HASH_LENGTH) - { - memcpy(ch.instanceHandle.value, &msg->buffer[msg->pos], payload_size); - } - else - { - EPROSIMA_LOG_WARNING(RTPS_MSG_IN, IDSTRING "Ignoring Serialized Payload for too large key-only data (" << - payload_size << ")"); - } - msg->pos += payload_size; + EPROSIMA_LOG_WARNING(RTPS_MSG_IN, IDSTRING "Serialized Payload value invalid or larger than maximum allowed size" + "(" << payload_size << "/" << (msg->length - msg->pos) << ")"); + ch.serializedPayload.data = nullptr; + ch.inline_qos.data = nullptr; + return false; } } From 2e5ffb05d5362fef1c8ab66f3f9c1a0223b69484 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Wed, 25 Sep 2024 09:40:04 +0200 Subject: [PATCH 5/9] Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company --- src/cpp/rtps/messages/MessageReceiver.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp index ce6d536548a..ffb752a87c7 100644 --- a/src/cpp/rtps/messages/MessageReceiver.cpp +++ b/src/cpp/rtps/messages/MessageReceiver.cpp @@ -877,7 +877,10 @@ bool MessageReceiver::proc_Submsg_Data( { if (payload_size <= PARAMETER_KEY_HASH_LENGTH) { - memcpy(ch.instanceHandle.value, &msg->buffer[msg->pos], payload_size); + if (!ch.instanceHandle.isDefined()) + { + memcpy(ch.instanceHandle.value, &msg->buffer[msg->pos], payload_size); + } } else { From 73f358690ef88f9b381d82b026b8b97f5a14fd1c Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Wed, 25 Sep 2024 09:43:04 +0200 Subject: [PATCH 6/9] Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company --- src/cpp/rtps/messages/MessageReceiver.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp index ffb752a87c7..3ef71893b3e 100644 --- a/src/cpp/rtps/messages/MessageReceiver.cpp +++ b/src/cpp/rtps/messages/MessageReceiver.cpp @@ -867,6 +867,7 @@ bool MessageReceiver::proc_Submsg_Data( uint32_t next_pos = msg->pos + payload_size; if (msg->length >= next_pos && payload_size > 0) { + FASTDDS_TODO_BEFORE(3, 1, "Pass keyFlag in serializedPayload, and always pass input data upwards"); if (dataFlag) { ch.serializedPayload.data = &msg->buffer[msg->pos]; From 43ae9a61a9d16e88e6e8d2c7dc00de28c404d348 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Thu, 26 Sep 2024 08:05:29 +0200 Subject: [PATCH 7/9] Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company --- src/cpp/rtps/reader/reader_utils.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cpp/rtps/reader/reader_utils.cpp b/src/cpp/rtps/reader/reader_utils.cpp index c56875e7165..efd2d3d52d9 100644 --- a/src/cpp/rtps/reader/reader_utils.cpp +++ b/src/cpp/rtps/reader/reader_utils.cpp @@ -31,8 +31,11 @@ bool change_is_relevant_for_filter( { bool ret = true; - if ((change.serializedPayload.data == nullptr) && - ((change.kind == fastdds::rtps::ALIVE) || !change.instanceHandle.isDefined())) + // If the change has no payload, it should have an instanceHandle. + // This is only allowed for UNREGISTERED and DISPOSED changes, where the instanceHandle is used to identify the + // instance to unregister or dispose. + if ((nullptr == change.serializedPayload.data) && + ((fastdds::rtps::ALIVE == change.kind) || !change.instanceHandle.isDefined())) { ret = false; } From 750eaa085ffd697e59405bd070d291b0e4347753 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Thu, 26 Sep 2024 08:08:41 +0200 Subject: [PATCH 8/9] Refs #21751. Document else. Signed-off-by: Miguel Company --- src/cpp/rtps/messages/MessageReceiver.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp index 3ef71893b3e..110e4ccb07b 100644 --- a/src/cpp/rtps/messages/MessageReceiver.cpp +++ b/src/cpp/rtps/messages/MessageReceiver.cpp @@ -874,7 +874,7 @@ bool MessageReceiver::proc_Submsg_Data( ch.serializedPayload.length = payload_size; ch.serializedPayload.max_size = payload_size; } - else + else // keyFlag would be true since we are inside an if (dataFlag || keyFlag) { if (payload_size <= PARAMETER_KEY_HASH_LENGTH) { From 125ea60a086af3277bf2fc825776f9074d6a3725 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Thu, 26 Sep 2024 09:46:26 +0200 Subject: [PATCH 9/9] Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company --- .../common/BlackboxTestsTransportUDP.cpp | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index 616cdcba52f..fdd5a8cdce4 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -645,10 +645,16 @@ static void KeyOnlyBigPayloadIgnored( { struct KeyOnlyBigPayloadDatagram { - std::array rtps_id{ {'R', 'T', 'P', 'S'} }; - std::array protocol_version{ {2, 3} }; - std::array vendor_id{ {0x01, 0x0F} }; - GuidPrefix_t sender_prefix{}; + struct RTPSHeader + { + std::array rtps_id{ {'R', 'T', 'P', 'S'} }; + std::array protocol_version{ {2, 3} }; + std::array vendor_id{ {0x01, 0x0F} }; + GuidPrefix_t sender_prefix{}; + } + header; + + static_assert(sizeof(RTPSHeader) == RTPSMESSAGE_HEADER_SIZE, "Unexpected size for RTPS header"); struct DataSubMsg { @@ -668,6 +674,8 @@ static void KeyOnlyBigPayloadIgnored( SequenceNumber_t sn{ 2 }; }; + static_assert(sizeof(Header) == RTPSMESSAGE_DATA_MIN_LENGTH, "Unexpected size for DATA header"); + struct InlineQoS { // PID_STATUS_INFO (unregistered + disposed) @@ -688,6 +696,8 @@ static void KeyOnlyBigPayloadIgnored( Sentinel sentinel; }; + static_assert(sizeof(InlineQoS) == 8 + 4, "Unexpected size for InlineQoS"); + struct SerializedData { std::array encapsulation {{0}}; @@ -695,13 +705,25 @@ static void KeyOnlyBigPayloadIgnored( std::array data {{0}}; }; + static_assert(sizeof(SerializedData) == 0x24 + 2 + 2, "Unexpected size for SerializedData"); + Header header; InlineQoS qos; SerializedData payload; } data; + + static_assert( + sizeof(DataSubMsg) == + sizeof(DataSubMsg::Header) + sizeof(DataSubMsg::InlineQoS) + sizeof(DataSubMsg::SerializedData), + "Unexpected size for DataSubMsg"); }; + static_assert( + sizeof(KeyOnlyBigPayloadDatagram) == + sizeof(KeyOnlyBigPayloadDatagram::RTPSHeader) + sizeof(KeyOnlyBigPayloadDatagram::DataSubMsg), + "Unexpected size for KeyOnlyBigPayloadDatagram"); + UDPMessageSender fake_msg_sender; // Force using UDP transport @@ -743,7 +765,7 @@ static void KeyOnlyBigPayloadIgnored( auto writer_guid = writer.datawriter_guid(); KeyOnlyBigPayloadDatagram malicious_packet{}; - malicious_packet.sender_prefix = writer_guid.guidPrefix; + malicious_packet.header.sender_prefix = writer_guid.guidPrefix; malicious_packet.data.header.writer_id = writer_guid.entityId; malicious_packet.data.header.reader_id = reader.datareader_guid().entityId; malicious_packet.data.payload.encapsulation[1] = CDR_LE;