user-misconfiguration may cause a heap-buffer-overflow bug in nav2_amcl about navigation2 HOT 8 CLOSED

That's an insane number, I wouldn't expect anything else to happen.

Added to the #4005 - I think we discussed last time its a low-priority task to protect every parameter's potential reasonable bounds, since that would take weeks and you're only finding these by setting insane values. If you find more in the future, feel free to append to your ticket (#4005).

from navigation2.

feel free to append to your ticket(#4005)

no problem, I got it ^_^

from navigation2.

I will say that if this interests you to address, something I thought of was adding a Nav2 util eq of ‘get_parameter’ whos arguments can include min-max ranges for ints & doubles and throws if not met. That would be a relatively time consuming, but feasible for someone that wants it, action.

Then the hard work comes to populating all those ranges. I figure one node at a time, once per week is a good way to break it into managable chunks.

Dynamic parameter updates are another issue. Perhaps we could refactor the params for all nodes/servers into their own param handler util (some have already) so all that range checking is handled in a separate file to keep the algorithm implementations cleaner. If we wanted to go even a bridge further, we could change all member params into rclcpp::Parameters that have valid ranges in their descriptions.

There’s actually not a shortage of good technical solutions. Its simply the scale that is daunting. But if that was important to you, I’d be happy to help flush out your design and then knock out the packages one at a time over a couple of months.

from navigation2.

We have some concerns about such security issue, while it is also a relatively low priority issue for us. Our submission of such issues mainly aims to provide a reminder and record of such issue for the navigation2 stack. Should we discover similar parameter issues in the future, we will add them directly to the ticket #4005 rather than open a new issue.

adding a Nav2 util eq of ‘get_parameter’ whos arguments can include min-max ranges for ints & doubles and throws if not met

it seems feasible, is it in need for us to open a PR for it?

from navigation2.

I try not do to do things half baked, since there may be some functional problems that require reworking the problem, but I was thinking something like

rclcpp::Parameter get_parameter<TypeT>(Node & node, string param_name, TypeT min, TypeT max)
{
  rclcpp::Parameter param;
  if (node->get_parameter(param_name, param)) {
    if (param >= min && min <= max) {
      return param;
    }
    // throw exception, out of bounds 
  }
  // throw exception, did not obtain
}

Then the TypeT of this function have overrides for the different versions. The double/int use something like this, but the string/bool just do the normal get parameter logic. It would be expanded with the double/int arrays to also check those parameters' values in the array AND also check the array size.

A couple of open questions:

• Which rclcpp get_parameters API makes most sense: the bool check or let it throw itself one if it doesn't exist?
• Even if we replace inline with this everywhere, how do we want to handle the dynamic parameters callback function

from navigation2.

I try not do to do things half baked

So do I, while It seems I need some time to delve deeper into this section of code and the API. I'd come back here to discuss further once I have more matured ideas.

from navigation2.

I spent some time studying the code in the ros2/rclcpp repository, and I noticed that the current code for get_parameter() in rclcpp doesn't seem to provide an interface for setting valid boundaries such as maximum and minimum values. Do you suggest that I propose such a design to rclcpp first and then apply it to nav2 later?

from navigation2.

The ‘ParameterDescriptor‘ has that!

from navigation2.