changeset 8558:5fbf6451a782

bug: harden header/environment values for roundup-server and cgi If the environment (cgi) or header variables (server) have values with characters outside of the printable ascii range (chr(32-126)), return HTTP 400 error. This is overly strict but nothing that Roundup looks at requires a larger range. When deploying with wsgi and Zope, server software should verify proper values. This fix was riggered by the waitress wsgi server bug: https://github.com/Pylons/waitress/security/advisories/GHSA-m5ff-3wj3-8ph4 which was caused by incorrect validation of header values resulting in a the proxy and waitress having different interpretations of what the header meant. My testing of the roundup.cgi script is to use a cgi->wsgi wrapper and run it under wsgi (using waitress). I need to try it under a real server that can run cgi. It looks like python http.server --cgi is missing definitions of HTTP_HOST and other required CGI variables. That's probably why the --cgi option was removed, but it leaves me without a good way to test. Maybe https://github.com/mdklatt/pytest-cgi could be used to test that front end? Arguably CGI is old, but cheap hosting still allows it.
author John Rouillard <rouilj@ieee.org>
date Wed, 08 Apr 2026 00:35:34 -0400
parents f80c566f5726
children 19670ecbad82
files CHANGES.txt frontends/roundup.cgi roundup/cgi/client.py roundup/scripts/roundup_server.py
diffstat 4 files changed, 87 insertions(+), 23 deletions(-) [+]
line wrap: on
line diff
--- a/CHANGES.txt	Mon Apr 06 22:10:23 2026 -0400
+++ b/CHANGES.txt	Wed Apr 08 00:35:34 2026 -0400
@@ -69,7 +69,14 @@
   supports some mime types, ads default mime type for files without a
   mime type (e.g. message contents). Cleaner code. (John Rouillard)
 - run hexora and mitigate/fix some medium sev and above. (John Rouillard)
-
+- Return 400 if environment or header variables 
+  have values with characters outside of the printable ascii range
+  (32-127). Applies to  roundup-server and roundup-cgi. wsgi and Zope
+  depend on the hosting server for sanitizing. Se waitress wsgi server
+  bug:
+  https://github.com/Pylons/waitress/security/advisories/GHSA-m5ff-3wj3-8ph4
+  (John Rouillard)
+  
 Features:
 
 - add support for authorized changes. User can be prompted to enter
--- a/frontends/roundup.cgi	Mon Apr 06 22:10:23 2026 -0400
+++ b/frontends/roundup.cgi	Wed Apr 08 00:35:34 2026 -0400
@@ -18,11 +18,14 @@
 
 # python version check
 from __future__ import print_function
+
+import sys
+import time
+
 from roundup import version_check
-from roundup.i18n import _
 from roundup.anypy.html import html_escape
-from roundup.anypy.strings import s2b, StringIO
-import sys, time
+from roundup.anypy.strings import StringIO, s2b
+from roundup.i18n import _
 
 #
 ##  Configuration
@@ -31,17 +34,17 @@
 # Configuration can also be provided through the OS environment (or via
 # the Apache "SetEnv" configuration directive). If the variables
 # documented below are set, they _override_ any configuation defaults
-# given in this file. 
+# given in this file.
 
 # TRACKER_HOMES is a list of trackers, in the form
 # "NAME=DIR<sep>NAME2=DIR2<sep>...", where <sep> is the directory path
-# separator (";" on Windows, ":" on Unix). 
+# separator (";" on Windows, ":" on Unix).
 
-# Make sure the NAME part doesn't include any url-unsafe characters like 
+# Make sure the NAME part doesn't include any url-unsafe characters like
 # spaces, as these confuse the cookie handling in browsers like IE.
 
 # ROUNDUP_LOG is the name of the logfile; if it's empty or does not exist,
-# logging is turned off (unless you changed the default below). 
+# logging is turned off (unless you changed the default below).
 
 # DEBUG_TO_CLIENT specifies whether debugging goes to the HTTP server (via
 # stderr) or to the web client (via cgitb).
@@ -52,15 +55,20 @@
 #    'example': '/path/to/example',
 }
 
+
 # Where to log debugging information to. Use an instance of DevNull if you
 # don't want to log anywhere.
 class DevNull:
     def write(self, info):
         pass
+
     def close(self):
         pass
+
     def flush(self):
         pass
+
+
 #LOG = open('/var/log/roundup.cgi.log', 'a')
 LOG = DevNull()
 
@@ -71,11 +79,12 @@
 
 #
 # Set up the error handler
-# 
+#
 try:
     import traceback
+
     from roundup.cgi import cgitb
-except:
+except ImportError:
     print("Content-Type: text/plain\n")
     print(_("Failed to import cgitb!\n\n"))
     s = StringIO()
@@ -100,13 +109,13 @@
         TRACKER_HOMES = {}
         for home in homes.split(os.pathsep):
             try:
-                name, dir = home.split('=', 1)
+                name, dir_ = home.split('=', 1)
             except ValueError:
                 # ignore invalid definitions
                 continue
-            if name and dir:
-                TRACKER_HOMES[name] = dir
-                
+            if name and dir_:
+                TRACKER_HOMES[name] = dir_
+
     logname = os.environ.get('ROUNDUP_LOG', '')
     if logname:
         LOG = open(logname, 'a')
@@ -123,28 +132,62 @@
     def __init__(self, wfile):
         self.rfile = sys.stdin
         self.wfile = wfile
+
     def write(self, data):
         self.wfile.write(data)
+
     def send_response(self, code):
-        self.write(s2b('Status: %s\r\n'%code))
+        self.write(s2b('Status: %s\r\n' % code))
+
     def send_header(self, keyword, value):
         self.write(s2b("%s: %s\r\n" % (keyword, value)))
