Skip to content
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

rtapi_app: Remove setuid? #362

Open
zultron opened this issue Jan 3, 2022 · 2 comments
Open

rtapi_app: Remove setuid? #362

zultron opened this issue Jan 3, 2022 · 2 comments

Comments

@zultron
Copy link
Contributor

zultron commented Jan 3, 2022

The rtapi_app executable is currently setuid root for the following reasons I can think of:

  • Permission to run with elevated scheduling priority and SCHED_FIFO in rt-preempt threads
  • Permission for HAL comps to access hardware (not sure on specifics)
  • Possibly, permission for HAL comps to access low-level networking, like hm2-eth

This is causing problems with the ROS2 port of hal_ros_control. The main issues are described in tormach/hal_ros_control#8:

For security reasons, setuid executables ignore $LD_LIBRARY_PATH. The ROS 1 version can work around this by adding /opt/ros/${ROS_DISTRO}/lib to ld.so.conf. However, it appears that ROS2 rcl searches through $LD_LIBRARY_PATH on its own to find librmw_fastrtps_cpp.so and related libs. The workaround is to reconstruct $LD_LIBRARY_PATH in rtapi_app_main().

Another issue stemming from setuid is fixed in the next PR (where launch files are updated): FastDDS uses shm-based discovery by default, and files written by the setuid node in /dev/shm can't be read by other nodes. The workaround is to configure UDP discovery for the setuid node; see eProsima/Fast-DDS#1750

A better way to get these privileges in rtapi_app would be with POSIX capabilities, e.g. using libcap. This issue is to open up dialog to identify the privileges needed both by rtapi_app itself and by comps (whether included in this distribution or not), and to figure out the best way of setting them, whether by using setcap on the rtapi_app executable or by configuring pam_cap for users/groups or some other means.

@zultron
Copy link
Contributor Author

zultron commented Jan 3, 2022

Expanding the scope of this issue, another possibility is to rework rtapi_app to move its main functionality into a shared lib so that downstream projects can trivially build their own rtapi_app with whatever settings on the executable they want.

This would also open up the way to enabling derived subclasses of the main rtapi_app class, where it could be tailored for a specific application. Continuing the ROS example, it would be great to have a custom rtapi_app with built-in ros2_control controller manager, logging through ROS rclcpp facilities, reading settings through ROS parameters, etc. Same with rtapi_msgd.

@cerna
Copy link
Contributor

cerna commented Jan 4, 2022

I am a big fan of executables being just a thin shim around underlying libraries with CLI slapped on top of it. So I am all in about implementing the loading functionality into its own library. Particularly with connection to #346. From the managing point of view (the ROS example), there is direct coupling of rtapi_app, msgd, halcmd and the realtime instrumentation script. The main process - rtapi_app - is not protected from murder via the Machinetalk attack, its full of hard assertions and has no exception handling for when it dies, which is not good for something directing movement of big mass. The multiple instance functionality is hard to use and the configuration as is is full of terrible atrocities, lately my own in runtime_config vs runtime_config_binary_tree.

I think it could take an inspiration in how the high level tools like docker are using low level tools like runC (in addition to the ROS2 controller manager to keep from the too deep lock-in possibility.)

However, this all sounds like a parallel problem to the setuid one. I am probably not the one to lobby for small changes, but I do think this has a big possibility of death by thousand paper cuts.

The setting of setuid bit seems like a shotgun approach similar to what includes (header files) used to be and there even is (or was) mention in the code to rework it to use the libcap and standard Linux Capabilities. I know, because I looked into it, but the gain-to-pain ratio was not good for me back then. So I am also excited about this change.

But for this to work, I think there needs to be a new metadata system passing between HAL modules, loader and the main process (rtapi_app one). HAL modules need to know which capabilities they need, the loader needs to scan for it and check against capabilities the rtapi_app was started and based on that decide if module loading/instantiation is possible.

And it is not just rtapi_app - currently the setuid target build these TARGETs:

sudo make setuid

Consolidate compiler generated dependencies of target syslog_async
[  0%] Built target syslog_async
[  0%] Checking the git repository for changes...
[  0%] Built target check_git
Consolidate compiler generated dependencies of target runtime_config
[  0%] Built target runtime_config
Consolidate compiler generated dependencies of target runtime_memory_api
[  0%] Built target runtime_memory_api
Consolidate compiler generated dependencies of target mkini
[  0%] Built target mkini
Consolidate compiler generated dependencies of target nanopb_proto_cc
[  0%] Built target nanopb_proto_cc
Consolidate compiler generated dependencies of target protobuf-nanopb-static
[  0%] Built target protobuf-nanopb-static
Consolidate compiler generated dependencies of target machinetalk_proto_cc
[ 40%] Built target machinetalk_proto_cc
[ 40%] Built target rtapi_pci_api
Consolidate compiler generated dependencies of target unmanaged_rtapi_pci
[ 40%] Built target unmanaged_rtapi_pci
Consolidate compiler generated dependencies of target unmanaged_runtime
[ 60%] Built target unmanaged_runtime
Consolidate compiler generated dependencies of target unmanaged_hal
[ 80%] Built target unmanaged_hal
Consolidate compiler generated dependencies of target machinetalk
[ 80%] Built target machinetalk
Consolidate compiler generated dependencies of target rtapi_app
[ 80%] Built target rtapi_app
[ 80%] Setting the SETUID rules on target 'rtapi_app'.
[ 80%] Built target rtapi_app_setuid
Consolidate compiler generated dependencies of target upci
[ 80%] Built target upci
Consolidate compiler generated dependencies of target pci_read
[100%] Built target pci_read
[100%] Setting the SETUID rules on target 'pci_read'.
[100%] Built target pci_read_setuid
Consolidate compiler generated dependencies of target pci_write
[100%] Built target pci_write
[100%] Setting the SETUID rules on target 'pci_write'.
[100%] Built target pci_write_setuid
[100%] Built target setuid

(Basically, there are rtapi_app, pci_read and pci_write executables with setuid. In future there may be more [or less]. Any new solution probably should take the additional executables into account.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants