Validate messages in subscription callbacks for critical error in population about navigation2 HOT 6 OPEN

some suggestions to avoid such buffer-overflow:

• just use the real size of vector as the boundary
• design nsg-recovery mechanism for /map ?

from navigation2.

It is usually expected that inputs are well formed - but even in writing this answer I find myself disagreeing with myself that maybe we should do some basic message validation to prevent and/or expose potential problems before users run into them.

My suggestion would be to audit the input information to Nav2 (occupancy grid messages, pointclouds, laser scans, etc) to get a list of all message types that come in. Then, we can look at them to see what places we need to do validation checks to make sure the message won't bring down the system (ie map size and actual data don't match; pointcloud strides and its data size don't match; unimplemented or invalid values in LaserScan angles, etc). At that point, we can make a new nav2_util/validate_messages.hpp that contains these validation methods and use those methods in all the callbacks to reject updates if they're invalid.

Basically change all callbacks to first thing do

{
  if (!nav2_util::validateMsg(msg)) {
    RCLCPP_ERROR(logger_, "Message came in at time X of type Y that is malformed. Rejecting.");
    return;
  }
}

Or, even more interestingly, we could use the topic tools in ROS 2 to create a message filter that the filter does this action so that all callbacks when triggered have already been pre-validated. There's examples of this using TF transforms, but we can make arbitrary filters.

from navigation2.

Interesting suggestion! ^_^

I agree that basic message validation is in need, especially considering the exposure of ros2-topics within the local network, where they are susceptible to both intentional and unintentional interference (in my opinion.

Your proposed solution indeed sounds great, while I must admit that direct implementation of nav2_util/validate_messages.hpp might be challenging for me. However, if there's already a basic framework of nav2_util::validateMsg(msg) in place, I'd be more than willing to contribute to refining its implementation details.

from navigation2.

It should just be a set of header files

bool validateMessage(const MyMessageType & msg) 
{
  // check the msg fields to make sure reasonably implemented
}

It should be very straight forward. I won't have time for this and since this is something important to you, I suggest taking the lead on it. The checks are simple: sizes match necessary sizes, no INF or NAN in key fields. It doesn't need to check quality, just anything that would cause a segfault or NAN where it isn't expected

from navigation2.

May I open a long-term PR after implementing a basic framework, to continually update it whenever I come across new tickets?

Alternatively, should I open a PR dedicated solely to fixing this issue after implementing the framework, and then open new PRs to address any new input validation issues that arise?

I want to choose one of them that minimizes disruption to you while still being effective. ^_^

from navigation2.

That sounds like a plan:

• Add the new nav2_util header with the map validator
• Use that map validator in a subscription filter or at the start of the callback where relevant
• Over time, as you find issues in your fuzzing experiments, add additional validation utils in the appropriate spots

The only thing I'd ask is that you make sure to setup for all subscriptions of a given type when you add it in one place (ie all occ grid subscribers) so that we don't have some where its done and some where its not. But, I think that perfectly aligns with your intent / need so I don't think you'd have an issue with that :-)

from navigation2.