Merge pull request #238 from pypiserver/237-CRLF-Injection

CRLF Injection Mitigation

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 os
 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 @@ import webtest
 
 # 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 os
 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