Fix unsafe yaml load on dynparam (#202)

Co-authored-by: Isaac Saito <[email protected]>

Author: florcabral

package.xml

@@ -37,4 +37,6 @@
   <exec_depend>roslib</exec_depend>
   <exec_depend>rospy</exec_depend>
   <exec_depend>rosservice</exec_depend>
+
+  <test_depend>rosbash</test_depend>
 </package>

scripts/dynparam

@@ -110,7 +110,7 @@ Examples:
     node = args[0]
     if len(args) == 2:
         node, value = args[0], args[1]
-        values_dict = yaml.load(value, Loader=yaml.Loader)
+        values_dict = yaml.safe_load(value)
         if type(values_dict) != dict:
             parser.error('invalid arguments. Please specify either a node name, parameter name and parameter value, or a node name and a YAML dictionary')
     elif len(args) == 3:
@@ -159,7 +159,7 @@ def do_load():
     f = open(path, 'r')
     try:
         params = {}
-        for doc in yaml.load_all(f.read(), Loader=yaml.Loader):
+        for doc in yaml.safe_load_all(f.read()):
             params.update(doc)
     finally:
         f.close()

test/CMakeLists.txt

@@ -17,3 +17,5 @@ target_link_libraries(dynamic_reconfigure-test_client dynamic_reconfigure_config
 add_dependencies(tests dynamic_reconfigure-test_client)
 
 add_rostest(test_python_simple_client.launch)
+
+add_rostest(test_dynparam.launch)

test/test_dynparam.launch

@@ -0,0 +1,5 @@
+<launch>
+  <node pkg="dynamic_reconfigure" type="dynamic_reconfigure-ref_server" name="ref_server" output="screen" />
+
+  <test test-name="test_dynparam" pkg="dynamic_reconfigure" type="test_dynparam.py" />
+</launch>

test/test_dynparam.py

@@ -0,0 +1,30 @@
+#!/usr/bin/env python
+
+import unittest
+import rostest
+import rospy
+import os
+import subprocess
+
+
+class TestDynparam(unittest.TestCase):
+
+    def test_dynparam(self):
+        unsafe_yaml_file = os.path.join(os.path.abspath(os.path.join(os.path.dirname(__file__))), 'test_unsafe_yaml.yaml')
+        retcode_unsafe_yaml = 0
+
+        try:
+            # run dynparam load with insecure yaml
+            output = subprocess.check_output(['rosrun dynamic_reconfigure dynparam load ref_server ' + unsafe_yaml_file], stderr=subprocess.STDOUT, shell=True, executable="/bin/bash")
+        except subprocess.CalledProcessError as e:
+            self.assertIn('could not determine a constructor for the tag'.encode(), e.output) # check the right error is being thrown
+            retcode_unsafe_yaml = e.returncode
+
+        # check the test failed
+        self.assertNotEqual(retcode_unsafe_yaml, 0)
+
+
+if __name__ == '__main__':
+    rospy.init_node('dynparam_test')
+    rospy.sleep(3.0)
+    rostest.rosrun('dynamic_reconfigure', 'test_dynparam', TestDynparam)

test/test_unsafe_yaml.yaml

@@ -0,0 +1,2 @@
+# When the line below gets read by unsafe yaml loader (e.g. `yaml.load`), the bash code may get executed, which is a huge security risk.
+str_: !!python/object/apply:os.system ["cat /etc/passwd"]