+
     def end_headers(self):
         self.write(b"\r\n")
+
     def start_response(self, headers, response):
         self.send_response(response)
         for key, value in headers:
             self.send_header(key, value)
         self.end_headers()
 
+
 #
 # Main CGI handler
 #
 def main(out, err):
     import os
+
+    import roundup.cgi.client
     import roundup.instance
+
+    request = RequestWrapper(out)
+    problem_header = roundup.cgi.client.are_header_values_safe(os.environ)
+    if problem_header:
+        error_body = """<!DOCTYPE HTML>
+        <html lang="en">
+        <head>
+          <meta charset="utf-8">
+          <title>Error response</title>
+        </head>
+        <body>
+          <h1>Error response</h1>
+          <p>Error code: 400</p>
+          <p>Message: Invalid value: %s.</p>
+          <p>Error code explanation: 400 - The value for the header is unacceptable.</p>
+        </body>
+        </html>
+        """ % problem_header
+
+        request.send_response(400)
+        request.send_header('Content-Type', 'text/html')
+        request.send_header('Content-Length', '%s' % len(error_body))
+        request.end_headers()
+        out.write(s2b(error_body))
+        return
+
+
     path = os.environ.get('PATH_INFO', '/').split('/')
-    request = RequestWrapper(out)
     request.path = os.environ.get('PATH_INFO', '/')
     tracker = path[1]
     os.environ['TRACKER_NAME'] = tracker
@@ -158,7 +201,7 @@
                 protocol = 'https'
             else:
                 protocol = 'http'
-            absolute_url = '%s://%s%s/'%(protocol, os.environ['HTTP_HOST'],
+            absolute_url = '%s://%s%s/' % (protocol, os.environ['HTTP_HOST'],
                 os.environ.get('REQUEST_URI', ''))
             request.send_header('Location', absolute_url)
             request.end_headers()
@@ -166,7 +209,6 @@
         else:
             tracker_home = TRACKER_HOMES[tracker]
             tracker = roundup.instance.open(tracker_home)
-            import roundup.cgi.client
             if hasattr(tracker, 'Client'):
                 client = tracker.Client(tracker, request, os.environ)
             else:
@@ -182,7 +224,7 @@
                 request.send_response(404)
                 request.send_header('Content-Type', 'text/html')
                 request.end_headers()
-                out.write(s2b('Not found: %s'%html_escape(client.path)))
+                out.write(s2b('Not found: %s' % html_escape(client.path)))
 
     else:
         from roundup.anypy import urllib_
@@ -194,12 +236,13 @@
         w(s2b(_('<body><h1>Roundup trackers index</h1><ol>\n')))
         homes = sorted(TRACKER_HOMES.keys())
         for tracker in homes:
-            w(s2b(_('<li><a href="%(tracker_url)s/index">%(tracker_name)s</a>\n')%{
-                'tracker_url': os.environ['SCRIPT_NAME']+'/'+
+            w(s2b(_('<li><a href="%(tracker_url)s/index">%(tracker_name)s</a>\n') % {
+                'tracker_url': os.environ['SCRIPT_NAME'] + '/' +
                                urllib_.quote(tracker),
                 'tracker_name': html_escape(tracker)}))
         w(s2b(_('</ol></body></html>')))
 
+
 #
 # Now do the actual CGI handling
 #
@@ -207,7 +250,8 @@
 try:
     # force input/output to binary (important for file up/downloads)
     if sys.platform == "win32":
-        import os, msvcrt
+        import msvcrt
+        import os
         msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY)
         msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY)
     checkconfig()
@@ -228,7 +272,7 @@
         out.write(cgitb.breaker())
         ts = time.ctime()
         out.write('''<p>%s: An error occurred. Please check
-            the server log for more information.</p>'''%ts)
+            the server log for more information.</p>''' % ts)
         print('EXCEPTION AT', ts, file=sys.stderr)
         traceback.print_exc(0, sys.stderr)
 
--- a/roundup/cgi/client.py	Mon Apr 06 22:10:23 2026 -0400
+++ b/roundup/cgi/client.py	Wed Apr 08 00:35:34 2026 -0400
@@ -132,6 +132,12 @@
     import random
     random.seed()
 
+_safe_char_set = {chr(x) for x in range(32,127)}
+def are_header_values_safe(header_list):
+    for header, value in header_list.items():
+        if (set(value) - _safe_char_set):
+            return header, value
+    return None
 
 class LiberalCookie(SimpleCookie):
     """ Python's SimpleCookie throws an exception if the cookie uses invalid
--- a/roundup/scripts/roundup_server.py	Mon Apr 06 22:10:23 2026 -0400
+++ b/roundup/scripts/roundup_server.py	Wed Apr 08 00:35:34 2026 -0400
@@ -278,6 +278,10 @@
             self.send_error(404, self.path)
         except client.Unauthorised as message:
             self.send_error(403, '%s (%s)' % (self.path, message))
+        except ValueError as e:
+            self.send_error(400,
+                            "Invalid value: %s" % str(e.args[0]),
+                            "The value for the header is unacceptable")
         except Exception:
             exc, val, tb = sys.exc_info()
             if hasattr(socket, 'timeout') and isinstance(val, socket.timeout):
@@ -455,6 +459,9 @@
         env['PATH_INFO'] = urllib_.unquote(rest)
         if query:
             env['QUERY_STRING'] = query
+        problem_header = client.are_header_values_safe(self.headers)
+        if problem_header:
+            raise ValueError(problem_header)
         if hasattr(self.headers, 'get_content_type'):
             # Python 3.  We need the raw header contents.
             content_type = self.headers.get('content-type')

Roundup Issue Tracker: http://roundup-tracker.org/