From d3f97883ebb2a87d69c1e056b938f4559032cdd9 Mon Sep 17 00:00:00 2001 From: Correctover Date: Wed, 15 Jul 2026 18:11:10 +0800 Subject: [PATCH] fix: add XXE protection for XML parsing (issue #617) Add _xml_safety module with safe_fromstring() and safe_parse() functions that detect and block XML External Entity (XXE) attack patterns before parsing. Changes: - panos/_xml_safety.py: New module with XXE protection - panos/userid.py: Replace ET.fromstring() with safe_fromstring() - tests/test_xml_safety.py: 6 test cases covering safe/unsafe XML Defense-in-depth for Python < 3.11 (expat 2.6.0+ has built-in protection). Backward compatible - normal XML parsing unaffected. Resolves #617 (Finding 1: Unsafe XML parser) --- panos/_xml_safety.py | 118 +++++++++++++++++++++++++++++++++++++++ panos/userid.py | 5 +- tests/test_xml_safety.py | 68 ++++++++++++++++++++++ 3 files changed, 189 insertions(+), 2 deletions(-) create mode 100644 panos/_xml_safety.py create mode 100644 tests/test_xml_safety.py diff --git a/panos/_xml_safety.py b/panos/_xml_safety.py new file mode 100644 index 00000000..8c4ddffd --- /dev/null +++ b/panos/_xml_safety.py @@ -0,0 +1,118 @@ +""" +Safe XML parsing utilities for pan-os-python. + +This module provides XML parsing functions that are protected against +XML External Entity (XXE) attacks. It addresses security findings +related to unsafe XML parser usage (see issue #617). + +Note: Python 3.11+ includes expat 2.6.0+ which has built-in XXE protection. +However, pan-os-python supports Python 2.7 and 3.5+, so this module +provides defense-in-depth for older Python versions. +""" + +from __future__ import absolute_import +import re +import sys +import xml.etree.ElementTree as ET + + +# Regex patterns for detecting dangerous XML constructs +_ENTITY_PATTERN = re.compile( + r']*>', + re.IGNORECASE | re.DOTALL +) +_DOCTYPE_PATTERN = re.compile( + r']*\[', # DOCTYPE with internal subset + re.IGNORECASE | re.DOTALL +) + + +def _check_xml_safety(xml_string): + """ + Check if an XML string contains potentially dangerous constructs. + + Args: + xml_string: The XML string to check + + Raises: + ValueError: If the XML contains dangerous constructs (entities, DOCTYPE with internal subset) + """ + if not isinstance(xml_string, str): + # Handle bytes + if isinstance(xml_string, bytes): + xml_string = xml_string.decode('utf-8', errors='ignore') + else: + return # Skip non-string types + + if _ENTITY_PATTERN.search(xml_string): + raise ValueError( + "XML contains ENTITY declarations which are not allowed. " + "This may indicate an XXE attack attempt." + ) + + if _DOCTYPE_PATTERN.search(xml_string): + raise ValueError( + "XML contains DOCTYPE with internal subset which is not allowed. " + "This may indicate an XXE attack attempt." + ) + + +def safe_fromstring(xml_string, *args, **kwargs): + """ + Safely parse an XML string, protecting against XXE attacks. + + This is a drop-in replacement for xml.etree.ElementTree.fromstring() + that includes XXE protection for Python versions < 3.11. + + Args: + xml_string: The XML string to parse + *args, **kwargs: Additional arguments passed to ET.fromstring() + + Returns: + Element: The parsed XML element + + Raises: + ValueError: If the XML contains dangerous constructs + """ + _check_xml_safety(xml_string) + return ET.fromstring(xml_string, *args, **kwargs) + + +def safe_parse(source, *args, **kwargs): + """ + Safely parse an XML file or file-like object, protecting against XXE attacks. + + This is a drop-in replacement for xml.etree.ElementTree.parse() + that includes XXE protection for Python versions < 3.11. + + Args: + source: File path or file-like object + *args, **kwargs: Additional arguments passed to ET.parse() + + Returns: + ElementTree: The parsed XML tree + + Raises: + ValueError: If the XML contains dangerous constructs + """ + # For file-like objects, we need to read and check content + if hasattr(source, 'read'): + content = source.read() + _check_xml_safety(content) + # Reset file position if possible + if hasattr(source, 'seek'): + source.seek(0) + # Create a new StringIO/StringBytes object + if isinstance(content, bytes): + from io import BytesIO + source = BytesIO(content) + else: + from io import StringIO + source = StringIO(content) + else: + # File path - read and check + with open(source, 'r') as f: + content = f.read() + _check_xml_safety(content) + + return ET.parse(source, *args, **kwargs) diff --git a/panos/userid.py b/panos/userid.py index 675b269e..7a82a8ce 100644 --- a/panos/userid.py +++ b/panos/userid.py @@ -18,6 +18,7 @@ """User-ID and Dynamic Address Group updates using the User-ID API""" import xml.etree.ElementTree as ET +from panos._xml_safety import safe_fromstring from copy import deepcopy from xml.sax.saxutils import escape, quoteattr @@ -62,7 +63,7 @@ def __init__(self, device, prefix="", ignore_dup_errors=True): self.ignore_dup_errors = ignore_dup_errors # Build the initial uid-message - self._uidmessage = ET.fromstring( + self._uidmessage = safe_fromstring( "" + "1.0" + "update" @@ -658,7 +659,7 @@ def get_user_tags(self, user=None, prefix=None): msg.append("{0}".format(escape(user))) msg.append("") - cmd = ET.fromstring("".join(msg)) + cmd = safe_fromstring("".join(msg)) if user is None: start_elm = cmd.find("./object/registered-user/all/start-point") diff --git a/tests/test_xml_safety.py b/tests/test_xml_safety.py new file mode 100644 index 00000000..5c61b9d3 --- /dev/null +++ b/tests/test_xml_safety.py @@ -0,0 +1,68 @@ +"""Tests for XML safety utilities.""" + +import pytest +from panos._xml_safety import safe_fromstring, _check_xml_safety + + +class TestXmlSafety: + """Test XML safety checks.""" + + def test_safe_xml(self): + """Test that safe XML parses correctly.""" + xml = "text" + result = safe_fromstring(xml) + assert result.tag == "root" + assert result.find("child").text == "text" + + def test_xxe_entity_blocked(self): + """Test that XML with ENTITY declarations is blocked.""" + malicious_xml = """ + + ]> + &xxe;""" + + with pytest.raises(ValueError, match="ENTITY declarations"): + safe_fromstring(malicious_xml) + + def test_xxe_doctype_blocked(self): + """Test that XML with DOCTYPE internal subset is blocked.""" + malicious_xml = """ + + ]> + text""" + + with pytest.raises(ValueError, match="DOCTYPE with internal subset"): + safe_fromstring(malicious_xml) + + def test_normal_doctype_allowed(self): + """Test that DOCTYPE without internal subset is allowed.""" + xml = 'text' + result = safe_fromstring(xml) + assert result.tag == "root" + + def test_bytes_input(self): + """Test that bytes input is handled correctly.""" + xml = b"text" + result = safe_fromstring(xml) + assert result.tag == "root" + + def test_uid_message_format(self): + """Test that pan-os-python's uid-message format works.""" + # This is the actual format used in userid.py + xml = ( + "" + "1.0" + "update" + "" + "" + ) + result = safe_fromstring(xml) + assert result.tag == "uid-message" + assert result.find("version").text == "1.0" + assert result.find("type").text == "update" + + +if __name__ == "__main__": + pytest.main([__file__, "-v"])