swri-robotics / marti_common

Common utility functions for MARTI
BSD 3-Clause "New" or "Revised" License
53 stars 62 forks source link

Potential vulnerability: topic names from ROS parameters #700

Closed tandf closed 1 year ago

tandf commented 1 year ago

Hi,

We notice that you are using topic names from ROS parameters at the following locations: https://github.com/swri-robotics/marti_common/blob/dcdab5002de03ea62cdfac350ee09d3df18e4951/swri_transform_util/nodes/initialize_origin.py#L104 https://github.com/swri-robotics/marti_common/blob/dcdab5002de03ea62cdfac350ee09d3df18e4951/swri_transform_util/nodes/initialize_origin.py#L106 https://github.com/swri-robotics/marti_common/blob/dcdab5002de03ea62cdfac350ee09d3df18e4951/swri_transform_util/nodes/initialize_origin.py#L110 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). If the initialize_origin/local_xy_gpsfix_topic, initialize_origin/local_xy_navsatfix_topic, and initialize_origin/local_xy_custom_topic parameters are changed, the initialize_origin node will not be able to locate the origin automatically. Moreover, if an attacker exists, she can even manipulate the origin by first changing the parameters to for example /gps_fake, and then forward the messages from topic /gps to /gps_fake after modifying the contents. 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/.

danthony06 commented 1 year ago

I understand what you are saying, but I don't think there is a practical means of solving this problem because the ROS 1 and the parameter server is insecure due to their design. If a malicious attacker has the ability to access the parameter server, there's a whole host of things that would be more straightforward attacks. Even if we were to implement this functionality using topic remapping, there's no way of stopping a malicious actor from publishing a bad value on any of these topics, and breaking these system.

Secure-ROS provides a partial mechanism for mitigating these types of attacks, which doesn't require modifying this code. SROS2 has even more complete security model which should make these types of attacks impossible in a properly configured system.