diff --git a/news/6413.bugfix b/news/6413.bugfix new file mode 100644 index 00000000000..68d0a72f64a --- /dev/null +++ b/news/6413.bugfix @@ -0,0 +1,3 @@ +Prevent ``pip install `` from permitting directory traversal if e.g. +a malicious server sends a ``Content-Disposition`` header with a filename +containing ``../`` or ``..\\``. diff --git a/src/pip/_internal/download.py b/src/pip/_internal/download.py index c98fae5d330..6a54f89400f 100644 --- a/src/pip/_internal/download.py +++ b/src/pip/_internal/download.py @@ -66,7 +66,8 @@ 'is_url', 'url_to_path', 'path_to_url', 'is_archive_file', 'unpack_vcs_link', 'unpack_file_url', 'is_vcs_url', 'is_file_url', - 'unpack_http_url', 'unpack_url'] + 'unpack_http_url', 'unpack_url', + 'parse_content_disposition', 'sanitize_content_filename'] logger = logging.getLogger(__name__) @@ -1050,6 +1051,29 @@ def unpack_url( write_delete_marker_file(location) +def sanitize_content_filename(filename): + # type: (str) -> str + """ + Sanitize the "filename" value from a Content-Disposition header. + """ + return os.path.basename(filename) + + +def parse_content_disposition(content_disposition, default_filename): + # type: (str, str) -> str + """ + Parse the "filename" value from a Content-Disposition header, and + return the default filename if the result is empty. + """ + _type, params = cgi.parse_header(content_disposition) + filename = params.get('filename') + if filename: + # We need to sanitize the filename to prevent directory traversal + # in case the filename contains ".." path parts. + filename = sanitize_content_filename(filename) + return filename or default_filename + + def _download_http_url( link, # type: Link session, # type: PipSession @@ -1097,10 +1121,7 @@ def _download_http_url( # Have a look at the Content-Disposition header for a better guess content_disposition = resp.headers.get('content-disposition') if content_disposition: - type, params = cgi.parse_header(content_disposition) - # We use ``or`` here because we don't want to use an "empty" value - # from the filename param. - filename = params.get('filename') or filename + filename = parse_content_disposition(content_disposition, filename) ext = splitext(filename)[1] if not ext: ext = mimetypes.guess_extension(content_type) diff --git a/tests/unit/test_download.py b/tests/unit/test_download.py index 438ebcb2e10..7baee5e04b4 100644 --- a/tests/unit/test_download.py +++ b/tests/unit/test_download.py @@ -12,6 +12,7 @@ import pip from pip._internal.download import ( CI_ENVIRONMENT_VARIABLES, MultiDomainBasicAuth, PipSession, SafeFileCache, + _download_http_url, parse_content_disposition, sanitize_content_filename, unpack_file_url, unpack_http_url, url_to_path, ) from pip._internal.exceptions import HashMismatch @@ -199,6 +200,90 @@ def test_unpack_http_url_bad_downloaded_checksum(mock_unpack_file): rmtree(download_dir) +@pytest.mark.parametrize("filename, expected", [ + ('dir/file', 'file'), + ('../file', 'file'), + ('../../file', 'file'), + ('../', ''), + ('../..', '..'), + ('/', ''), +]) +def test_sanitize_content_filename(filename, expected): + """ + Test inputs where the result is the same for Windows and non-Windows. + """ + assert sanitize_content_filename(filename) == expected + + +@pytest.mark.parametrize("filename, win_expected, non_win_expected", [ + ('dir\\file', 'file', 'dir\\file'), + ('..\\file', 'file', '..\\file'), + ('..\\..\\file', 'file', '..\\..\\file'), + ('..\\', '', '..\\'), + ('..\\..', '..', '..\\..'), + ('\\', '', '\\'), +]) +def test_sanitize_content_filename__platform_dependent( + filename, + win_expected, + non_win_expected +): + """ + Test inputs where the result is different for Windows and non-Windows. + """ + if sys.platform == 'win32': + expected = win_expected + else: + expected = non_win_expected + assert sanitize_content_filename(filename) == expected + + +@pytest.mark.parametrize("content_disposition, default_filename, expected", [ + ('attachment;filename="../file"', 'df', 'file'), +]) +def test_parse_content_disposition( + content_disposition, + default_filename, + expected +): + actual = parse_content_disposition(content_disposition, default_filename) + assert actual == expected + + +def test_download_http_url__no_directory_traversal(tmpdir): + """ + Test that directory traversal doesn't happen on download when the + Content-Disposition header contains a filename with a ".." path part. + """ + mock_url = 'http://www.example.com/whatever.tgz' + contents = b'downloaded' + link = Link(mock_url) + + session = Mock() + resp = MockResponse(contents) + resp.url = mock_url + resp.headers = { + # Set the content-type to a random value to prevent + # mimetypes.guess_extension from guessing the extension. + 'content-type': 'random', + 'content-disposition': 'attachment;filename="../out_dir_file"' + } + session.get.return_value = resp + + download_dir = tmpdir.join('download') + os.mkdir(download_dir) + file_path, content_type = _download_http_url( + link, + session, + download_dir, + hashes=None, + progress_bar='on', + ) + # The file should be downloaded to download_dir. + actual = os.listdir(download_dir) + assert actual == ['out_dir_file'] + + @pytest.mark.parametrize("url,win_expected,non_win_expected", [ ('file:tmp', 'tmp', 'tmp'), ('file:c:/path/to/file', r'C:\path\to\file', 'c:/path/to/file'),