Skip to content

Commit d6dd58f

Browse files
committed
Fix CVE-2025-62799
This commit fixes CVE-2025-62799 by refactoring the handling of RTPS DATA_FRAG messages and reserving space for possible alignment required to store a fragment index. This is an squashed commit of a privately reviewed branch. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Reviewed-by: cferreiragonz <carlosferreira@eprosima.com>
1 parent 19d6a2b commit d6dd58f

File tree

9 files changed

+269
-9
lines changed

9 files changed

+269
-9
lines changed

include/fastdds/rtps/common/CacheChange.hpp

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include <atomic>
2323
#include <cassert>
24+
#include <limits>
2425

2526
#include <fastdds/rtps/common/ChangeKind_t.hpp>
2627
#include <fastdds/rtps/common/FragmentNumber.hpp>
@@ -328,6 +329,62 @@ struct FASTDDS_EXPORTED_API CacheChange_t
328329
return is_fully_assembled();
329330
}
330331

332+
/**
333+
* @brief Calculate the minimum required payload size to store a fragmented change.
334+
*
335+
* @param[in] payload_size Size of the full payload.
336+
* @param[in] fragment_size Size of each fragment.
337+
* @param[out] min_required_size Minimum required size to store the fragmented payload.
338+
*/
339+
static bool calculate_required_fragmented_payload_size(
340+
uint32_t payload_size,
341+
uint16_t fragment_size,
342+
uint32_t& min_required_size)
343+
{
344+
if ((0 == fragment_size) || (payload_size <= fragment_size))
345+
{
346+
min_required_size = payload_size;
347+
return true;
348+
}
349+
350+
// In order to avoid overflow on the calculations, we limit the maximum payload size
351+
constexpr uint32_t MAX_PAYLOAD_SIZE = std::numeric_limits<uint32_t>::max() - 4u - 3u;
352+
if (payload_size > MAX_PAYLOAD_SIZE)
353+
{
354+
return false;
355+
}
356+
357+
// Ensure fragment size is at least 4 bytes to store fragment index
358+
if (fragment_size < 4u)
359+
{
360+
return false;
361+
}
362+
363+
// Calculate number of fragments without risk of overflow
364+
uint32_t fragment_count = payload_size / fragment_size;
365+
if (0 != (payload_size % fragment_size))
366+
{
367+
++fragment_count;
368+
}
369+
370+
// This cannot overflow as the result will always be <= payload_size
371+
uint32_t last_fragment_offset = (fragment_count - 1) * fragment_size;
372+
373+
// Since we will write a fragment index at the beginning of each fragment,
374+
// we need to ensure there is space for it in the last fragment.
375+
// Note: we already imposed limits to ensure no overflow occurs.
376+
min_required_size = (last_fragment_offset + 3u) & ~3u; // Align last fragment size to 4 bytes
377+
min_required_size += 4u; // Add fragment index size
378+
379+
// Ensure minimum size is at least payload size
380+
if (min_required_size < payload_size)
381+
{
382+
min_required_size = payload_size;
383+
}
384+
385+
return true;
386+
}
387+
331388
private:
332389

333390
// Fragment size

