From 923ba361d8f757f0656cfd216525aca4848e02aa Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 6 Apr 2009 06:33:26 +0000 Subject: [PATCH] - Issue #2254: Fix CGIHTTPServer information disclosure. Relative paths are now collapsed within the url properly before looking in cgi_directories. --- Lib/CGIHTTPServer.py | 69 ++++++++++++++++++++++++++---------- Lib/test/test_httpservers.py | 46 ++++++++++++++++++++++++ Misc/NEWS | 3 ++ 3 files changed, 100 insertions(+), 18 deletions(-) diff --git a/Lib/CGIHTTPServer.py b/Lib/CGIHTTPServer.py index 71f03687856..13bfcdd22f0 100644 --- a/Lib/CGIHTTPServer.py +++ b/Lib/CGIHTTPServer.py @@ -70,27 +70,20 @@ class CGIHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): return SimpleHTTPServer.SimpleHTTPRequestHandler.send_head(self) def is_cgi(self): - """Test whether self.path corresponds to a CGI script, - and return a boolean. + """Test whether self.path corresponds to a CGI script. - This function sets self.cgi_info to a tuple (dir, rest) - when it returns True, where dir is the directory part before - the CGI script name. Note that rest begins with a - slash if it is not empty. + Returns True and updates the cgi_info attribute to the tuple + (dir, rest) if self.path requires running a CGI script. + Returns False otherwise. - The default implementation tests whether the path - begins with one of the strings in the list - self.cgi_directories (and the next character is a '/' - or the end of the string). + The default implementation tests whether the normalized url + path begins with one of the strings in self.cgi_directories + (and the next character is a '/' or the end of the string). """ - - path = self.path - - for x in self.cgi_directories: - i = len(x) - if path[:i] == x and (not path[i:] or path[i] == '/'): - self.cgi_info = path[:i], path[i+1:] - return True + splitpath = _url_collapse_path_split(self.path) + if splitpath[0] in self.cgi_directories: + self.cgi_info = splitpath + return True return False cgi_directories = ['/cgi-bin', '/htbin'] @@ -330,6 +323,46 @@ class CGIHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): self.log_message("CGI script exited OK") +# TODO(gregory.p.smith): Move this into an appropriate library. +def _url_collapse_path_split(path): + """ + Given a URL path, remove extra '/'s and '.' path elements and collapse + any '..' references. + + Implements something akin to RFC-2396 5.2 step 6 to parse relative paths. + + Returns: A tuple of (head, tail) where tail is everything after the final / + and head is everything before it. Head will always start with a '/' and, + if it contains anything else, never have a trailing '/'. + + Raises: IndexError if too many '..' occur within the path. + """ + # Similar to os.path.split(os.path.normpath(path)) but specific to URL + # path semantics rather than local operating system semantics. + path_parts = [] + for part in path.split('/'): + if part == '.': + path_parts.append('') + else: + path_parts.append(part) + # Filter out blank non trailing parts before consuming the '..'. + path_parts = [part for part in path_parts[:-1] if part] + path_parts[-1:] + if path_parts: + tail_part = path_parts.pop() + else: + tail_part = '' + head_parts = [] + for part in path_parts: + if part == '..': + head_parts.pop() + else: + head_parts.append(part) + if tail_part and tail_part == '..': + head_parts.pop() + tail_part = '' + return ('/' + '/'.join(head_parts), tail_part) + + nobody = None def nobody_uid(): diff --git a/Lib/test/test_httpservers.py b/Lib/test/test_httpservers.py index 31321c3fa6a..6edc7d3276c 100644 --- a/Lib/test/test_httpservers.py +++ b/Lib/test/test_httpservers.py @@ -7,6 +7,7 @@ Josip Dzolonga, and Michael Otteneder for the 2007/08 GHOP contest. from BaseHTTPServer import BaseHTTPRequestHandler, HTTPServer from SimpleHTTPServer import SimpleHTTPRequestHandler from CGIHTTPServer import CGIHTTPRequestHandler +import CGIHTTPServer import os import sys @@ -315,6 +316,45 @@ class CGIHTTPServerTestCase(BaseTestCase): finally: BaseTestCase.tearDown(self) + def test_url_collapse_path_split(self): + test_vectors = { + '': ('/', ''), + '..': IndexError, + '/.//..': IndexError, + '/': ('/', ''), + '//': ('/', ''), + '/\\': ('/', '\\'), + '/.//': ('/', ''), + 'cgi-bin/file1.py': ('/cgi-bin', 'file1.py'), + '/cgi-bin/file1.py': ('/cgi-bin', 'file1.py'), + 'a': ('/', 'a'), + '/a': ('/', 'a'), + '//a': ('/', 'a'), + './a': ('/', 'a'), + './C:/': ('/C:', ''), + '/a/b': ('/a', 'b'), + '/a/b/': ('/a/b', ''), + '/a/b/c/..': ('/a/b', ''), + '/a/b/c/../d': ('/a/b', 'd'), + '/a/b/c/../d/e/../f': ('/a/b/d', 'f'), + '/a/b/c/../d/e/../../f': ('/a/b', 'f'), + '/a/b/c/../d/e/.././././..//f': ('/a/b', 'f'), + '../a/b/c/../d/e/.././././..//f': IndexError, + '/a/b/c/../d/e/../../../f': ('/a', 'f'), + '/a/b/c/../d/e/../../../../f': ('/', 'f'), + '/a/b/c/../d/e/../../../../../f': IndexError, + '/a/b/c/../d/e/../../../../f/..': ('/', ''), + } + for path, expected in test_vectors.iteritems(): + if isinstance(expected, type) and issubclass(expected, Exception): + self.assertRaises(expected, + CGIHTTPServer._url_collapse_path_split, path) + else: + actual = CGIHTTPServer._url_collapse_path_split(path) + self.assertEquals(expected, actual, + msg='path = %r\nGot: %r\nWanted: %r' % ( + path, actual, expected)) + def test_headers_and_content(self): res = self.request('/cgi-bin/file1.py') self.assertEquals(('Hello World\n', 'text/html', 200), \ @@ -339,6 +379,12 @@ class CGIHTTPServerTestCase(BaseTestCase): self.assertEquals(('Hello World\n', 'text/html', 200), \ (res.read(), res.getheader('Content-type'), res.status)) + def test_no_leading_slash(self): + # http://bugs.python.org/issue2254 + res = self.request('cgi-bin/file1.py') + self.assertEquals(('Hello World\n', 'text/html', 200), + (res.read(), res.getheader('Content-type'), res.status)) + def test_main(verbose=None): try: diff --git a/Misc/NEWS b/Misc/NEWS index 9e875c92ecb..78b37cc797b 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -216,6 +216,9 @@ Core and Builtins Library ------- +- Issue #2254: Fix CGIHTTPServer information disclosure. Relative paths are + now collapsed within the url properly before looking in cgi_directories. + - Issue #5095: Added bdist_msi to the list of bdist supported formats. Initial fix by Steven Bethard.