-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
user-misconfiguration may cause a heap-buffer-overflow bug in nav2_amcl #4157
Comments
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). |
no problem, I got it ^_^ |
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. |
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.
it seems feasible, is it in need for us to open a PR for it? |
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
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:
|
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. |
I spent some time studying the code in the ros2/rclcpp repository, and I noticed that the current code for |
The ‘ParameterDescriptor‘ has that! |
Bug report
Required Info:
Steps to reproduce issue
launch the navigation2 within defaulted
tb3_simulation_launch.py
as following commandsthere's only one difference between
my_nav2_params.yaml
and defaultednav2_params.yaml
,(we wrongly setamcl->ros_parameters->z_rand
as a very large value ), as following:then, just wait for the navigation2 launching.
Expected behavior
Processes should not crash, or at least processes should not result in heap buffer overflow, thereby providing the possibility for hackers to inject malicious code.
Actual behavior
Additional information
It's understood that "It is more a matter of time / inclination to set bounds on all N params in the stack (and make sure the dynamic parameter callbacks are respecting them too). " by us. Therefore, some bugs in navigation2 resulting from user-misconfiguration that lead to Denial of Service (DoS) behavior can actually be resolved by users themselves.
However, bugs like heap-buffer-overflow may pose security issues within system software. Attackers can exploit heap buffer overflow to overwrite function pointers or program counters, thereby injecting malicious code into the program for execution, escalating to more serious security concerns.
a suggestion that may be helpful:
z_rand
innav2_amcl
to avoid such heap-buffer-overflow bug?The text was updated successfully, but these errors were encountered: