CVE-2019-20916: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: The pip package before 19.2 for Python allows Directory Traversal when a URL is given in an install command, because a Content-Disposition header can have ../ in a filename, as demonstrated by overwriting the /root/.ssh/authorized_keys file. This occurs in _download_http_url in _internal/download.py.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.5
Published: 2020-09-04T20:15Z
Repository: https://github.com/gzpan123/pip
Language: Python
Files changed: news/6413.bugfix, src/pip/_internal/download.py, tests/unit/test_download.py
Commit date: 2019-06-11 00:22:29 -0700
Commit message: FIX #6413 pip install <url> allow directory traversal

Diff: diff --git a/news/6413.bugfix b/news/6413.bugfix
new file mode 100644
index 00000000..68d0a72f
--- /dev/null
+++ b/news/6413.bugfix
@@ -0,0 +1,3 @@
+Prevent ``pip install <url>`` 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 c98fae5d..6a54f894 100644
--- a/src/pip/_internal/download.py
+++ b/src/pip/_internal/download.py
@@ -64,11 +64,12 @@ HAS_TLS = (ssl is not None) or IS_PYOPENSSL
 
 __all__ = ['get_file_content',
            '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__)
 
 
@@ -1048,10 +1049,33 @@ def unpack_url(
         )
     if only_download:
         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
     temp_dir,  # type: str
     hashes,  # type: Hashes
@@ -1095,14 +1119,11 @@ def _download_http_url(
     content_type = resp.headers.get('content-type', '')
     filename = link.filename  # fallback
     # 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)
         if ext:
             filename += ext
diff --git a/tests/unit/test_download.py b/tests/unit/test_download.py
index 438ebcb2..7baee5e0 100644
--- a/tests/unit/test_download.py
+++ b/tests/unit/test_download.py
@@ -10,10 +10,11 @@ import pytest
 from mock import Mock, patch
 
 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
 from pip._internal.models.link import Link
 from pip._internal.utils.hashes import Hashes
@@ -197,10 +198,94 @@ def test_unpack_http_url_bad_downloaded_checksum(mock_unpack_file):
 
     finally:
         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'),
     ('file:/path/to/file', r'\path\to\file', '/path/to/file'),
     ('file://localhost/tmp/file', r'\tmp\file', '/tmp/file'),

Vulnerable code:            'unpack_http_url', 'unpack_url']
        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
Fixed code: Prevent ``pip install <url>`` from permitting directory traversal if e.g.
a malicious server sends a ``Content-Disposition`` header with a filename
containing ``../`` or ``..\\``.
           'unpack_http_url', 'unpack_url',
           'parse_content_disposition', 'sanitize_content_filename']
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


        filename = parse_content_disposition(content_disposition, filename)
    _download_http_url, parse_content_disposition, sanitize_content_filename,
@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']