Assertion failure in SequenceNumber.h via malformed SPDP packet only when compiled in logging-enabled (Debug) mode

[squizz617]

Is there an already existing issue for this?

• I have searched the existing issues

Expected behavior

Malformed submessages handled gracefully.

Current behavior

I came across two following issues.

1. An SPDP payload with a malformed heartbeat submessage triggers an assertion failure at fastrtps/include/fastdds/rtps/common/SequenceNumber.h:247.
2. This only happens when fastrtps is compiled with the following CMake arguments for logging:

--cmake-args -DCMAKE_BUILD_TYPE=Debug -DLOG_NO_INFO=OFF -DINTERNAL_DEBUG=ON -DLOG_CONSUMER_DEFAULT=STDOUT -DLOG_NO_WARNING=OFF -DLOG_NO_ERROR=OFF

Steps to reproduce

1. Build fastdds v2.9.0 and HelloWorldExample with logging (Debug mode)

mkdir -p ~/fastdds-log/src
cd ~/fastdds-log
wget https://raw.githubusercontent.com/eProsima/Fast-DDS/v2.9.0/fastrtps.repos
vcs import src < fastrtps.repos
colcon build --cmake-args -DCMAKE_BUILD_TYPE=Debug -DLOG_NO_INFO=OFF -DINTERNAL_DEBUG=ON -DLOG_CONSUMER_DEFAULT=STDOUT -DLOG_NO_WARNING=OFF -DLOG_NO_ERROR=OFF
source install/setup.sh
cd src/fastrtps/examples/cpp/dds/HelloWorldExample
cmake .
make

2. Build fastdds v2.9.0 and HelloWorldExample without logging

mkdir -p ~/fastdds-nolog/src
cd ~/fastdds-nolog
wget https://raw.githubusercontent.com/eProsima/Fast-DDS/v2.9.0/fastrtps.repos
vcs import src < fastrtps.repos
colcon build
source install/setup.sh
cd src/fastrtps/examples/cpp/dds/HelloWorldExample
cmake .
make

3. Launch DDSHelloWorldExample with logging and send the malformed packet

   • Launching:

   source ~/fastdds-log/install/setup.sh && ~/fastdds-log/src/fastrtps/examples/cpp/dds/HelloWorldExample/DDSHelloWorldExample publisher
   

   • Hexdump of the SPDP packet to send (to 239.255.0.1:7400):

   0000  45 00 00 68 00 01 40 00 40 11 D9 76 80 3D F0 CF  E..h..@.@..v.=..
   0010  EF FF 00 01 05 39 1C E8 00 54 08 B9 52 54 50 53  .....9...T..RTPS
   0020  02 04 01 0F 01 03 02 42 AC 11 00 02 45 E5 E2 FD  .......B....E...
   0030  07 BF 34 00 00 00 10 00 00 00 00 00 00 01 00 C2  ..4.............
   0040  00 00 00 00 A7 9B EA 8C BE ED DC CB 00 03 00 00  ................
   0050  77 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00  w...............
   0060  00 00 00 00 00 00 00 00                          ........
   

   • Result after sending the packet:

   Starting 
   Publisher running 10 samples.
   DDSHelloWorldExample: /home/seulbae/fastdds-log/src/fastrtps/include/fastdds/rtps/common/SequenceNumber.h:247: eprosima::fastrtps::rtps::SequenceNumber_t eprosima::fastrtps::rtps::operator-(const eprosima::fastrtps::rtps::SequenceNumber_t&, uint32_t): Assertion `0 < res.high' failed.
   [1]    3561127 abort       publisher
   

   Assertion failure has been triggered!

4. Launch DDSHelloWorldExample without logging and send the malformed packet

   • Launching:

   source ~/fastdds-nolog/install/setup.sh && ~/fastdds-nolog/src/fastrtps/examples/cpp/dds/HelloWorldExample/DDSHelloWorldExample publisher
   

   • Send the same SPDP packet

   • Result after sending:

   Starting 
   Publisher running 10 samples.
   

   No assertion failure this time..

Fast DDS version/commit

Tag v2.9.0 (4c55488)

Platform/Architecture

Ubuntu Focal 20.04 amd64

Transport layer

Default configuration, UDPv4 & SHM

Additional context

Alternatively, you can directly use this PoC to quickly test the behavior. But make sure to compile this with an afl compiler (e.g., afl-clang-fast++) or manually enable the FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION flag to keep participantGuidPrefix uninitialized.

Thank you.

XML configuration file

No response

Relevant log output

No response

Network traffic capture

No response

[squizz617]

[Update] sharing my analysis. Seems like the culprit is the compiler optimization.

The heartbeat submessage has bad sequence numbers, i.e.,

firstSN.high = 0xc2000100 // -1040187136
firstSN.low = 0x00

lastSN.high = 0x8cea9ba7
lastSN.low = 0xcbdcedbe

The sanity check line at MessageReceiver.cpp:1057, invokes the - operator (firstSN - 1):

inline SequenceNumber_t operator -(
        const SequenceNumber_t& seq,
        const uint32_t inc) noexcept
{
    SequenceNumber_t res(seq.high, seq.low - inc);

    if (inc > seq.low)
    {
        // Being the type of the parameter an 'uint32_t', the decrement of 'high' will be as much as 1.
        assert(0 < res.high);
        --res.high;
    }

    return res;
}

in which inc (== 1) > seq.low (== 0) is false and the assert() inside is triggered as 0 < res.high (== -1040187136) is false.

However, when built with default config using colcon and cmake, which sets -O2 or -O3, the check is replaced with other instructions and the block is never executed, hence no assertion failure. More info about this is here (pages 5-9).

[MiguelCompany]

@squizz617 Thank you for the report and the analysis.

We will try to have a regression test and a fix.

I think that MessageReceiver should first check that the received sequences are valid (i.e. they have a positive high part), and then check for (lastSN + 1) != firstSN, to avoid calling the substraction operand.

[squizz617]

Hi @MiguelCompany, thank you for your reply.

I think that MessageReceiver should first check that the received sequences are valid

I agree. Having a validity check on firstSN.value is also congruent with what's stated in section 8.3.8.6.3 of the RTPS specification:

"the submessage is invalid if firstSN.value is zero or negative"

[squizz617]

Hi @MiguelCompany, could you assign a CVE ID for this issue? A relevant CWE would be CWE-617: Reachable Assertion.

[carnil]

CVE-2023-39949 appears to have been assigned for this issue.