SWAT-20: Remove header injection vulnerability on redirect

There was a Header Injection issue (remember HTTP is a text protocol and new
lines matter).  Basically you could inject a header or even the text with
certain parameters.  Typically, this failed on redirects because of the location
header.  We now throw an exception if the redirect url includes a new line or
carriage return.  Neither of these are valid (though you can presumably escape
them).

turbogears/controllers.py

@@ -325,7 +325,7 @@ def expose(template=None, validators=None, allow_json=None, html=None,
             applied to that arg
     @keyparam inputform deprecated. A form object that generates the
             input to this method
-    @keyparam exclude_from_memory_profiling allows to exclude individual end points from memory profiling. Can be 
+    @keyparam exclude_from_memory_profiling allows to exclude individual end points from memory profiling. Can be
             used for performance or in case profiling generates errors
     """
     if html:
@@ -602,60 +602,67 @@ def check_app_root():
     request.app_root = app_root
 
 
-def get_server_name(): 
-    """Return name of the server this application runs on. 
- 
-    Respects 'Host' and 'X-Forwarded-Host' header. 
- 
-    See the docstring of the 'absolute_url' function for more information. 
- 
-    """ 
-    get = config.get 
-    h = request.headers 
-    host = get('tg.url_domain') or h.get('X-Forwarded-Host', h.get('Host')) 
-    if not host: 
-        host = '%s:%s' % (get('server.socket_host', 'localhost'), 
-            get('server.socket_port', 8080)) 
-    return host 
-
-
-def absolute_url(tgpath='/', params=None, **kw): 
-    """Return absolute URL (including schema and host to this server). 
- 
-    Tries to account for 'Host' header and reverse proxying 
-    ('X-Forwarded-Host'). 
- 
-    The host name is determined this way: 
- 
-    * If the config setting 'tg.url_domain' is set and non-null, use this value. 
-    * Else, if the 'base_url_filter.use_x_forwarded_host' config setting is 
-      True, use the value from the 'Host' or 'X-Forwarded-Host' request header. 
-    * Else, if config setting 'base_url_filter.on' is True and 
-      'base_url_filter.base_url' is non-null, use its value for the host AND 
-      scheme part of the URL. 
-    * As a last fallback, use the value of 'server.socket_host' and 
-      'server.socket_port' config settings (defaults to 'localhost:8080'). 
- 
-    The URL scheme ('http' or 'http') used is determined in the following way: 
- 
-    * If 'base_url_filter.base_url' is used, use the scheme from this URL. 
-    * If there is a 'X-Use-SSL' request header, use 'https'. 
-    * Else, if the config setting 'tg.url_scheme' is set, use its value. 
-    * Else, use the value of 'cherrypy.request.scheme'. 
- 
-    """ 
-    get = config.get 
-    use_xfh = get('base_url_filter.use_x_forwarded_host', False) 
-    if request.headers.get('X-Use-SSL'): 
-        scheme = 'https' 
-    else: 
-        scheme = get('tg.url_scheme') 
-    if not scheme: 
-        scheme = request.scheme 
-    base_url = '%s://%s' % (scheme, get_server_name()) 
-    if get('base_url_filter.on', False) and not use_xfh: 
-        base_url = get('base_url_filter.base_url').rstrip('/') 
-    return '%s%s' % (base_url, url(tgpath, params, **kw)) 
+def get_server_name():
+    """Return name of the server this application runs on.
+
+    Respects 'Host' and 'X-Forwarded-Host' header.
+
+    See the docstring of the 'absolute_url' function for more information.
+
+    """
+    get = config.get
+    h = request.headers
+    host = get('tg.url_domain') or h.get('X-Forwarded-Host', h.get('Host'))
+    if not host:
+        host = '%s:%s' % (get('server.socket_host', 'localhost'),
+            get('server.socket_port', 8080))
+    return host
+
+
+def absolute_url(tgpath='/', params=None, **kw):
+    """Return absolute URL (including schema and host to this server).
+
+    Tries to account for 'Host' header and reverse proxying
+    ('X-Forwarded-Host').
+
+    The host name is determined this way:
+
+    * If the config setting 'tg.url_domain' is set and non-null, use this value.
+    * Else, if the 'base_url_filter.use_x_forwarded_host' config setting is
+      True, use the value from the 'Host' or 'X-Forwarded-Host' request header.
+    * Else, if config setting 'base_url_filter.on' is True and
+      'base_url_filter.base_url' is non-null, use its value for the host AND
+      scheme part of the URL.
+    * As a last fallback, use the value of 'server.socket_host' and
+      'server.socket_port' config settings (defaults to 'localhost:8080').
+
+    The URL scheme ('http' or 'http') used is determined in the following way:
+
+    * If 'base_url_filter.base_url' is used, use the scheme from this URL.
+    * If there is a 'X-Use-SSL' request header, use 'https'.
+    * Else, if the config setting 'tg.url_scheme' is set, use its value.
+    * Else, use the value of 'cherrypy.request.scheme'.
+
+    """
+    get = config.get
+    use_xfh = get('base_url_filter.use_x_forwarded_host', False)
+    if request.headers.get('X-Use-SSL'):
+        scheme = 'https'
+    else:
+        scheme = get('tg.url_scheme')
+    if not scheme:
+        scheme = request.scheme
+    base_url = '%s://%s' % (scheme, get_server_name())
+    if get('base_url_filter.on', False) and not use_xfh:
+        base_url = get('base_url_filter.base_url').rstrip('/')
+    return '%s%s' % (base_url, url(tgpath, params, **kw))
+
+
+class InvalidRedirectException(Exception):
+    """
+    An invalid redirect url was provided.  Redirects cannot
+    include a carriage return (\r) or new line (\n) character.
+    """
 
 
 def redirect(redirect_path, redirect_params=None, **kw):
@@ -673,6 +680,8 @@ def redirect(redirect_path, redirect_params=None, **kw):
         if path.startswith(request.app_root):
             path = path[len(request.app_root):]
         redirect_path = urlparse.urljoin(path, redirect_path)
+    if set(redirect_path).intersection({'\r', '\n'}):
+        raise InvalidRedirectException('Invalid redirect: {}'.format(redirect_path))
     raise cherrypy.HTTPRedirect(url(tgpath=redirect_path,
         tgparams=redirect_params, **kw))
 

turbogears/release.py

@@ -28,7 +28,7 @@
     http://svn.turbogears.org/trunk#egg=turbogears-dev
 """
 
-version = "1.0.11.8"
+version = "1.0.11.9"
 description = "Front-to-back, open-source, rapid web development framework"
 long_description = __doc__
 author = "Kevin Dangoor"

turbogears/tests/test_controllers.py

@@ -257,6 +257,10 @@ def redirect_to_path_list(self, path):
     def redirect_to_path_tuple(self, path):
         raise redirect((path, 'index'))
 
+    [expose()]
+    def redirect_error(self):
+        raise redirect('/blah\r\n')
+
 
 class TestRoot(unittest.TestCase):
 
@@ -712,6 +716,11 @@ def test_redirect_to_path(self):
                 assert (cherrypy.response.headers['Location']
                     == 'http://localhost/subthing/index'), url
 
+    def test_redirect_error(self):
+        url = "/redirect_error"
+        testutil.create_request(url)
+        assert cherrypy.response.status.startswith("500")
+
     def test_multi_values(self):
         testutil.create_request("/")
         assert url("/foo", bar=[1, 2]) in \