Forbid substrings based on a regexp in name_filter middleware

In comments from https://review.openstack.org/8798 it was raised that it
might make sense to forbid some substrings in the name_filter
middleware.

There is now a new forbidden_regexp option for the name_filter
middleware to specify which substrings to forbid. The default is
"/\./|/\.\./|/\.$|/\.\.$" (or in a non-regexp language: the /./ and /../
substrings as well as strings ending with /. or /..).

This can be useful for extra paranoia to avoid directory traversals
(bug 1005908), or for more general filtering.

Change-Id: I39bf2de45b9dc7d3ca4d350d24b3f2276e958a62
DocImpact: new forbidden_regexp option for the name_filter middleware

doc/manpages/proxy-server.conf.5

@@ -427,6 +427,8 @@ The default is \fBegg:swift#name_check\fR.
 Characters that will not be allowed in a name.
 .IP \fBmaximum_length\fR
 Maximum number of characters that can be in the name.
+.IP \fBforbidden_regexp\fR
+Python regular expressions of substrings that will not be allowed in a name.
 .RE
 
 

etc/proxy-server.conf-sample

@@ -257,6 +257,7 @@ use = egg:swift#formpost
 use = egg:swift#name_check
 # forbidden_chars = '"`<>
 # maximum_length = 255
+# forbidden_regexp = /\./|/\.\./|/\.$|/\.\.$
 
 [filter:proxy-logging]
 use = egg:swift#proxy_logging

swift/common/middleware/name_check.py

@@ -36,6 +36,7 @@ The filter returns HTTPBadRequest if path is invalid.
 @author: eamonn-otoole
 '''
 
+import re
 from swift.common.utils import get_logger
 from webob import Request
 from webob.exc import HTTPBadRequest
@@ -43,6 +44,7 @@ from urllib2 import unquote
 
 FORBIDDEN_CHARS = "\'\"`<>"
 MAX_LENGTH = 255
+FORBIDDEN_REGEXP = "/\./|/\.\./|/\.$|/\.\.$"
 
 
 class NameCheckMiddleware(object):
@@ -53,6 +55,12 @@ class NameCheckMiddleware(object):
         self.forbidden_chars = self.conf.get('forbidden_chars',
                                             FORBIDDEN_CHARS)
         self.maximum_length = self.conf.get('maximum_length', MAX_LENGTH)
+        self.forbidden_regexp = self.conf.get('forbidden_regexp',
+                                              FORBIDDEN_REGEXP)
+        if self.forbidden_regexp:
+            self.forbidden_regexp_compiled = re.compile(self.forbidden_regexp)
+        else:
+            self.forbidden_regexp_compiled = None
         self.logger = get_logger(self.conf, log_route='name_check')
 
     def check_character(self, req):
@@ -84,6 +92,23 @@ class NameCheckMiddleware(object):
         else:
             return False
 
+    def check_regexp(self, req):
+        '''
+        Checks that req.path doesn't contain a substring matching regexps.
+        Returns True if there are any forbidden substring
+        Returns False if there aren't any forbidden substring
+        '''
+        if self.forbidden_regexp_compiled is None:
+            return False
+
+        self.logger.debug("name_check: path %s" % req.path)
+        self.logger.debug("name_check: self.forbidden_regexp %s" %
+                           self.forbidden_regexp)
+
+        unquoted_path = unquote(req.path)
+        match = self.forbidden_regexp_compiled.search(unquoted_path)
+        return (match is not None)
+
     def __call__(self, env, start_response):
         req = Request(env)
 
@@ -95,6 +120,11 @@ class NameCheckMiddleware(object):
             return HTTPBadRequest(request=req,
                body=("Object/Container name longer than the allowed maximum %s"
                % self.maximum_length))(env, start_response)
+        elif self.check_regexp(req):
+            return HTTPBadRequest(request=req,
+               body=("Object/Container name contains a forbidden substring "
+                     "from regular expression %s"
+               % self.forbidden_regexp))(env, start_response)
         else:
             # Pass on to downstream WSGI component
             return self.app(env, start_response)

test/unit/common/middleware/test_name_check.py

@@ -27,6 +27,7 @@ from swift.common.middleware import name_check
 
 MAX_LENGTH = 255
 FORBIDDEN_CHARS = '\'\"<>`'
+FORBIDDEN_REGEXP = "/\./|/\.\./|/\.$|/\.\.$"
 
 
 class FakeApp(object):
@@ -39,7 +40,7 @@ class TestNameCheckMiddleware(unittest.TestCase):
 
     def setUp(self):
         self.conf = {'maximum_length': MAX_LENGTH, 'forbidden_chars':
-                     FORBIDDEN_CHARS}
+                     FORBIDDEN_CHARS, 'forbidden_regexp': FORBIDDEN_REGEXP}
         self.test_check = name_check.filter_factory(self.conf)(FakeApp())
 
     def test_valid_length_and_character(self):
@@ -67,6 +68,24 @@ class TestNameCheckMiddleware(unittest.TestCase):
                     % self.conf['maximum_length']))
         self.assertEquals(resp.status_int, 400)
 
+    def test_invalid_regexp(self):
+        for s in ['/.', '/..', '/./foo', '/../foo']:
+            path = '/V1.0/' + s
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                             ).get_response(self.test_check)
+            self.assertEquals(resp.body,
+                    ("Object/Container name contains a forbidden substring "
+                     "from regular expression %s"
+                     % self.conf['forbidden_regexp']))
+            self.assertEquals(resp.status_int, 400)
+
+    def test_valid_regexp(self):
+        for s in ['/...', '/.\.', '/foo']:
+            path = '/V1.0/' + s
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                             ).get_response(self.test_check)
+            self.assertEquals(resp.body, 'OK')
+
 
 if __name__ == '__main__':
     unittest.main()