search

eclipse-cyclonedds / cyclonedds

Eclipse Cyclone DDS project
https://projects.eclipse.org/projects/iot.cyclonedds
Other
843 stars 350 forks source link

Denial of Service due to an invalid locator in DATA submessage #1772

Open squizz617 opened 1 year ago

squizz617 commented 1 year ago

Hi,

After the initial PDP handshake, the second PDP message that contains an invalid METATRAFFIC_UNICAST_LOCATOR, i.e., 10.0.0.15:0, seems to cause a denial of service.

Invalid locator param (bytes 0x40 - 0x5b):

0000   32 00 18 00 01 00 00 00 00 00 00 00 00 00 00 00
0010   00 00 00 00 00 00 00 00 0a 00 00 0f

Wireshark dissection of the above param:

PID_METATRAFFIC_UNICAST_LOCATOR (LOCATOR_KIND_UDPV4, 10.0.0.15:0)
    parameterId: PID_METATRAFFIC_UNICAST_LOCATOR (0x0032)
    parameterLength: 24
    locator
        Kind: LOCATOR_KIND_UDPV4 (0x00000001)
        Port: 0
            [Expert Info (Warning/Protocol): Invalid Port]
        Address: 10.0.0.15

It doesn't respond to any further packets at this point.

Thank you.

eboasson commented 1 year ago

Thank you!

It shouldn't accept the port number 0 (and that's why sendmsg keeps failing), so that's clearly a bug.

Fixing that bug won't resolve the underlying problem: if you provide incorrect locators in the discovery messages, then there won't be communication, and it is possible to update these locators in subsequent discovery messages. So if instead of a port number 0 you'd have given in the wrong address in a locator, you'd still have a problem even if the UDP stack would no longer return an error.

In pretty sure there are ways in which Cyclone DDS' behaviour can be improved in such cases, but I am inclined to think that protecting against this problem can only be mitigated by authenticating the discovery data. Would you agree?

squizz617 commented 1 year ago

Thank you for the prompt confirmation!

if instead of a port number 0 you'd have given in the wrong address in a locator, you'd still have a problem

I was focusing mostly on the security perspective, i.e., whether an attacker can crash/hang a Cyclone DDS client. Port number 0 definitely triggers a DoS. However, provided a wrong IPv4 address (you mean a non-existent address, right?), doesn't Cyclone DDS "bounce back" after the lease duration expires and then accept other connections? From this perspective, the behavior seems legitimate to me. What do you think?

eboasson commented 1 year ago

It should always allow accepting new peers and continue to communicate with other peers. If it didn't in this case, that's interesting and an issue in itself.

(you mean a non-existent address, right?)

Non-existent, or just some other machine or port number other than what it actually accepts packets on.

The lease expiry takes care of removing a peer that it no longer receives data from, but if that peer keeps sending packets and renewing its lease, while advertising an address other than its own address[^1], then it will remain in existence. The "victim" node can only discover this through the absence of ACKs or, if you're lucky, ICMP "host unreachable" messages. It doesn't listen for the latter (it would be nice if it did, though) and pays scant attention to whether or not is receiving ACKs (I think it really should pay more attention to that).

So if all the data being sent by the victim is from writers that use either keep-last history setting, or unreliable ("best-effort" in the spec) communication, then it'll happily continue wasting bandwidth because it doesn't run into any limit or timer.

If there is a reliable, keep-all writer, then at some point it has to receive an ACK to make progress, or the writer's max_blocking_time kicks in. I thought there was also a timeout for deciding a reader is "unresponsive" in this case, but that apparently exists only in my head ...

[^1]: Presumably deliberately, though a network configuration change also does this until Cyclone detects and handles those.