Fix XSS attacks as reported by Tim Wintle

6 files changed, 44 insertions, 18 deletions

diff --git a/docs/news.txt b/docs/news.txt
index 7ff0529..3168815 100644
--- a/docs/news.txt
+++ b/docs/news.txt
@@ -3,6 +3,15 @@ News
 
 .. contents::
 
+1.7.4
+-----
+
+* Fix XSS bug (security issue) with not found handlers for
+  :class:`paste.urlparser.StaticURLParser` and
+  :class:`paste.urlmap.URLMap`.  If you ask for a path with
+  ``/--><script>...`` that will be inserted in the error page and can
+  execute Javascript.  Reported by Tim Wintle.
+
 1.7.3.1
 -------
 
diff --git a/paste/httpexceptions.py b/paste/httpexceptions.py
index 8e2f81c..208d5cf 100644
--- a/paste/httpexceptions.py
+++ b/paste/httpexceptions.py
@@ -77,7 +77,7 @@ import types
 from paste.wsgilib import catch_errors_app
 from paste.response import has_header, header_value, replace_header
 from paste.request import resolve_relative_url
-from paste.util.quoting import strip_html, html_quote, no_quote
+from paste.util.quoting import strip_html, html_quote, no_quote, comment_quote
 
 SERVER_NAME = 'WSGI Server'
 TEMPLATE = """\
@@ -212,12 +212,12 @@ class HTTPException(Exception):
 
     def plain(self, environ):
         """ text/plain representation of the exception """
-        body = self.make_body(environ, strip_html(self.template), no_quote)
+        body = self.make_body(environ, strip_html(self.template), comment_quote)
         return ('%s %s\r\n%s\r\n' % (self.code, self.title, body))
 
     def html(self, environ):
         """ text/html representation of the exception """
-        body = self.make_body(environ, self.template, html_quote, no_quote)
+        body = self.make_body(environ, self.template, html_quote, comment_quote)
         return TEMPLATE % {
                    'title': self.title,
                    'code': self.code,
@@ -334,14 +334,14 @@ class _HTTPMove(HTTPRedirection):
 
     def relative_redirect(cls, dest_uri, environ, detail=None, headers=None, comment=None):
         """
-        Create a redirect object with the dest_uri, which may be relative, 
+        Create a redirect object with the dest_uri, which may be relative,
         considering it relative to the uri implied by the given environ.
         """
         location = resolve_relative_url(dest_uri, environ)
         headers = headers or []
         headers.append(('Location', location))
         return cls(detail=detail, headers=headers, comment=comment)
-    
+
     relative_redirect = classmethod(relative_redirect)
 
     def location(self):
@@ -658,4 +658,3 @@ def make_middleware(app, global_conf=None, warning_level=None):
     return HTTPExceptionHandler(app, warning_level=warning_level)
 
 __all__.extend(['HTTPExceptionHandler', 'get_exception'])
-
diff --git a/paste/urlmap.py b/paste/urlmap.py
index c80ce71..a636531 100644
--- a/paste/urlmap.py
+++ b/paste/urlmap.py
@@ -7,6 +7,7 @@ Map URL prefixes to WSGI applications.  See ``URLMap``
 from UserDict import DictMixin
 import re
 import os
+import cgi
 from paste import httpexceptions
 
 __all__ = ['URLMap', 'PathProxyURLMap']
@@ -77,12 +78,12 @@ class URLMap(DictMixin):
     dispatch to.  URLs are matched most-specific-first, i.e., longest
     URL first.  The ``SCRIPT_NAME`` and ``PATH_INFO`` environmental
     variables are adjusted to indicate the new context.
-    
+
     URLs can also include domains, like ``http://blah.com/foo``, or as
     tuples ``('blah.com', '/foo')``.  This will match domain names; without
     the ``http://domain`` or with a domain of ``None`` any domain will be
     matched (so long as no other explicit domain matches).  """
-    
+
     def __init__(self, not_found_app=None):
         self.applications = []
         if not not_found_app:
@@ -105,7 +106,7 @@ class URLMap(DictMixin):
         extra += '\nHTTP_HOST: %r' % environ.get('HTTP_HOST')
         app = httpexceptions.HTTPNotFound(
             environ['PATH_INFO'],
-            comment=extra).wsgi_application
+            comment=cgi.escape(extra)).wsgi_application
         return app(environ, start_response)
 
     def normalize_url(self, url, trim=True):
