Fix pypa#3907 tar file placed outside of target location

news/3907.bugfix

@@ -0,0 +1 @@
+Abort the installation process and raise an exception if one of the tar/zip file will be placed outside of target location causing security issue.

src/pip/_internal/utils/misc.py

@@ -82,7 +82,8 @@ def cast(type_, value):  # type: ignore
            'unzip_file', 'untar_file', 'unpack_file', 'call_subprocess',
            'captured_stdout', 'ensure_dir',
            'ARCHIVE_EXTENSIONS', 'SUPPORTED_EXTENSIONS', 'WHEEL_EXTENSION',
-           'get_installed_version', 'remove_auth_from_url']
+           'get_installed_version', 'remove_auth_from_url',
+           'is_within_directory']
 
 
 logger = logging.getLogger(__name__)
@@ -334,6 +335,18 @@ def is_svn_page(html):
             re.search(r'Powered by (?:<a[^>]*?>)?Subversion', html, re.I))
 
 
+def is_within_directory(directory, target):
+    # type: ((Union[str, Text]), (Union[str, Text])) -> bool
+    """
+    Return true if the absolute path of target is within the directory
+    """
+    abs_directory = os.path.abspath(directory)
+    abs_target = os.path.abspath(target)
+
+    prefix = os.path.commonprefix([abs_directory, abs_target])
+    return prefix == abs_directory
+
+
 def file_contents(filename):
     # type: (str) -> Text
     with open(filename, 'rb') as fp:
@@ -628,6 +641,12 @@ def unzip_file(filename, location, flatten=True):
                 fn = split_leading_dir(name)[1]
             fn = os.path.join(location, fn)
             dir = os.path.dirname(fn)
+            if not is_within_directory(location, fn):
+                raise InstallationError(
+                    'The zip file (%s) has a file (%s) trying to install '
+                    'outside target directory (%s)' %
+                    (filename, fn, location)
+                )
             if fn.endswith('/') or fn.endswith('\\'):
                 # A directory
                 ensure_dir(fn)
@@ -687,6 +706,12 @@ def untar_file(filename, location):
                 # https://github.com/python/mypy/issues/1174
                 fn = split_leading_dir(fn)[1]  # type: ignore
             path = os.path.join(location, fn)
+            if not is_within_directory(location, path):
+                raise InstallationError(
+                    'The tar file (%s) has a file (%s) trying to install '
+                    'outside target directory (%s)' %
+                    (filename, path, location)
+                )
             if member.isdir():
                 ensure_dir(path)
             elif member.issym():

tests/unit/test_utils.py

@@ -11,9 +11,11 @@
 import shutil
 import stat
 import sys
+import tarfile
 import tempfile
 import time
 import warnings
+import zipfile
 from io import BytesIO
 from logging import DEBUG, ERROR, INFO, WARNING
 from textwrap import dedent
@@ -44,6 +46,7 @@
     format_command_args,
     get_installed_distributions,
     get_prog,
+    is_within_directory,
     make_subprocess_output_error,
     netloc_has_port,
     normalize_path,
@@ -360,6 +363,27 @@ def confirm_files(self):
                 "mode: %s, expected mode: %s" % (mode, expected_mode)
             )
 
+    def make_zip_file(self, filename, file_list):
+        """
+        Create a zip file for test case
+        """
+        test_zip = os.path.join(self.tempdir, filename)
+        with zipfile.ZipFile(test_zip, 'w') as myzip:
+            for item in file_list:
+                myzip.writestr(item, 'file content')
+        return test_zip
+
+    def make_tar_file(self, filename, file_list):
+        """
+        Create a tar file for test case
+        """
+        test_tar = os.path.join(self.tempdir, filename)
+        with tarfile.open(test_tar, 'w') as mytar:
+            for item in file_list:
+                file_tarinfo = tarfile.TarInfo(item)
+                mytar.addfile(file_tarinfo, 'file content')
+        return test_tar
+
     def test_unpack_tgz(self, data):
         """
         Test unpacking a *.tgz, and setting execute permissions
@@ -380,6 +404,58 @@ def test_unpack_zip(self, data):
         unzip_file(test_file, self.tempdir)
         self.confirm_files()
 
+    def test_unpack_zip_failure(self):
+        """
+        Test unpacking a *.zip with file containing .. path
+        and expect exception
+        """
+        test_zip = self.make_zip_file('test_zip.zip',
+                                      ['regular_file.txt',
+                                       os.path.join('..', 'outside_file.txt')])
+        with pytest.raises(
+                InstallationError,
+                match=r'.*trying to install outside target directory.*'):
+            unzip_file(test_zip, self.tempdir)
+
+    def test_unpack_zip_success(self):
+        """
+        Test unpacking a *.zip with regular files,
+        no file will be installed outside target directory after unpack
+        so no exception raised
+        """
+        test_zip = self.make_zip_file(
+            'test_zip.zip',
+            ['regular_file1.txt',
+             os.path.join('dir', 'dir_file1.txt'),
+             os.path.join('dir', '..', 'dir_file2.txt')])
+        unzip_file(test_zip, self.tempdir)
+
+    def test_unpack_tar_failure(self):
+        """
+        Test unpacking a *.tar with file containing .. path
+        and expect exception
+        """
+        test_tar = self.make_tar_file('test_tar.tar',
+                                      ['regular_file.txt',
+                                       os.path.join('..', 'outside_file.txt')])
+        with pytest.raises(
+                InstallationError,
+                match=r'.*trying to install outside target directory.*'):
+            untar_file(test_tar, self.tempdir)
+
+    def test_unpack_tar_success(self):
+        """
+        Test unpacking a *.tar with regular files,
+        no file will be installed outside target directory after unpack
+        so no exception raised
+        """
+        test_tar = self.make_tar_file(
+            'test_tar.tar',
+            ['regular_file1.txt',
+             os.path.join('dir', 'dir_file1.txt'),
+             os.path.join('dir', '..', 'dir_file2.txt')])
+        untar_file(test_tar, self.tempdir)
+
 
 class Failer:
     def __init__(self, duration=1):
@@ -1483,3 +1559,20 @@ def test_make_setuptools_shim_args__unbuffered_output(unbuffered_output):
         unbuffered_output=unbuffered_output
     )
     assert ('-u' in args) == unbuffered_output
+
+
+@pytest.mark.parametrize('args, expected', [
+    # Test the second containing the first.
+    (('parent/sub', 'parent/'), False),
+    # Test the first not ending in a trailing slash.
+    (('parent', 'parent/foo'), True),
+    # Test target containing `..` but still inside the parent.
+    (('parent/', 'parent/foo/../bar'), True),
+    # Test target within the parent
+    (('parent/', 'parent/sub'), True),
+    # Test target outside parent
+    (('parent/', 'parent/../sub'), False),
+])
+def test_is_within_directory(args, expected):
+    result = is_within_directory(*args)
+    assert result == expected