From eb733249a16f6a5268f3570fcaaf8490e8f27b03 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Fri, 27 Sep 2024 10:16:26 +0200 Subject: [PATCH 1/3] Discard changes with big key-only payload and no key hash (#5262) * Refs #21707. Regression test. Signed-off-by: Miguel Company * Refs #21707. Test both best_effort and reliable. Signed-off-by: Miguel Company * Refs #21707. Fix by considering offending change as irrelevant. Signed-off-by: Miguel Company * Refs #21707. Refactor code in MessageReceiver. Signed-off-by: Miguel Company * Refs #21707. Avoid overriding instanceHandle. Signed-off-by: Miguel Company * Refs #21707. Add TODO so we don't forget to correctly process key-only payloads in the future. Signed-off-by: Miguel Company * Refs #21751. Document and improve code in `change_is_relevant_for_filter`. Signed-off-by: Miguel Company * Refs #21751. Document else. Signed-off-by: Miguel Company * Refs #21751. Added static assertions for structure sizes. Signed-off-by: Miguel Company --------- Signed-off-by: Miguel Company (cherry picked from commit 175add5c2600811058e65256adae62b2ff16c772) # Conflicts: # test/blackbox/common/BlackboxTestsTransportUDP.cpp --- src/cpp/rtps/messages/MessageReceiver.cpp | 54 +++--- src/cpp/rtps/reader/reader_utils.cpp | 9 + .../common/BlackboxTestsTransportUDP.cpp | 176 ++++++++++++++++++ 3 files changed, 209 insertions(+), 30 deletions(-) diff --git a/src/cpp/rtps/messages/MessageReceiver.cpp b/src/cpp/rtps/messages/MessageReceiver.cpp index 9ccddad21ed..90658369dbe 100644 --- a/src/cpp/rtps/messages/MessageReceiver.cpp +++ b/src/cpp/rtps/messages/MessageReceiver.cpp @@ -862,46 +862,40 @@ 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) + FASTDDS_TODO_BEFORE(3, 1, "Pass keyFlag in serializedPayload, and always pass input data upwards"); + 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 + else // keyFlag would be true since we are inside an if (dataFlag || keyFlag) { - 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) + { + if (!ch.instanceHandle.isDefined()) + { + 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; } } diff --git a/src/cpp/rtps/reader/reader_utils.cpp b/src/cpp/rtps/reader/reader_utils.cpp index ed7a024f498..1b4fe88abea 100644 --- a/src/cpp/rtps/reader/reader_utils.cpp +++ b/src/cpp/rtps/reader/reader_utils.cpp @@ -31,6 +31,15 @@ bool change_is_relevant_for_filter( { bool ret = true; + // 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; + } + // Only evaluate filter on ALIVE changes, as UNREGISTERED and DISPOSED are always relevant if ((nullptr != filter) && (fastrtps::rtps::ALIVE == change.kind) && (!filter->is_relevant(change, reader_guid))) { diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index a8b0334a40f..04871efac87 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -15,7 +15,11 @@ #include #include #include +<<<<<<< HEAD #include +======= +#include +>>>>>>> 175add5c2 (Discard changes with big key-only payload and no key hash (#5262)) #include #include @@ -652,6 +656,178 @@ TEST(TransportUDP, MaliciousManipulatedDataOctetsToNextHeaderIgnore) reader.block_for_all(); } +/** + * This is a regression test for redmine issue #21707. + */ +static void KeyOnlyBigPayloadIgnored( + PubSubWriter& writer, + PubSubReader& reader) +{ + struct KeyOnlyBigPayloadDatagram + { + 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 + { + 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 }; + }; + + static_assert(sizeof(Header) == RTPSMESSAGE_DATA_MIN_LENGTH, "Unexpected size for DATA header"); + + 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; + }; + + static_assert(sizeof(InlineQoS) == 8 + 4, "Unexpected size for InlineQoS"); + + struct SerializedData + { + std::array encapsulation {{0}}; + std::array encapsulation_opts {{0}}; + 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 + auto udp_transport = std::make_shared(); + + // Set common 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; + 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.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; + 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(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 9bbfb48e11fc721428d932773686769fe4942c09 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Fri, 27 Sep 2024 10:27:33 +0200 Subject: [PATCH 2/3] Fix conflicts Signed-off-by: Miguel Company --- test/blackbox/common/BlackboxTestsTransportUDP.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/blackbox/common/BlackboxTestsTransportUDP.cpp b/test/blackbox/common/BlackboxTestsTransportUDP.cpp index 04871efac87..173a456c24e 100644 --- a/test/blackbox/common/BlackboxTestsTransportUDP.cpp +++ b/test/blackbox/common/BlackboxTestsTransportUDP.cpp @@ -15,11 +15,8 @@ #include #include #include -<<<<<<< HEAD #include -======= #include ->>>>>>> 175add5c2 (Discard changes with big key-only payload and no key hash (#5262)) #include #include From 76505330c5529dc6ee02b0d856856f4f40a3304f Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Fri, 27 Sep 2024 10:35:17 +0200 Subject: [PATCH 3/3] Fix build Signed-off-by: Miguel Company --- src/cpp/rtps/reader/reader_utils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpp/rtps/reader/reader_utils.cpp b/src/cpp/rtps/reader/reader_utils.cpp index 1b4fe88abea..67b3c98b537 100644 --- a/src/cpp/rtps/reader/reader_utils.cpp +++ b/src/cpp/rtps/reader/reader_utils.cpp @@ -35,7 +35,7 @@ bool change_is_relevant_for_filter( // 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())) + ((fastrtps::rtps::ALIVE == change.kind) || !change.instanceHandle.isDefined())) { ret = false; }