@@ -113,7 +114,7 @@ class URLMap(DictMixin):
             domain = url[0]
             url = self.normalize_url(url[1])[1]
             return domain, url
-        assert (not url or url.startswith('/') 
+        assert (not url or url.startswith('/')
                 or self.domain_url_re.search(url)), (
             "URL fragments must start with / or http:// (you gave %r)" % url)
         match = self.domain_url_re.search(url)
@@ -165,7 +166,7 @@ class URLMap(DictMixin):
             if app_url == dom_url:
                 return app
         raise KeyError(
-            "No application with the url %r (domain: %r; existing: %s)" 
+            "No application with the url %r (domain: %r; existing: %s)"
             % (url[1], url[0] or '*', self.applications))
 
     def __delitem__(self, url):
@@ -202,8 +203,8 @@ class URLMap(DictMixin):
                 return app(environ, start_response)
         environ['paste.urlmap_object'] = self
         return self.not_found_application(environ, start_response)
-    
-            
+
+
 class PathProxyURLMap(object):
 
     """
@@ -225,7 +226,7 @@ class PathProxyURLMap(object):
         self.base_paste_url = self.map.normalize_url(base_paste_url)
         self.base_path = base_path
         self.builder = builder
-        
+
     def __setitem__(self, url, app):
         if isinstance(app, (str, unicode)):
             app_fn = os.path.join(self.base_path, app)
@@ -233,7 +234,7 @@ class PathProxyURLMap(object):
         url = self.map.normalize_url(url)
         # @@: This means http://foo.com/bar will potentially
         # match foo.com, but /base_paste_url/bar, which is unintuitive
-        url = (url[0] or self.base_paste_url[0], 
+        url = (url[0] or self.base_paste_url[0],
                self.base_paste_url[1] + url[1])
         self.map[url] = app
 
@@ -247,5 +248,3 @@ class PathProxyURLMap(object):
         self.map.not_found_application = value
     not_found_application = property(not_found_application__get,
                                      not_found_application__set)
-        
-    
diff --git a/paste/util/quoting.py b/paste/util/quoting.py
index b596d7f..582cc40 100644
--- a/paste/util/quoting.py
+++ b/paste/util/quoting.py
@@ -76,6 +76,13 @@ def no_quote(s):
     """
     return s
 
+_comment_quote_re = re.compile(r'\-\s*\>')
+def comment_quote(s):
+    """
+    Quote that makes sure text can't escape a comment
+    """
+    return _comment_quote_re.sub('-&gt', str(s))
+
 url_quote = urllib.quote
 url_unquote = urllib.unquote
 
diff --git a/tests/test_urlmap.py b/tests/test_urlmap.py
index 1f7fd2a..60b66eb 100644
--- a/tests/test_urlmap.py
+++ b/tests/test_urlmap.py
@@ -39,4 +39,9 @@ def test_map():
     res.mustcontain('script_name="/f"')
     res.mustcontain('path_info="/z/y"')
     res.mustcontain('f-only')
-    
+
+def test_404():
+    mapper = URLMap({})
+    app = TestApp(mapper, extra_environ={'HTTP_ACCEPT': 'text/html'})
+    res = app.get("/-->%0D<script>alert('xss')</script>", status=404)
+    assert '--><script' not in res.body
diff --git a/tests/test_urlparser.py b/tests/test_urlparser.py
index 6f9d200..790535d 100644
--- a/tests/test_urlparser.py
+++ b/tests/test_urlparser.py
@@ -106,6 +106,13 @@ def test_relative_path_in_static_parser():
     app = StaticURLParser(relative_path('find_file'))
     assert '..' not in app.root_directory
 
+def test_xss():
+    app = TestApp(StaticURLParser(relative_path('find_file')),
+                  extra_environ={'HTTP_ACCEPT': 'text/html'})
+    res = app.get("/-->%0D<script>alert('xss')</script>", status=404)
+    print res
+    assert 0
+
 def test_static_parser():
     app = StaticURLParser(path('find_file'))
     testapp = TestApp(app)