diff --git a/swift/account/server.py b/swift/account/server.py index fd839a4909..ee55c0556b 100644 --- a/swift/account/server.py +++ b/swift/account/server.py @@ -29,7 +29,7 @@ import simplejson from swift.common.db import AccountBroker from swift.common.utils import get_logger, get_param, hash_path, \ - normalize_timestamp, split_path, storage_directory + normalize_timestamp, split_path, storage_directory, XML_EXTRA_ENTITIES from swift.common.constraints import ACCOUNT_LISTING_LIMIT, \ check_mount, check_float, check_utf8 from swift.common.db_replicator import ReplicatorRpc @@ -79,6 +79,9 @@ class AccountController(object): try: drive, part, account, container = split_path(unquote(req.path), 3, 4) + if (account and not check_utf8(account)) or \ + (container and not check_utf8(container)): + raise ValueError('NULL characters not allowed in names') except ValueError, err: return HTTPBadRequest(body=str(err), content_type='text/plain', request=req) @@ -201,8 +204,8 @@ class AccountController(object): marker = get_param(req, 'marker', '') end_marker = get_param(req, 'end_marker') query_format = get_param(req, 'format') - except UnicodeDecodeError, err: - return HTTPBadRequest(body='parameters not utf8', + except (UnicodeDecodeError, ValueError), err: + return HTTPBadRequest(body='parameters not utf8 or contain NULLs', content_type='text/plain', request=req) if query_format: req.accept = 'application/%s' % query_format.lower() @@ -228,7 +231,7 @@ class AccountController(object): output_list = ['', '' % account] for (name, object_count, bytes_used, is_subdir) in account_list: - name = saxutils.escape(name) + name = saxutils.escape(name, XML_EXTRA_ENTITIES) if is_subdir: output_list.append('' % name) else: diff --git a/swift/common/constraints.py b/swift/common/constraints.py index ad5a37b9a8..1f314ef0bb 100644 --- a/swift/common/constraints.py +++ b/swift/common/constraints.py @@ -159,13 +159,16 @@ def check_float(string): def check_utf8(string): """ - Validate if a string is valid UTF-8. + Validate if a string is valid UTF-8 and has no NULL characters. :param string: string to be validated - :returns: True if the string is valid utf-8, False otherwise + :returns: True if the string is valid utf-8 and has no NULL characters, + False otherwise """ try: string.decode('UTF-8') - return True except UnicodeDecodeError: return False + if '\x00' in string: + return False + return True diff --git a/swift/common/utils.py b/swift/common/utils.py index f95fa4aa96..2629849775 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -42,6 +42,7 @@ from eventlet import greenio, GreenPool, sleep, Timeout, listen from eventlet.green import socket, subprocess, ssl, thread, threading import netifaces +from swift.common.constraints import check_utf8 from swift.common.exceptions import LockTimeout, MessageTimeout # logging doesn't import patched as cleanly as one would like @@ -74,6 +75,8 @@ if hash_conf.read('/etc/swift/swift.conf'): # Used when reading config values TRUE_VALUES = set(('true', '1', 'yes', 'on', 't', 'y')) +# Used with xml.sax.saxutils.escape +XML_EXTRA_ENTITIES = dict((chr(x), '&#x%x;' % x) for x in xrange(1, 0x20)) def validate_configuration(): if HASH_PATH_SUFFIX == '': @@ -110,8 +113,8 @@ def get_param(req, name, default=None): :returns: HTTP request parameter value """ value = req.str_params.get(name, default) - if value: - value.decode('utf8') # Ensure UTF8ness + if value and not check_utf8(value): + raise ValueError('Not valid UTF-8 or contains NULL characters') return value diff --git a/swift/container/server.py b/swift/container/server.py index bc3856d18e..9a7d32399d 100644 --- a/swift/container/server.py +++ b/swift/container/server.py @@ -32,7 +32,7 @@ from webob.exc import HTTPAccepted, HTTPBadRequest, HTTPConflict, \ from swift.common.db import ContainerBroker from swift.common.utils import get_logger, get_param, hash_path, \ - normalize_timestamp, storage_directory, split_path + normalize_timestamp, storage_directory, split_path, XML_EXTRA_ENTITIES from swift.common.constraints import CONTAINER_LISTING_LIMIT, \ check_mount, check_float, check_utf8 from swift.common.bufferedhttp import http_connect @@ -167,6 +167,10 @@ class ContainerController(object): try: drive, part, account, container, obj = split_path( unquote(req.path), 4, 5, True) + if (account and not check_utf8(account)) or \ + (container and not check_utf8(container)) or \ + (obj and not check_utf8(obj)): + raise ValueError('NULL characters not allowed in names') except ValueError, err: return HTTPBadRequest(body=str(err), content_type='text/plain', request=req) @@ -277,7 +281,7 @@ class ContainerController(object): return HTTPPreconditionFailed(request=req, body='Maximum limit is %d' % CONTAINER_LISTING_LIMIT) query_format = get_param(req, 'format') - except UnicodeDecodeError, err: + except (UnicodeDecodeError, ValueError), err: return HTTPBadRequest(body='parameters not utf8', content_type='text/plain', request=req) if query_format: @@ -312,21 +316,23 @@ class ContainerController(object): xml_output = [] for (name, created_at, size, content_type, etag) in container_list: # escape name and format date here - name = saxutils.escape(name) + name = saxutils.escape(name, XML_EXTRA_ENTITIES) created_at = datetime.utcfromtimestamp( float(created_at)).isoformat() if content_type is None: xml_output.append('%s' '' % (name, name)) else: - content_type = saxutils.escape(content_type) + content_type = saxutils.escape(content_type, + XML_EXTRA_ENTITIES) xml_output.append('%s%s'\ '%d%s'\ '%s' % \ (name, etag, size, content_type, created_at)) container_list = ''.join([ '\n', - '' % saxutils.quoteattr(container), + '' % + saxutils.quoteattr(container, XML_EXTRA_ENTITIES), ''.join(xml_output), '']) else: if not container_list: diff --git a/test/functionalnosetests/test_account.py b/test/functionalnosetests/test_account.py index 4b5da32da1..02f48e62ba 100755 --- a/test/functionalnosetests/test_account.py +++ b/test/functionalnosetests/test_account.py @@ -2,6 +2,7 @@ import unittest from nose import SkipTest +from uuid import uuid4 from swift.common.constraints import MAX_META_COUNT, MAX_META_NAME_LENGTH, \ MAX_META_OVERALL_SIZE, MAX_META_VALUE_LENGTH @@ -132,6 +133,32 @@ class TestAccount(unittest.TestCase): resp.read() self.assertEquals(resp.status, 400) + def test_name_control_chars(self): + if skip: + raise SkipTest + + container = uuid4().hex + + def put(url, token, parsed, conn): + conn.request('PUT', '%s/%s%%01test' % + (parsed.path, container), '', + {'X-Auth-Token': token, 'Content-Length': '0'}) + return check_response(conn) + + resp = retry(put) + resp.read() + self.assertTrue(resp.status in (201, 202)) + + def get(url, token, parsed, conn): + conn.request('GET', '%s?format=xml' % (parsed.path,), '', + {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(get) + body = resp.read() + self.assertEquals(resp.status, 200) + self.assertTrue('%stest' % (container,) in body) + if __name__ == '__main__': unittest.main() diff --git a/test/functionalnosetests/test_container.py b/test/functionalnosetests/test_container.py index 738231e02b..a89b224899 100755 --- a/test/functionalnosetests/test_container.py +++ b/test/functionalnosetests/test_container.py @@ -522,6 +522,50 @@ class TestContainer(unittest.TestCase): resp.read() self.assertEquals(resp.status, 201) + def test_name_control_chars(self): + if skip: + raise SkipTest + + def put(url, token, parsed, conn): + conn.request('PUT', '%s/%s%%00test' % (parsed.path, self.name), '', + {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(put) + resp.read() + # NULLs not allowed + self.assertEquals(resp.status, 412) + + def put(url, token, parsed, conn): + conn.request('PUT', '%s/%s%%01test' % (parsed.path, self.name), '', + {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(put) + resp.read() + # 0x01 allowed + self.assertTrue(resp.status in (201, 202)) + + def put(url, token, parsed, conn): + conn.request('PUT', '%s/%s/object%%01test' % + (parsed.path, self.name), '', + {'X-Auth-Token': token, 'Content-Length': '0'}) + return check_response(conn) + + resp = retry(put) + resp.read() + self.assertTrue(resp.status in (201, 202)) + + def get(url, token, parsed, conn): + conn.request('GET', '%s/%s?format=xml' % (parsed.path, self.name), + '', {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(get) + body = resp.read() + self.assertEquals(resp.status, 200) + self.assertTrue('objecttest' in body) + if __name__ == '__main__': unittest.main() diff --git a/test/functionalnosetests/test_object.py b/test/functionalnosetests/test_object.py index 5975cf16a2..73bfae2f5e 100644 --- a/test/functionalnosetests/test_object.py +++ b/test/functionalnosetests/test_object.py @@ -541,6 +541,30 @@ class TestObject(unittest.TestCase): resp.read() self.assertEquals(resp.status, 204) + def test_name_control_chars(self): + if skip: + raise SkipTest + + def put(url, token, parsed, conn): + conn.request('PUT', '%s/%s/obj%%00test' % (parsed.path, + self.container), 'test', {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(put) + resp.read() + # NULLs not allowed + self.assertEquals(resp.status, 412) + + def put(url, token, parsed, conn): + conn.request('PUT', '%s/%s/obj%%01test' % (parsed.path, + self.container), 'test', {'X-Auth-Token': token}) + return check_response(conn) + + resp = retry(put) + resp.read() + # 0x01 allowed + self.assertEquals(resp.status, 201) + if __name__ == '__main__': unittest.main() diff --git a/test/unit/account/test_server.py b/test/unit/account/test_server.py index 238b7f3d18..cbc89395f1 100644 --- a/test/unit/account/test_server.py +++ b/test/unit/account/test_server.py @@ -22,6 +22,7 @@ from StringIO import StringIO import simplejson import xml.dom.minidom from webob import Request +from xml.parsers.expat import ExpatError from swift.account.server import AccountController, ACCOUNT_LISTING_LIMIT from swift.common.utils import normalize_timestamp @@ -450,7 +451,8 @@ class TestAccountController(unittest.TestCase): 'X-Bytes-Used': '0', 'X-Timestamp': normalize_timestamp(0)}) self.controller.PUT(req) - req = Request.blank('/sda1/p/a/c2', environ={'REQUEST_METHOD': 'PUT'}, + req = Request.blank('/sda1/p/a/c2%04', + environ={'REQUEST_METHOD': 'PUT'}, headers={'X-Put-Timestamp': '2', 'X-Delete-Timestamp': '0', 'X-Object-Count': '0', @@ -462,7 +464,15 @@ class TestAccountController(unittest.TestCase): resp = self.controller.GET(req) self.assertEquals(resp.content_type, 'application/xml') self.assertEquals(resp.status_int, 200) - dom = xml.dom.minidom.parseString(resp.body) + try: + dom = xml.dom.minidom.parseString(resp.body) + except ExpatError, err: + # Expat doesn't like control characters, which are XML 1.1 + # compatible. Soooo, we have to replace them. We'll do a specific + # replace in this case, but real code that uses Expat will need + # something more resilient. + dom = xml.dom.minidom.parseString( + resp.body.replace('', '\\x04')) self.assertEquals(dom.firstChild.nodeName, 'account') listing = \ [n for n in dom.firstChild.childNodes if n.nodeName != '#text'] @@ -483,7 +493,7 @@ class TestAccountController(unittest.TestCase): self.assertEquals(sorted([n.nodeName for n in container]), ['bytes', 'count', 'name']) node = [n for n in container if n.nodeName == 'name'][0] - self.assertEquals(node.firstChild.nodeValue, 'c2') + self.assertEquals(node.firstChild.nodeValue, 'c2\\x04') node = [n for n in container if n.nodeName == 'count'][0] self.assertEquals(node.firstChild.nodeValue, '0') node = [n for n in container if n.nodeName == 'bytes'][0] @@ -495,7 +505,8 @@ class TestAccountController(unittest.TestCase): 'X-Bytes-Used': '2', 'X-Timestamp': normalize_timestamp(0)}) self.controller.PUT(req) - req = Request.blank('/sda1/p/a/c2', environ={'REQUEST_METHOD': 'PUT'}, + req = Request.blank('/sda1/p/a/c2%04', + environ={'REQUEST_METHOD': 'PUT'}, headers={'X-Put-Timestamp': '2', 'X-Delete-Timestamp': '0', 'X-Object-Count': '3', @@ -506,7 +517,15 @@ class TestAccountController(unittest.TestCase): environ={'REQUEST_METHOD': 'GET'}) resp = self.controller.GET(req) self.assertEquals(resp.status_int, 200) - dom = xml.dom.minidom.parseString(resp.body) + try: + dom = xml.dom.minidom.parseString(resp.body) + except ExpatError, err: + # Expat doesn't like control characters, which are XML 1.1 + # compatible. Soooo, we have to replace them. We'll do a specific + # replace in this case, but real code that uses Expat will need + # something more resilient. + dom = xml.dom.minidom.parseString( + resp.body.replace('', '\\x04')) self.assertEquals(dom.firstChild.nodeName, 'account') listing = \ [n for n in dom.firstChild.childNodes if n.nodeName != '#text'] @@ -526,7 +545,7 @@ class TestAccountController(unittest.TestCase): self.assertEquals(sorted([n.nodeName for n in container]), ['bytes', 'count', 'name']) node = [n for n in container if n.nodeName == 'name'][0] - self.assertEquals(node.firstChild.nodeValue, 'c2') + self.assertEquals(node.firstChild.nodeValue, 'c2\\x04') node = [n for n in container if n.nodeName == 'count'][0] self.assertEquals(node.firstChild.nodeValue, '3') node = [n for n in container if n.nodeName == 'bytes'][0] @@ -959,6 +978,35 @@ class TestAccountController(unittest.TestCase): resp = self.controller.GET(req) self.assert_(resp.status_int in (204, 412), resp.status_int) + def test_params_no_null(self): + self.controller.PUT(Request.blank('/sda1/p/a', + headers={'X-Timestamp': normalize_timestamp(1)}, + environ={'REQUEST_METHOD': 'PUT'})) + for param in ('delimiter', 'format', 'limit', 'marker', + 'prefix'): + req = Request.blank('/sda1/p/a?%s=\x00' % param, + environ={'REQUEST_METHOD': 'GET'}) + resp = self.controller.GET(req) + self.assertEquals(resp.status_int, 400) + + def test_PUT_account_no_null(self): + req = Request.blank('/sda1/p/test\x00test', + environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'}) + resp = self.controller.PUT(req) + self.assertEquals(resp.status_int, 400) + + def test_PUT_container_no_null(self): + req = Request.blank('/sda1/p/a', + environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'}) + resp = self.controller.PUT(req) + self.assertEquals(resp.status_int, 201) + req = Request.blank('/sda1/p/a/test\x00test', + environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_PUT_TIMESTAMP': '1', + 'HTTP_X_DELETE_TIMESTAMP': '0', + 'HTTP_X_OBJECT_COUNT': '0', 'HTTP_X_BYTES_USED': '0'}) + resp = self.controller.PUT(req) + self.assertEquals(resp.status_int, 400) + if __name__ == '__main__': unittest.main() diff --git a/test/unit/container/test_server.py b/test/unit/container/test_server.py index 117a0dccd5..7ef0cee5c3 100644 --- a/test/unit/container/test_server.py +++ b/test/unit/container/test_server.py @@ -246,6 +246,24 @@ class TestContainerController(unittest.TestCase): resp = self.controller.PUT(req) self.assertEquals(resp.status_int, 201) + def test_PUT_container_no_null(self): + req = Request.blank('/sda1/p/a/test\x00test', + environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'}) + resp = self.controller.PUT(req) + self.assertEquals(resp.status_int, 400) + + def test_PUT_object_no_null(self): + req = Request.blank('/sda1/p/a/test', + environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'}) + resp = self.controller.PUT(req) + self.assertEquals(resp.status_int, 201) + req = Request.blank('/sda1/p/a/test/test\x00test', + environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1', + 'HTTP_X_SIZE': '0', 'HTTP_X_CONTENT_TYPE': 'text/plain', + 'HTTP_X_ETAG': 'd41d8cd98f00b204e9800998ecf8427e'}) + resp = self.controller.PUT(req) + self.assertEquals(resp.status_int, 400) + def test_PUT_account_update(self): bindsock = listen(('127.0.0.1', 0)) def accept(return_code, expected_timestamp): @@ -582,25 +600,25 @@ class TestContainerController(unittest.TestCase): resp = self.controller.PUT(req) # fill the container for i in range(3): - req = Request.blank('/sda1/p/a/xmlc/%s'%i, environ= - {'REQUEST_METHOD': 'PUT', - 'HTTP_X_TIMESTAMP': '1', - 'HTTP_X_CONTENT_TYPE': 'text/plain', - 'HTTP_X_ETAG': 'x', - 'HTTP_X_SIZE': 0}) + req = Request.blank('/sda1/p/a/xmlc/%s%%%02x' % (i, i + 1), + environ={'REQUEST_METHOD': 'PUT', + 'HTTP_X_TIMESTAMP': '1', + 'HTTP_X_CONTENT_TYPE': 'text/plain', + 'HTTP_X_ETAG': 'x', + 'HTTP_X_SIZE': 0}) resp = self.controller.PUT(req) self.assertEquals(resp.status_int, 201) xml_body = '\n' \ '' \ - '0x0' \ + '0x0' \ 'text/plain' \ '1970-01-01T00:00:01' \ '' \ - '1x0' \ + '1x0' \ 'text/plain' \ '1970-01-01T00:00:01' \ '' \ - '2x0' \ + '2x0' \ 'text/plain' \ '1970-01-01T00:00:01' \ '' \ @@ -822,6 +840,17 @@ class TestContainerController(unittest.TestCase): resp = self.controller.GET(req) self.assert_(resp.status_int in (204, 412), resp.status_int) + def test_params_no_null(self): + self.controller.PUT(Request.blank('/sda1/p/a/c', + headers={'X-Timestamp': normalize_timestamp(1)}, + environ={'REQUEST_METHOD': 'PUT'})) + for param in ('delimiter', 'format', 'limit', 'marker', 'path', + 'prefix'): + req = Request.blank('/sda1/p/a/c?%s=\x00' % param, + environ={'REQUEST_METHOD': 'GET'}) + resp = self.controller.GET(req) + self.assertEquals(resp.status_int, 400) + if __name__ == '__main__': unittest.main() diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 604ca630da..025db60887 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -2111,6 +2111,20 @@ class TestObjectController(unittest.TestCase): exp = 'HTTP/1.1 412' self.assertEquals(headers[:len(exp)], exp) + def test_chunked_put_bad_utf8_null(self): + # Check invalid utf-8 + (prolis, acc1lis, acc2lis, con2lis, con2lis, obj1lis, obj2lis) = \ + _test_sockets + sock = connect_tcp(('localhost', prolis.getsockname()[1])) + fd = sock.makefile() + fd.write('GET /v1/a%00 HTTP/1.1\r\nHost: localhost\r\n' + 'Connection: close\r\nX-Auth-Token: t\r\n' + 'Content-Length: 0\r\n\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = 'HTTP/1.1 412' + self.assertEquals(headers[:len(exp)], exp) + def test_chunked_put_bad_path_no_controller(self): # Check bad path, no controller (prolis, acc1lis, acc2lis, con2lis, con2lis, obj1lis, obj2lis) = \