Disable rsh synchronize rsync_opts

This change prevents executor host command execution through a
malicious rsh synchronize rsync_opts.

Fixes bug #2006526

Change-Id: I3cd17ca91410394f164d8ea7cd91a1ea5890f998

releasenotes/notes/synchronize-rsh-553872cf5422acf9.yaml

@@ -0,0 +1,6 @@
+---
+security:
+  - |
+    The synchronize rsh rsync_opts is now prohibited for security reasons
+    as it could be used to circumvent executor host command execution
+    restrictions.

tests/fixtures/config/remote-action-modules/git/org_project/playbooks/synchronize-good.yaml

@@ -16,6 +16,8 @@
         mode: pull
         src: "{{ destdir.path }}/"
         verify_host: true
+      environment:
+        OK: "we can use custom env"
 
     - name: Validate push
       stat:
@@ -39,6 +41,8 @@
         dest: "{{ destdir.path }}/pull/"
         mode: push
         src: "{{ zuul.executor.log_root }}/push"
+        rsync_opts:
+          - "--safe-links"
         verify_host: true
 
     - name: Validate pull

tests/fixtures/config/remote-action-modules/git/org_project/playbooks/synchronize-rsh-bad.yaml

@@ -0,0 +1,8 @@
+- hosts: all
+  tasks:
+    - synchronize:
+        rsync_opts:
+          - "--rsh={{ zuul.executor.log_root }}/oops"
+        src: "/proc/cmdline"
+        dest: "{{ zuul.executor.log_root }}/noop"
+        mode: pull

tests/fixtures/config/remote-action-modules/git/org_project/playbooks/synchronize-rsh-env-bad.yaml

@@ -0,0 +1,8 @@
+- hosts: all
+  tasks:
+    - synchronize:
+        src: "/proc/cmdline"
+        dest: "{{ zuul.executor.log_root }}/noop"
+        mode: pull
+      environment:
+        RSYNC_RSH: "{{ zuul.executor.log_root }}/oops"

tests/remote/test_remote_action_modules.py

@@ -20,6 +20,7 @@ from tests.base import AnsibleZuulTestCase, FIXTURE_DIR
 ERROR_ACCESS_OUTSIDE = "Accessing files from outside the working dir"
 ERROR_SYNC_TO_OUTSIDE = "Syncing files to outside the working dir"
 ERROR_SYNC_FROM_OUTSIDE = "Syncing files from outside the working dir"
+ERROR_SYNC_RSH = "Using custom synchronize rsh is prohibited"
 
 
 class TestActionModules25(AnsibleZuulTestCase):
@@ -96,6 +97,10 @@ class TestActionModules25(AnsibleZuulTestCase):
         self._run_job('assemble-bad-dir-with-symlink', 'FAILURE',
                       ERROR_ACCESS_OUTSIDE)
 
+    def test_synchronize_rsh_fail(self):
+        self._run_job('synchronize-rsh-bad', 'FAILURE', ERROR_SYNC_RSH)
+        self._run_job('synchronize-rsh-env-bad', 'FAILURE', ERROR_SYNC_RSH)
+
     def test_command_module(self):
         self._run_job('command-good', 'SUCCESS')
 

zuul/ansible/base/action/synchronize.py

@@ -18,6 +18,26 @@ from zuul.ansible import paths
 synchronize = paths._import_ansible_action_plugin("synchronize")
 
 
+def is_opt_prohibited(rsync_arg):
+    prohibited_opts = (
+        "--rsh",
+        "-e",
+    )
+    return any(filter(lambda opt: rsync_arg.startswith(opt), prohibited_opts))
+
+
+def is_env_prohibited(env_keys):
+    prohibited_env = (
+        "RSYNC_RSH",
+    )
+    return any(filter(lambda env: env in prohibited_env, env_keys))
+
+
+def is_prohibited(rsync_opts, environment):
+    return (any(filter(is_opt_prohibited, list(map(str.strip, rsync_opts)))) or
+            any(filter(is_env_prohibited, list(map(dict.keys, environment)))))
+
+
 class ActionModule(synchronize.ActionModule):
 
     def run(self, tmp=None, task_vars=None):
@@ -40,6 +60,12 @@ class ActionModule(synchronize.ActionModule):
             self._task.args['rsync_opts'] = []
         if '--safe-links' not in self._task.args['rsync_opts']:
             self._task.args['rsync_opts'].append('--safe-links')
+        if is_prohibited(
+                self._task.args.get('rsync_opts', []),
+                self._task.environment if self._task.environment else {}):
+            return dict(
+                failed=True,
+                msg="Using custom synchronize rsh is prohibited")
 
         if mode == 'push' and not paths._is_safe_path(
                 source, allow_trusted=True):