CRLF Injection Mitigation

Resolves #237

Previously, we were not running any sort of URL escaping on values
passed in from the client that were used for redirects. This allowed
injection attacks via URL encoded newlines in the original request.

This update ensures that all user-supplied paths that are used as
components of redirects are passed through `urllib.parse.quote()`
(or the python 2 equivalent) prior to being used in a redirect
response.

Also specified 127.0.0.1 rather than 0.0.0.0 (the default) in server
tests to avoid triggering firewall dialogs when testing on MacOS

Author: mplanchard

pypiserver/_app.py

@@ -250,7 +250,7 @@ def simpleindex():
 @auth("list")
 def simple(prefix=""):
     # PEP 503: require normalized prefix
-    normalized = core.normalize_pkgname(prefix)
+    normalized = core.normalize_pkgname_for_url(prefix)
     if prefix != normalized:
         return redirect('/simple/{0}/'.format(normalized), 301)
 
@@ -327,11 +327,5 @@ def server_static(filename):
 @app.route('/:prefix')
 @app.route('/:prefix/')
 def bad_url(prefix):
-    p = request.fullpath
-    if p.endswith("/"):
-        p = p[:-1]
-    p = p.rsplit('/', 1)[0]
-    p += "/simple/%s/" % prefix
-
-    return redirect(p)
-
+    """Redirect unknown root URLs to /simple/."""
+    return redirect(core.get_bad_url_redirect_path(request, prefix))

pypiserver/core.py

@@ -11,6 +11,11 @@
 import re
 import sys
 
+try:  # PY3
+    from urllib.parse import quote
+except ImportError:  # PY2
+    from urllib import quote
+
 import pkg_resources
 
 from . import Configuration
@@ -183,6 +188,11 @@ def normalize_pkgname(name):
     return re.sub(r"[-_.]+", "-", name).lower()
 
 
+def normalize_pkgname_for_url(name):
+    """Perform PEP 503 normalization and ensure the value is safe for URLs."""
+    return quote(re.sub(r"[-_.]+", "-", name).lower())
+
+
 def is_allowed_path(path_part):
     p = path_part.replace("\\", "/")
     return not (p.startswith(".") or "/." in p)
@@ -273,6 +283,17 @@ def store(root, filename, save_method):
     save_method(dest_fn, overwrite=True)  # Overwite check earlier.
 
 
+def get_bad_url_redirect_path(request, prefix):
+    """Get the path for a bad root url."""
+    p = request.fullpath
+    if p.endswith("/"):
+        p = p[:-1]
+    p = p.rsplit('/', 1)[0]
+    prefix = quote(prefix)
+    p += "/simple/{}/".format(prefix)
+    return p
+
+
 def _digest_file(fpath, hash_algo):
     """
     Reads and digests a file according to specified hashing-algorith.

tests/doubles.py

@@ -0,0 +1,10 @@
+"""Test doubles."""
+
+
+class Namespace(object):
+    """Simple namespace."""
+
+    def __init__(self, **kwargs):
+        """Instantiate the namespace with the provided kwargs."""
+        for k, v in kwargs.items():
+            setattr(self, k, v)

tests/test_app.py

@@ -20,6 +20,7 @@
 
 # Local Imports
 from pypiserver import __main__, bottle
+
 import tests.test_core as test_core
 
 
@@ -430,11 +431,11 @@ def test_upload_badFilename(package, root, testapp):
 def test_remove_pkg_missingNaveVersion(name, version, root, testapp):
     msg = "Missing 'name'/'version' fields: name=%s, version=%s"
     params = {':action': 'remove_pkg', 'name': name, 'version': version}
-    params = dict((k, v) for k,v in params.items() if v is not None)
+    params = dict((k, v) for k, v in params.items() if v is not None)
     resp = testapp.post("/", expect_errors=1, params=params)
 
     assert resp.status == '400 Bad Request'
-    assert msg %(name, version) in hp.unescape(resp.text)
+    assert msg % (name, version) in hp.unescape(resp.text)
 
 
 def test_remove_pkg_notFound(root, testapp):

tests/test_core.py

@@ -7,6 +7,7 @@
 import pytest
 
 from pypiserver import __main__, core
+from tests.doubles import Namespace
 
 
 ## Enable logging to detect any problems with it
@@ -90,3 +91,20 @@ def test_hashfile(tmpdir, algo, digest):
     f = tmpdir.join("empty")
     f.ensure()
     assert core.digest_file(f.strpath, algo) == digest
+
+
+def test_redirect_prefix_encodes_newlines():
+    """Ensure raw newlines are url encoded in the generated redirect."""
+    request = Namespace(
+        fullpath='/\nSet-Cookie:malicious=1;'
+    )
+    prefix = '\nSet-Cookie:malicious=1;'
+    newpath = core.get_bad_url_redirect_path(request, prefix)
+    assert '\n' not in newpath
+
+
+def test_normalize_pkgname_for_url_encodes_newlines():
+    """Ensure newlines are url encoded in package names for urls."""
+    assert '\n' not in core.normalize_pkgname_for_url(
+        '/\nSet-Cookie:malicious=1;'
+    )

tests/test_server.py

@@ -61,8 +61,16 @@ def _run_server(packdir, port, authed, other_cli=''):
         'partial': "-Ptests/htpasswd.a.a -a update",
     }
     pswd_opts = pswd_opt_choices[authed]
-    cmd = "%s -m pypiserver.__main__ -vvv --overwrite -p %s %s %s %s" % (
-        sys.executable, port, pswd_opts, other_cli, packdir)
+    cmd = (
+        "%s -m pypiserver.__main__ -vvv --overwrite -i 127.0.0.1 "
+        "-p %s %s %s %s" % (
+            sys.executable,
+            port,
+            pswd_opts,
+            other_cli,
+            packdir,
+        )
+    )
     proc = subprocess.Popen(cmd.split(), bufsize=_BUFF_SIZE)
     time.sleep(SLEEP_AFTER_SRV)
     assert proc.poll() is None