From 08a0eeb90cf8bb96e3d3507a7d724e04fde72503 Mon Sep 17 00:00:00 2001 From: Wilson Mo Date: Sun, 18 Aug 2019 22:23:52 +0800 Subject: [PATCH 1/2] Fix #3907 tar file placed outside of target location --- news/3907.bugfix | 1 + src/pip/_internal/utils/unpacking.py | 24 +++++++ tests/unit/test_utils_unpacking.py | 101 ++++++++++++++++++++++++++- 3 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 news/3907.bugfix diff --git a/news/3907.bugfix b/news/3907.bugfix new file mode 100644 index 00000000000..c61975e7bf1 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/src/pip/_internal/utils/unpacking.py b/src/pip/_internal/utils/unpacking.py index dcfe5819e5b..ed66187ae10 100644 --- a/src/pip/_internal/utils/unpacking.py +++ b/src/pip/_internal/utils/unpacking.py @@ -85,6 +85,18 @@ def has_leading_dir(paths): return True +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 unzip_file(filename, location, flatten=True): # type: (str, str, bool) -> None """ @@ -107,6 +119,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) @@ -166,6 +184,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(): diff --git a/tests/unit/test_utils_unpacking.py b/tests/unit/test_utils_unpacking.py index 96fcb99569f..40440fb7c27 100644 --- a/tests/unit/test_utils_unpacking.py +++ b/tests/unit/test_utils_unpacking.py @@ -2,10 +2,19 @@ import shutil import stat import sys +import tarfile import tempfile import time +import zipfile -from pip._internal.utils.unpacking import untar_file, unzip_file +import pytest + +from pip._internal.exceptions import InstallationError +from pip._internal.utils.unpacking import ( + is_within_directory, + untar_file, + unzip_file, +) class TestUnpackArchives(object): @@ -71,6 +80,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 @@ -90,3 +120,72 @@ def test_unpack_zip(self, data): test_file = data.packages.joinpath("test_zip.zip") 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) + + +@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 From 8c94b703540de03a42394c23f45d45bcea484c66 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Tue, 24 Sep 2019 22:43:06 -0400 Subject: [PATCH 2/2] Address review comments --- news/3907.bugfix | 3 +- src/pip/_internal/utils/unpacking.py | 16 ++++++----- tests/unit/test_utils_unpacking.py | 42 +++++++++++++--------------- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/news/3907.bugfix b/news/3907.bugfix index c61975e7bf1..24d711df482 100644 --- a/news/3907.bugfix +++ b/news/3907.bugfix @@ -1 +1,2 @@ -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. \ No newline at end of file +Abort installation if any archive contains a file which would be placed +outside the extraction location. diff --git a/src/pip/_internal/utils/unpacking.py b/src/pip/_internal/utils/unpacking.py index ed66187ae10..81a368f27ce 100644 --- a/src/pip/_internal/utils/unpacking.py +++ b/src/pip/_internal/utils/unpacking.py @@ -120,11 +120,11 @@ def unzip_file(filename, location, flatten=True): 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) + message = ( + 'The zip file ({}) has a file ({}) trying to install ' + 'outside target directory ({})' ) + raise InstallationError(message.format(filename, fn, location)) if fn.endswith('/') or fn.endswith('\\'): # A directory ensure_dir(fn) @@ -185,10 +185,12 @@ def untar_file(filename, location): fn = split_leading_dir(fn)[1] # type: ignore path = os.path.join(location, fn) if not is_within_directory(location, path): + message = ( + 'The tar file ({}) has a file ({}) trying to install ' + 'outside target directory ({})' + ) raise InstallationError( - 'The tar file (%s) has a file (%s) trying to install ' - 'outside target directory (%s)' % - (filename, path, location) + message.format(filename, path, location) ) if member.isdir(): ensure_dir(path) diff --git a/tests/unit/test_utils_unpacking.py b/tests/unit/test_utils_unpacking.py index 40440fb7c27..af9ae1c0e71 100644 --- a/tests/unit/test_utils_unpacking.py +++ b/tests/unit/test_utils_unpacking.py @@ -126,13 +126,11 @@ 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.*'): + files = ['regular_file.txt', os.path.join('..', 'outside_file.txt')] + test_zip = self.make_zip_file('test_zip.zip', files) + with pytest.raises(InstallationError) as e: unzip_file(test_zip, self.tempdir) + assert 'trying to install outside target directory' in str(e.value) def test_unpack_zip_success(self): """ @@ -140,11 +138,12 @@ def test_unpack_zip_success(self): 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')]) + files = [ + 'regular_file1.txt', + os.path.join('dir', 'dir_file1.txt'), + os.path.join('dir', '..', 'dir_file2.txt'), + ] + test_zip = self.make_zip_file('test_zip.zip', files) unzip_file(test_zip, self.tempdir) def test_unpack_tar_failure(self): @@ -152,13 +151,11 @@ 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.*'): + files = ['regular_file.txt', os.path.join('..', 'outside_file.txt')] + test_tar = self.make_tar_file('test_tar.tar', files) + with pytest.raises(InstallationError) as e: untar_file(test_tar, self.tempdir) + assert 'trying to install outside target directory' in str(e.value) def test_unpack_tar_success(self): """ @@ -166,11 +163,12 @@ def test_unpack_tar_success(self): 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')]) + files = [ + 'regular_file1.txt', + os.path.join('dir', 'dir_file1.txt'), + os.path.join('dir', '..', 'dir_file2.txt'), + ] + test_tar = self.make_tar_file('test_tar.tar', files) untar_file(test_tar, self.tempdir)