src/cpp/rtps/builtin/discovery/endpoint/EDPSimple.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ void EDPSimple::processPersistentData(
277277

278278
CacheChange_t* change_to_add = nullptr;
279279

280-
if (!reader.first->reserve_cache(change->serializedPayload.length, change_to_add)) //Reserve a new cache from the corresponding cache pool
280+
if (!reader.first->reserve_cache(change->serializedPayload.length, 0, change_to_add)) //Reserve a new cache from the corresponding cache pool
281281
{
282282
EPROSIMA_LOG_ERROR(RTPS_EDP, "Problem reserving CacheChange in EDPServer reader");
283283
return;

src/cpp/rtps/builtin/discovery/participant/PDPServer.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1033,7 +1033,7 @@ bool PDPServer::remove_remote_participant(
10331033

10341034
// TODO check in standard if DROP payload is always 0
10351035
// We create the drop from Reader to make release simplier
1036-
endpoints->reader.reader_->reserve_cache(mp_builtin->m_att.writerPayloadSize, pC);
1036+
endpoints->reader.reader_->reserve_cache(mp_builtin->m_att.writerPayloadSize, 0, pC);
10371037

10381038
// We must create the corresponding DATA(p[UD])
10391039
if (nullptr != pC)
@@ -1718,7 +1718,7 @@ bool PDPServer::process_backup_discovery_database_restore(
17181718
std::istringstream(it.value()["change"]["sample_identity"].get<std::string>()) >> sample_identity_aux;
17191719

17201720
// Reserve memory for new change. There will not be changes from own server
1721-
if (!endpoints->reader.reader_->reserve_cache(length, change_aux))
1721+
if (!endpoints->reader.reader_->reserve_cache(length, 0, change_aux))
17221722
{
17231723
EPROSIMA_LOG_ERROR(RTPS_PDP_SERVER, "Error creating CacheChange");
17241724
// TODO release changes and exit
@@ -1757,7 +1757,7 @@ bool PDPServer::process_backup_discovery_database_restore(
17571757
else
17581758
{
17591759
// Reserve memory for new change. There will not be changes from own server
1760-
if (!edp->publications_reader_.first->reserve_cache(length, change_aux))
1760+
if (!edp->publications_reader_.first->reserve_cache(length, 0, change_aux))
17611761
{
17621762
EPROSIMA_LOG_ERROR(RTPS_PDP_SERVER, "Error creating CacheChange");
17631763
// TODO release changes and exit
@@ -1796,7 +1796,7 @@ bool PDPServer::process_backup_discovery_database_restore(
17961796
else
17971797
{
17981798
// Reserve memory for new change. There will not be changes from own server
1799-
if (!edp->subscriptions_reader_.first->reserve_cache(length, change_aux))
1799+
if (!edp->subscriptions_reader_.first->reserve_cache(length, 0, change_aux))
18001800
{
18011801
EPROSIMA_LOG_ERROR(RTPS_PDP_SERVER, "Error creating CacheChange");
18021802
// TODO release changes and exit

src/cpp/rtps/reader/BaseReader.cpp

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#include <rtps/reader/BaseReader.hpp>
2020

21+
#include <algorithm>
2122
#include <cassert>
2223
#include <cstdint>
2324
#include <mutex>
@@ -279,21 +280,45 @@ std::shared_ptr<LocalReaderPointer> BaseReader::get_local_pointer()
279280

280281
bool BaseReader::reserve_cache(
281282
uint32_t cdr_payload_size,
283+
uint16_t fragment_size,
282284
fastdds::rtps::CacheChange_t*& change)
283285
{
284286
std::lock_guard<decltype(mp_mutex)> guard(mp_mutex);
285287

286288
change = nullptr;
287289

290+
// Calculate and validate required payload size
291+
uint32_t reserve_size = fixed_payload_size_ > 0 ? fixed_payload_size_ : cdr_payload_size;
292+
uint32_t payload_size = std::min(reserve_size, cdr_payload_size);
293+
uint32_t min_required_size = 0;
294+
if (!CacheChange_t::calculate_required_fragmented_payload_size(payload_size, fragment_size, min_required_size))
295+
{
296+
EPROSIMA_LOG_WARNING(RTPS_READER,
297+
"Required payload size calculation overflows for payload size '" << payload_size <<
298+
"' and fragment size '" << fragment_size << "'");
299+
return false;
300+
}
301+
302+
if (min_required_size > reserve_size)
303+
{
304+
if (fixed_payload_size_ > 0)
305+
{
306+
EPROSIMA_LOG_WARNING(RTPS_READER,
307+
"Fixed payload size '" << fixed_payload_size_ <<
308+
"' is insufficient for fragmentation with fragment size '" << fragment_size << "'");
309+
return false;
310+
}
311+
reserve_size = min_required_size;
312+
}
313+
288314
fastdds::rtps::CacheChange_t* reserved_change = nullptr;
289315
if (!change_pool_->reserve_cache(reserved_change))
290316
{
291317
EPROSIMA_LOG_WARNING(RTPS_READER, "Problem reserving cache from pool");
292318
return false;
293319
}
294320

295-
uint32_t payload_size = fixed_payload_size_ ? fixed_payload_size_ : cdr_payload_size;
296-
if (!payload_pool_->get_payload(payload_size, reserved_change->serializedPayload))
321+
if (!payload_pool_->get_payload(reserve_size, reserved_change->serializedPayload))
297322
{
298323
change_pool_->release_cache(reserved_change);
299324
EPROSIMA_LOG_WARNING(RTPS_READER, "Problem reserving payload from pool");

src/cpp/rtps/reader/BaseReader.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,12 +177,14 @@ class BaseReader
177177
* @brief Reserve a CacheChange_t.
178178
*
179179
* @param [in] cdr_payload_size Size of the received payload.
180+
* @param [in] fragment_size Size of each fragment (0 if not fragmented).
180181
* @param [out] change Pointer to the reserved change.
181182
*
182183
* @return True if correctly reserved.
183184
*/
184185
bool reserve_cache(
185186
uint32_t cdr_payload_size,
187+
uint16_t fragment_size,
186188
fastdds::rtps::CacheChange_t*& change);
187189

188190
/**

src/cpp/rtps/reader/StatefulReader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,7 @@ bool StatefulReader::process_data_frag_msg(
738738
if (!history_->get_change(change_to_add->sequenceNumber, change_to_add->writerGUID, &work_change))
739739
{
740740
// A new change should be reserved
741-
if (reserve_cache(sampleSize, work_change))
741+
if (reserve_cache(sampleSize, change_to_add->getFragmentSize(), work_change))
742742
{
743743
if (work_change->serializedPayload.max_size < sampleSize)
744744
{

src/cpp/rtps/reader/StatelessReader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,7 @@ bool StatelessReader::process_data_frag_msg(
793793
// Check if a new change should be reserved
794794
if (work_change == nullptr)
795795
{
796-
if (reserve_cache(sampleSize, work_change))
796+
if (reserve_cache(sampleSize, change_to_add->getFragmentSize(), work_change))
797797
{
798798
if (work_change->serializedPayload.max_size < sampleSize)
799799
{

test/blackbox/common/BlackboxTestsTransportUDP.cpp

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -808,6 +808,103 @@ TEST(TransportUDP, KeyOnlyBigPayloadIgnored_BestEffort)
808808
KeyOnlyBigPayloadIgnored(writer, reader);
809809
}
810810

811+
// Regression test for redmine issue #23830
812+
TEST(TransportUDP, MaliciousDataFragUnalignedSizes)
813+
{
814+
// Force using UDP transport
815+
auto udp_transport = std::make_shared<UDPv4TransportDescriptor>();
816+
817+
PubSubWriter<UnboundedHelloWorldPubSubType> writer(TEST_TOPIC_NAME);
818+
PubSubReader<UnboundedHelloWorldPubSubType> reader(TEST_TOPIC_NAME);
819+
820+
struct MaliciousDataFragUnalignedSizes
821+
{
822+
std::array<char, 4> rtps_id{ {'R', 'T', 'P', 'S'} };
823+
std::array<uint8_t, 2> protocol_version{ {2, 3} };
824+
std::array<uint8_t, 2> vendor_id{ {0x01, 0x0F} };
825+
GuidPrefix_t sender_prefix{};
826+
827+
struct DataFragSubMsg
828+
{
829+
struct Header
830+
{
831+
uint8_t submessage_id = 0x16;
832+
#if FASTDDS_IS_BIG_ENDIAN_TARGET
833+
uint8_t flags = 0x00;
834+
#else
835+
uint8_t flags = 0x01;
836+
#endif // FASTDDS_IS_BIG_ENDIAN_TARGET
837+
uint16_t octets_to_next_header = 0x22;
838+
uint16_t extra_flags = 0;
839+
uint16_t octets_to_inline_qos = 0x1c;
840+
EntityId_t reader_id{};
841+
EntityId_t writer_id{};
842+
SequenceNumber_t sn{100};
843+
uint32_t fragment_starting_num = 2;
844+
uint16_t fragments_in_submessage = 1;
845+
uint16_t fragment_size = 50002;
846+
uint32_t sample_size = 50003;
847+
};
848+
849+
struct SerializedData
850+
{
851+
octet data[2] {0xAA, 0xAA};
852+
};
853+
854+
Header header;
855+
SerializedData payload;
856+
}
857+
data;
858+
};
859+
860+
UDPMessageSender fake_msg_sender;
861+
862+
// Set common QoS
863+
reader.disable_builtin_transport().add_user_transport_to_pparams(udp_transport)
864+
.history_depth(10).reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS);
865+
writer.history_depth(10).reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS);
866+
867+
// Set custom reader locator so we can send malicious data to a known location
868+
Locator_t reader_locator;
869+
ASSERT_TRUE(IPLocator::setIPv4(reader_locator, "127.0.0.1"));
870+
reader_locator.port = 7000;
871+
reader.add_to_unicast_locator_list("127.0.0.1", 7000);
872+
873+
// Initialize and wait for discovery
874+
reader.init();
875+
ASSERT_TRUE(reader.isInitialized());
876+
writer.init();
877+
ASSERT_TRUE(writer.isInitialized());
878+
879+
reader.wait_discovery();
880+
writer.wait_discovery();
881+
882+
auto data = default_unbounded_helloworld_data_generator();
883+
reader.startReception(data);
884+
writer.send(data);
885+
ASSERT_TRUE(data.empty());
886+
887+
// Send malicious data
888+
{
889+
auto writer_guid = writer.datawriter_guid();
890+
891+
MaliciousDataFragUnalignedSizes malicious_packet{};
892+
malicious_packet.sender_prefix = writer_guid.guidPrefix;
893+
malicious_packet.data.header.writer_id = writer_guid.entityId;
894+
malicious_packet.data.header.reader_id = reader.datareader_guid().entityId;
895+
896+
CDRMessage_t msg(0);
897+
uint32_t msg_len = static_cast<uint32_t>(sizeof(malicious_packet));
898+
msg.init(reinterpret_cast<octet*>(&malicious_packet), msg_len);
899+
msg.length = msg_len;
900+
msg.pos = msg_len;
901+
fake_msg_sender.send(msg, reader_locator);
902+
}
903+
904+
// Block reader until reception finished or timeout.
905+
reader.block_for_all();
906+
}
907+
811908
// Test for ==operator UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method
812909
// Test for copy UDPTransportDescriptor is not required as it is an abstract class and in UDPv4 is same method
813910

test/unittest/rtps/common/CacheChangeTests.cpp

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,85 @@ TEST(CacheChange, FragmentManagement)
142142
}
143143
}
144144

145+
TEST(CacheChange, calculate_required_fragmented_payload_size)
146+
{
147+
struct TestCase
148+
{
149+
uint32_t payload_size;
150+
uint16_t fragment_size;
151+
bool expected_result;
152+
uint32_t expected_min_required_size;
153+
};
154+
155+
const std::vector<TestCase> test_cases =
156+
{
157+
// No fragmentation
158+
{100u, 0u, true, 100u},
159+
{50u, 100u, true, 50u},
160+
// Minimum fragment size
161+
{100u, 1u, false, 0u},
162+
{100u, 2u, false, 0u},
163+
{100u, 3u, false, 0u},
164+
// Fragmentation cases
165+
{100u, 20u, true, 100u},
166+
{101u, 20u, true, 104u},
167+
{102u, 20u, true, 104u},
168+
{103u, 20u, true, 104u},
169+
{50004u, 50003u, true, 50008u},
170+
{50003u, 50002u, true, 50008u},
171+
{50002u, 50001u, true, 50008u},
172+
{50001u, 50000u, true, 50004u},
173+
// Typical UDP max fragment size
174+
{1234567u, 64000u, true, 1234567u},
175+
{1234568u, 64000u, true, 1234568u},
176+
{1234569u, 64000u, true, 1234569u},
177+
{1234570u, 64000u, true, 1234570u},
178+
// Edge cases
179+
{5u, 4u, true, 8u},
180+
{6u, 4u, true, 8u},
181+
{7u, 4u, true, 8u},
182+
{8u, 4u, true, 8u},
183+
{
184+
std::numeric_limits<uint32_t>::max() - 4u - 3u,
185+
std::numeric_limits<uint16_t>::max(),
186+
true,
187+
std::numeric_limits<uint32_t>::max() - 4u - 3u
188+
},
189+
{
190+
std::numeric_limits<uint32_t>::max() - 4u - 3u,
191+
4u,
192+
true,
193+
std::numeric_limits<uint32_t>::max() - 4u - 3u
194+
}
195+
};
196+
197+
for (const auto& test_case : test_cases)
198+
{
199+
uint32_t min_required_size = 0;
200+
bool result = CacheChange_t::calculate_required_fragmented_payload_size(
201+
test_case.payload_size,
202+
test_case.fragment_size,
203+
min_required_size);
204+
EXPECT_EQ(result, test_case.expected_result)
205+
<< "Failed for payload_size=" << test_case.payload_size
206+
<< ", fragment_size=" << test_case.fragment_size;
207+
if (result)
208+
{
209+
EXPECT_EQ(min_required_size, test_case.expected_min_required_size)
210+
<< "Failed for payload_size=" << test_case.payload_size
211+
<< ", fragment_size=" << test_case.fragment_size;
212+
}
213+
}
214+
215+
// Oversized payload
216+
constexpr uint32_t MAX_PAYLOAD_SIZE = std::numeric_limits<uint32_t>::max() - 4u - 3u;
217+
for (uint32_t payload_size = std::numeric_limits<uint32_t>::max(); payload_size > MAX_PAYLOAD_SIZE; --payload_size)
218+
{
219+
uint32_t min_required_size = 0;
220+
EXPECT_FALSE(CacheChange_t::calculate_required_fragmented_payload_size(payload_size, 20, min_required_size));
221+
}
222+
}
223+
145224
int main(
146225
int argc,
147226
char** argv)

0 commit comments

Comments
 (0)