Add a check if the second argument of logging macros is a string literal

[shumov-ag]

Signed-off-by: Artem Shumov agshumov@sberautotech.ru

This PR prevents non string literal from being passed on to RCLCPP_@(severity) macros

[aprotyas]

Can you add a test case for this? Edit: of course, static asserts are a build-time construct.

[shumov-ag]

@aprotyas. Some build failure tests already exist (but disabled). I used a similar way to make the test

[tylerjw]

I don't understand, what is the upside to this charge? Is there something wrong with using a not string literal with the macros?

[shumov-ag]

I don't understand, what is the upside to this charge? Is there something wrong with using a not string literal with the macros?

https://docs.ros.org/en/foxy/Releases/Release-Galactic-Geochelone.html#change-in-rclcpp-s-logging-macros

[tylerjw]

I don't understand how this fixes the ability to use string parsing to execute code as you still allow "%s" but I understand the reasoning at least. Some day it would be nice to migrate away from these macros to something more modern... Maybe something that uses fmt and sbdlog directly as there is only one logging back end anyway. Using fmt (or std::format from 20) would fix the security issue and enable a ton of nice functionality.

[tylerjw]

Another option would be to constrain the input to the string_view type and drop all the c-style formatting. std::string and char* are both convertible to string_view and if a user wants to do c-style formatting which has this problem, they can do that on their own.

[clalancette]

I don't understand how this fixes the ability to use string parsing to execute code as you still allow "%s" but I understand the reasoning at least.

I think https://owasp.org/www-community/attacks/Format_string_attack explains it pretty well, but in my understanding the problem is that if you pass a raw string in (like fprintf("foo %s")), then you have no guarantees that there are enough format arguments to support that %s. Since that string is attacker controlled, they can make the fprintf implementation go whizzing off through memory, looking for a sentinel \0 (or similar such nonsense with other format parameters). That in turn can lead to bad things happening. On the other hand, if you always do fprintf("%s", "foo %s"), then you are guaranteed that you have a properly-terminated string, even if it may not be exactly what you were expecting. So I think the need for it is pretty clear.

Another option would be to constrain the input to the string_view type and drop all the c-style formatting. std::string and char* are both convertible to string_view and if a user wants to do c-style formatting which has this problem, they can do that on their own.

I would be all for adding options using either string_view or fmt (once we move to C++20), but I don't think we can reasonably take away the existing macros, at least in the short-to-medium term. There is far too much code that depends on them.

[tylerjw]

I think it is helpful to have examples to talk about. How does this stop a user from doing this: https://godbolt.org/z/sKGEPcYr3

You are trying to protect users who log things that they might get from somewhere else. But that doesn't fix the problem of the user forgetting arguments and giving you a "%s" or something similar.

[shumov-ag]

But that doesn't fix the problem of the user forgetting arguments and giving you a "%s" or something similar.

Gcc like compilers have __attribute__ ((format)) to check for this. Logging macros use a function rcutils_log() and it is declared with this attribute

This PR is just a protection against using:

std::string s = "some string"
RCLCPP_DEBUG(logger, s.c_str())

[clalancette]

You are trying to protect users who log things that they might get from somewhere else. But that doesn't fix the problem of the user forgetting arguments and giving you a "%s" or something similar.

As @shumov-ag said, that will generally give you a compiler warning. Using the code in your link, and compiling with g++ will give you:

foo.cpp: In function ‘int main()’:
foo.cpp:9:9: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=]
    9 |     LOG("%s");
      |         ^~~~
foo.cpp:3:25: note: in definition of macro ‘LOG’
    3 | #define LOG(...) printf(__VA_ARGS__)
      |                         ^~~~~~~~~~~
foo.cpp:9:11: note: format string is defined here
    9 |     LOG("%s");
      |          ~^
      |           |
      |           char*

[tylerjw]

Thank you for the detailed explanations. This change looks like it does what is intended. I'm sorry if my questions were annoying.

[jacobperron]

While I think this is a good change to make, perhaps we consider making this a deprecation warning instead of a compiler error?This would avoid outright breaking downstream code.

Thoughts?

[tylerjw]

I much prefer a compiler error to a surprise runtime error.

[fujitatomoya]

I much prefer a compiler error to a surprise runtime error.

agree.

a deprecation warning instead of a compiler error?

i understand this is what we usually do. but since this security issue, probably it could be different?

[clalancette]

The thing is, this should already a warning to users; they'll get the equivalent of what I put in #1918 (comment) (even if they don't have -Wall enabled). This is escalating it from that warning to an outright compilation failure.

With that in mind, I first rebuilt a local ROS 2 core workspace with this patch in place. Then I went and downloaded all of the packages currently released into Rolling (rosinstall_generator --repos ALL --rosdistro rolling --deps | vcs import src). I then built all of them in a single giant workspace. Various ones failed for various reasons, but I did see a handful of failures because of this new compile-time error. In particular, the following packages failed to build:

• compressed_depth_image_transport
• rviz_visual_tools
• controller_manager
• rmf_traffic_ros2
• rcss3d_agent
• nao_lola
• teleop_twist_joy
• diagnostic_aggregator
• gazebo_plugins
• laser_proc

(note that there may be more hidden behind these; if any package fails, all things that depend on it are skipped as well so there may be other errors lurking)

Before we put this change in, I think we should consider fixing up the failing parts of the ecosystem. That will make it much easier for this to go in without breaking things downstream. I'll at least start with teleop_twist_joy, since that is one that I maintain.

[jacobperron]

I'm a bit confused. Currently, I do not see a warning when trying to compile the following:

    std::string log_message = "Created publisher";
    RCLCPP_INFO(this->get_logger(), log_message.c_str());

I'm pretty sure -Wall is set in the CMakeLists.txt too. So, from my perspective the change proposed in this PR goes from no warning to an build error directly.

I've pushed the complete example to a branch here: https://github.com/ros2/examples/blob/jacob/repro_rclcpp_1918/rclcpp/topics/minimal_publisher/lambda.cpp

Built with:

colcon build --event-handlers console_direct+ --packages-select examples_rclcpp_minimal_publisher

[jacobperron]

Also, what I am proposing is introducing a build-time warning (if feasible), not necessarily a runtime warning.

[clalancette]

I'm also confused. I know for a fact that in the past, we had warnings on things like:

    std::string log_message = "Created publisher";
    RCLCPP_INFO(this->get_logger(), log_message.c_str());

(we have PRs, like #1442 , where we fixed these and turned on additional warning). But like you, I can't actually make this warning happen currently, and I'm not sure why. Possibly we need to do some additional annotation on the macros to make this happen, and that might actually accomplish what you are asking for; making this a warning first.