Builds: prevent code injection in cwd (#8198)

This isn't a vulnerability since this command is executed inside
a container with a unprivileged user.

I have replaced the usage of manual `cd` with using the `workdir`
option from docker. This option doesn't support env vars expansion
(which is safer), so I had to introduce a new setting.

Author: stsewd

readthedocs/doc_builder/environments.py

@@ -75,7 +75,8 @@ class BuildCommand(BuildCommandResultMixin):
     :py:class:`readthedocs.builds.models.BuildCommandResult` model.
 
     :param command: string or array of command parameters
-    :param cwd: current working path for the command
+    :param cwd: Absolute path used as the current working path for the command.
+        Defaults to ``RTD_DOCKER_WORKDIR``.
     :param shell: execute command in shell, default=False
     :param environment: environment variables to add to environment
     :type environment: dict
@@ -102,7 +103,7 @@ def __init__(
     ):
         self.command = command
         self.shell = shell
-        self.cwd = cwd or '$HOME'
+        self.cwd = cwd or settings.RTD_DOCKER_WORKDIR
         self.user = user or settings.RTD_DOCKER_USER
         self.environment = environment.copy() if environment else {}
         if 'PATH' in self.environment:
@@ -152,9 +153,7 @@ def run(self):
             proc = subprocess.Popen(
                 command,
                 shell=self.shell,
-                # This is done here for local builds, but not for docker,
-                # as we want docker to expand inside the container
-                cwd=os.path.expandvars(self.cwd),
+                cwd=self.cwd,
                 stdin=None,
                 stdout=subprocess.PIPE,
                 stderr=subprocess.STDOUT,
@@ -271,6 +270,11 @@ class DockerBuildCommand(BuildCommand):
     Build command to execute in docker container
     """
 
+    bash_escape_re = re.compile(
+        r"([\t\ \!\"\#\$\&\'\(\)\*\:\;\<\>\?\@"
+        r'\[\\\]\^\`\{\|\}\~])'
+    )
+
     def __init__(self, *args, escape_command=True, **kwargs):
         """
         Override default to extend behavior.
@@ -300,6 +304,7 @@ def run(self):
                 cmd=self.get_wrapped_command(),
                 environment=self.environment,
                 user=self.user,
+                workdir=self.cwd,
                 stdout=True,
                 stderr=True,
             )
@@ -345,28 +350,28 @@ def get_wrapped_command(self):
         ``escape_command=True`` in the init method this escapes a good majority
         of those characters.
         """
-        bash_escape_re = re.compile(
-            r"([\t\ \!\"\#\$\&\'\(\)\*\:\;\<\>\?\@"
-            r'\[\\\]\^\`\{\|\}\~])',
-        )
         prefix = ''
         if self.bin_path:
-            prefix += 'PATH={}:$PATH '.format(self.bin_path)
+            bin_path = self._escape_command(self.bin_path)
+            prefix += f'PATH={bin_path}:$PATH '
 
         command = (
-            ' '.join([
-                bash_escape_re.sub(r'\\\1', part) if self.escape_command else part
+            ' '.join(
+                self._escape_command(part) if self.escape_command else part
                 for part in self.command
-            ])
+            )
         )
         return (
-            "/bin/sh -c 'cd {cwd} && {prefix}{cmd}'".format(
-                cwd=self.cwd,
+            "/bin/sh -c '{prefix}{cmd}'".format(
                 prefix=prefix,
                 cmd=command,
             )
         )
 
+    def _escape_command(self, cmd):
+        r"""Escape the command by prefixing suspicious chars with `\`."""
+        return self.bash_escape_re.sub(r'\\\1', cmd)
+
 
 class BaseEnvironment:
 

readthedocs/doc_builder/python_environments.py

@@ -323,7 +323,7 @@ def setup_base(self):
             # Don't use virtualenv bin that doesn't exist yet
             bin_path=None,
             # Don't use the project's root, some config files can interfere
-            cwd='$HOME',
+            cwd=None,
         )
 
     def install_core_requirements(self):

readthedocs/rtd_tests/tests/test_doc_building.py

@@ -13,7 +13,7 @@
 from unittest.mock import Mock, PropertyMock, mock_open, patch
 
 import pytest
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django_dynamic_fixture import get
 from docker.errors import APIError as DockerAPIError
 from docker.errors import DockerException
@@ -356,6 +356,7 @@ def test_failing_execution_with_unexpected_exception(self):
         })
 
 
+@override_settings(RTD_DOCKER_WORKDIR='/tmp/')
 class TestDockerBuildEnvironment(TestCase):
 
     """Test docker build environment."""
@@ -699,7 +700,8 @@ def test_command_execution(self):
 
         self.mocks.docker_client.exec_create.assert_called_with(
             container='build-123-project-6-pip',
-            cmd="/bin/sh -c 'cd /tmp && echo\\ test'",
+            cmd="/bin/sh -c 'echo\\ test'",
+            workdir='/tmp',
             environment=mock.ANY,
             user='docs:docs',
             stderr=True,
@@ -759,7 +761,8 @@ def test_command_not_recorded(self):
 
         self.mocks.docker_client.exec_create.assert_called_with(
             container='build-123-project-6-pip',
-            cmd="/bin/sh -c 'cd /tmp && echo\\ test'",
+            cmd="/bin/sh -c 'echo\\ test'",
+            workdir='/tmp',
             environment=mock.ANY,
             user='docs:docs',
             stderr=True,
@@ -806,7 +809,8 @@ def test_record_command_as_success(self):
 
         self.mocks.docker_client.exec_create.assert_called_with(
             container='build-123-project-6-pip',
-            cmd="/bin/sh -c 'cd /tmp && echo\\ test'",
+            cmd="/bin/sh -c 'echo\\ test'",
+            workdir='/tmp',
             environment=mock.ANY,
             user='docs:docs',
             stderr=True,
@@ -1010,6 +1014,7 @@ def test_container_timeout(self):
         })
 
 
+@override_settings(RTD_DOCKER_WORKDIR='/tmp/')
 class TestBuildCommand(TestCase):
 
     """Test build command creation."""
@@ -1090,6 +1095,7 @@ def test_unicode_output(self, mock_subprocess):
         )
 
 
+@override_settings(RTD_DOCKER_WORKDIR='/tmp/')
 class TestDockerBuildCommand(TestCase):
 
     """Test docker build commands."""
@@ -1109,7 +1115,7 @@ def test_wrapped_command(self):
         )
         self.assertEqual(
             cmd.get_wrapped_command(),
-            "/bin/sh -c 'cd /tmp/foobar && pip install requests'",
+            "/bin/sh -c 'pip install requests'",
         )
         cmd = DockerBuildCommand(
             ['python', '/tmp/foo/pip', 'install', 'Django>1.7'],
@@ -1120,7 +1126,7 @@ def test_wrapped_command(self):
             cmd.get_wrapped_command(),
             (
                 '/bin/sh -c '
-                "'cd /tmp/foobar && PATH=/tmp/foo:$PATH "
+                "'PATH=/tmp/foo:$PATH "
                 r"python /tmp/foo/pip install Django\>1.7'"
             ),
         )

readthedocs/settings/base.py

@@ -441,6 +441,7 @@ def TEMPLATES(self):
     # https://docs.docker.com/engine/reference/run/#user
     RTD_DOCKER_USER = 'docs:docs'
     RTD_DOCKER_SUPER_USER = 'root:root'
+    RTD_DOCKER_WORKDIR = '/home/docs/'
 
     RTD_DOCKER_COMPOSE = False