From ad5b91b5f5fb956cc7a74e9bcc2c5c022174dd54 Mon Sep 17 00:00:00 2001 From: Aymeric Ducroquetz Date: Tue, 25 Oct 2022 22:07:53 +0200 Subject: [PATCH 2/2] Detect and log attempted XXE injections Change-Id: I1b55770ae678632d8293b7971b8fe9b78c9018d4 Co-Authored-By: Romain de Joux Related-Bug: #1998625 --- swift/common/middleware/s3api/acl_handlers.py | 2 +- swift/common/middleware/s3api/acl_utils.py | 8 ++++---- swift/common/middleware/s3api/controllers/acl.py | 3 ++- swift/common/middleware/s3api/controllers/bucket.py | 2 +- .../middleware/s3api/controllers/multi_delete.py | 2 +- .../middleware/s3api/controllers/multi_upload.py | 2 +- .../middleware/s3api/controllers/versioning.py | 3 ++- swift/common/middleware/s3api/etree.py | 13 +++++++++++++ swift/common/middleware/s3api/s3request.py | 2 +- 9 files changed, 26 insertions(+), 11 deletions(-) diff --git a/swift/common/middleware/s3api/acl_handlers.py b/swift/common/middleware/s3api/acl_handlers.py index 699b38771..99fb5767e 100644 --- a/swift/common/middleware/s3api/acl_handlers.py +++ b/swift/common/middleware/s3api/acl_handlers.py @@ -165,7 +165,7 @@ class BaseAclHandler(object): if not body: raise MissingSecurityHeader(missing_header_name='x-amz-acl') try: - elem = fromstring(body, ACL.root_tag) + elem = fromstring(body, ACL.root_tag, logger=self.logger) acl = ACL.from_elem( elem, True, self.req.conf.allow_no_owner) except(XMLSyntaxError, DocumentInvalid): diff --git a/swift/common/middleware/s3api/acl_utils.py b/swift/common/middleware/s3api/acl_utils.py index b2821a3d2..1dc8a3f8f 100644 --- a/swift/common/middleware/s3api/acl_utils.py +++ b/swift/common/middleware/s3api/acl_utils.py @@ -20,7 +20,7 @@ from swift.common.middleware.s3api.s3response import S3NotImplemented, \ MalformedACLError, InvalidArgument -def swift_acl_translate(acl, group='', user='', xml=False): +def swift_acl_translate(acl, group='', user='', xml=False, logger=None): """ Takes an S3 style ACL and returns a list of header/value pairs that implement that ACL in Swift, or "NotImplemented" if there isn't a way to do @@ -49,7 +49,7 @@ def swift_acl_translate(acl, group='', user='', xml=False): if xml: # We are working with XML and need to parse it try: - elem = fromstring(acl, 'AccessControlPolicy') + elem = fromstring(acl, 'AccessControlPolicy', logger=logger) except (XMLSyntaxError, DocumentInvalid): raise MalformedACLError() acl = 'unknown' @@ -75,7 +75,7 @@ def swift_acl_translate(acl, group='', user='', xml=False): return swift_acl[acl] -def handle_acl_header(req): +def handle_acl_header(req, logger=None): """ Handle the x-amz-acl header. Note that this header currently used for only normal-acl @@ -92,7 +92,7 @@ def handle_acl_header(req): req.query_string = '' try: - translated_acl = swift_acl_translate(amz_acl) + translated_acl = swift_acl_translate(amz_acl, logger=logger) except ACLError: raise InvalidArgument('x-amz-acl', amz_acl) diff --git a/swift/common/middleware/s3api/controllers/acl.py b/swift/common/middleware/s3api/controllers/acl.py index 79e4f9d1b..69fc64cba 100644 --- a/swift/common/middleware/s3api/controllers/acl.py +++ b/swift/common/middleware/s3api/controllers/acl.py @@ -116,7 +116,8 @@ class AclController(Controller): # We very likely have an XML-based ACL request. # let's try to translate to the request header try: - translated_acl = swift_acl_translate(xml, xml=True) + translated_acl = swift_acl_translate( + xml, xml=True, logger=self.logger) except ACLError: raise MalformedACLError() diff --git a/swift/common/middleware/s3api/controllers/bucket.py b/swift/common/middleware/s3api/controllers/bucket.py index c9443fa97..fe89afe5a 100644 --- a/swift/common/middleware/s3api/controllers/bucket.py +++ b/swift/common/middleware/s3api/controllers/bucket.py @@ -372,7 +372,7 @@ class BucketController(Controller): # check location try: elem = fromstring( - xml, 'CreateBucketConfiguration', self.logger) + xml, 'CreateBucketConfiguration', logger=self.logger) location = elem.find('./LocationConstraint').text except (XMLSyntaxError, DocumentInvalid): raise MalformedXML() diff --git a/swift/common/middleware/s3api/controllers/multi_delete.py b/swift/common/middleware/s3api/controllers/multi_delete.py index 17cbb62e9..c43385df4 100644 --- a/swift/common/middleware/s3api/controllers/multi_delete.py +++ b/swift/common/middleware/s3api/controllers/multi_delete.py @@ -78,7 +78,7 @@ class MultiObjectDeleteController(Controller): raise MissingRequestBodyError() req.check_md5(xml) - elem = fromstring(xml, 'Delete', self.logger) + elem = fromstring(xml, 'Delete', logger=self.logger) quiet = elem.find('./Quiet') self.quiet = quiet is not None and quiet.text.lower() == 'true' diff --git a/swift/common/middleware/s3api/controllers/multi_upload.py b/swift/common/middleware/s3api/controllers/multi_upload.py index eff0c50fe..d3126f3ca 100644 --- a/swift/common/middleware/s3api/controllers/multi_upload.py +++ b/swift/common/middleware/s3api/controllers/multi_upload.py @@ -698,7 +698,7 @@ class UploadController(Controller): del req.headers['etag'] complete_elem = fromstring( - xml, 'CompleteMultipartUpload', self.logger) + xml, 'CompleteMultipartUpload', logger=self.logger) for part_elem in complete_elem.iterchildren('Part'): part_number = int(part_elem.find('./PartNumber').text) diff --git a/swift/common/middleware/s3api/controllers/versioning.py b/swift/common/middleware/s3api/controllers/versioning.py index 2d31d2af5..0a0040e9f 100644 --- a/swift/common/middleware/s3api/controllers/versioning.py +++ b/swift/common/middleware/s3api/controllers/versioning.py @@ -63,7 +63,8 @@ class VersioningController(Controller): xml = req.xml(MAX_PUT_VERSIONING_BODY_SIZE) try: - elem = fromstring(xml, 'VersioningConfiguration') + elem = fromstring( + xml, 'VersioningConfiguration', logger=self.logger) status = elem.find('./Status').text except (XMLSyntaxError, DocumentInvalid): raise MalformedXML() diff --git a/swift/common/middleware/s3api/etree.py b/swift/common/middleware/s3api/etree.py index e16b75340..8b5309bc6 100644 --- a/swift/common/middleware/s3api/etree.py +++ b/swift/common/middleware/s3api/etree.py @@ -16,6 +16,7 @@ import lxml.etree from copy import deepcopy from pkg_resources import resource_stream # pylint: disable-msg=E0611 +import re import six from swift.common.utils import get_logger @@ -25,6 +26,8 @@ from swift.common.middleware.s3api.utils import camel_to_snake, \ XMLNS_S3 = 'http://s3.amazonaws.com/doc/2006-03-01/' XMLNS_XSI = 'http://www.w3.org/2001/XMLSchema-instance' +ENTITY_STR_RE = re.compile('', re.DOTALL) +ENTITY_BYTES_RE = re.compile(b'', re.DOTALL) class XMLSyntaxError(S3Exception): @@ -57,6 +60,16 @@ def cleanup_namespaces(elem): def fromstring(text, root_tag=None, logger=None): + if logger: + has_entity = False + if isinstance(text, str): + has_entity = ENTITY_STR_RE.search(text) is not None + elif isinstance(text, bytes): + has_entity = ENTITY_BYTES_RE.search(text) is not None + if has_entity: + logger.warn( + 'The XML contains an entity, it can be an XXE injection') + try: elem = lxml.etree.fromstring(text, parser) except lxml.etree.XMLSyntaxError as e: diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py index 19a8304e1..01d12dea9 100644 --- a/swift/common/middleware/s3api/s3request.py +++ b/swift/common/middleware/s3api/s3request.py @@ -1436,7 +1436,7 @@ class S3Request(swob.Request): """ if 'HTTP_X_AMZ_ACL' in self.environ: - handle_acl_header(self) + handle_acl_header(self, logger=app.logger) return self._get_response(app, method, container, obj, headers, body, query) -- 2.38.1