Mercurial > p > roundup > code
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')
