Potential vulnerability: topic names from ROS parameters

[tandf]

Hi,

We notice that you are using topic names from ROS parameter at the following locations:

tf_remapper_cpp/src/tf_remapper_node.cpp

Line 54 in 59eca1c

this->oldTfTopic, 100, &TfRemapperNode::oldTfCallback, this);

tf_remapper_cpp/src/tf_remapper_node.cpp

Line 56 in 59eca1c

this->remappedTfTopic, 100, this->staticTf);

tf_remapper_cpp/src/tf_remapper_node.cpp

Line 60 in 59eca1c

this->remappedTfTopic, 100, &TfRemapperNode::remappedTfCallback, this);

tf_remapper_cpp/src/tf_remapper_node.cpp

Line 62 in 59eca1c

this->oldTfTopic, 100, this->staticTf);

For security reasons detailed below, we strongly suggest avoiding the usage of strings from parameters as topic names.

Although parameters are usually set in parameter files, they can also be changed by nodes. Specifically, other nodes in the same ROS application can also change the parameters listed above before it’s used, either by accident or intentionally (i.e., by potential attackers).
Such changes can lead to disruption of the remapper functionality, as the input or the output tf topic is changed. Moreover, if an attacker exists, she can even change the tf message content by first fool the remapper to publish to a wrong topic like /tf_fake, and then forwarding messages from /tf_fake to /tf after modifying the message contents. Downstream functionalities like navigation will be affected, and severe consequences such as crashes may happen. Because ROS is an OSS (open-source software) community, third-party nodes are widely used in ROS applications, usually without complete vetting of their behavior, which gives the opportunity to potentially malicious actors to inject malicious code (e.g, by submitting hypocrite commits like in other OSS systems [1]) to infiltrate the ROS applications that use it (or software supply chain attacks, one of the primary means for real-world attackers today [2]).

We understand that using parameters to set topic names brings flexibility. Still, for the purpose of security, we strongly suggest that you avoid such vulnerable programming patterns if possible. For example, to avoid the exposure of this specific vulnerability, you may consider alternatives like remapping, which is designed for configuring names when launching the nodes.

[1] Q. Wu and K. Lu, “On the feasibility of stealthily introducing vulnerabilities in open-source software via hypocrite commits,” 2021, https://linuxreviews.org/images/d/d9/OpenSourceInsecurity.pdf.
[2] Supply chain attacks are the hacker’s new favourite weapon. and the threat is getting bigger. https://www.zdnet.com/article/supply-chain-attacks-are-the-hackers-new-favourite-weapon-and-the-threat-is-getting-bigger/.

[peci1]

Thank you for the report. I understand the merit of the vulnerability, though I'm not convinced ROS1 nodes can change parameters during runtime in such a way that they affect the already running remapper. In my view, this can only happen during launch time, so the problem is not as big as you picture it.

If a system should be used in industrial conditions, I assume it is the responsibility of the programmer to make sure that only known and required parameters are set and unexpected parameters are not. And that is easy to check during launch time.

It would be probably possible to change this node to use the topic remapping functionality, but that would break API for existing code. Although this is not a core ROS package, I follow the same guarantees regarding API stability - i.e. not breaking API in a released ROS version. So it's technically impossible to resolve this issue in the currently released versions. When we'll be doing a ROS 2 port of this package (if it's needed at all in ROS 2), we'll consider this security issue and try to write the code in such a way that it doesn't rely on parameters.