Skip to content

security: use defusedxml to protect against XML attacks - #4094

Merged
tonioo merged 1 commit into
modoboa:masterfrom
AndrianBalanescu:security/use-defusedxml-1556
Jul 10, 2026
Merged

security: use defusedxml to protect against XML attacks#4094
tonioo merged 1 commit into
modoboa:masterfrom
AndrianBalanescu:security/use-defusedxml-1556

Conversation

@AndrianBalanescu

Copy link
Copy Markdown
Contributor

Description

Closes #1556

This PR replaces unsafe XML parsing with hardened equivalents to protect against XXE (XML External Entity), billion laughs, and other XML decoding attacks.

Changes

modoboa/autoconfig/views.py

  • Replaced import xml.etree.ElementTree as ET with from defusedxml.ElementTree import fromstring
  • The _email_address_from_xml_body() function parses Outlook Autodiscover XML from untrusted HTTP POST bodies — this is a direct attack surface
  • Updated exception handling to catch both ParseError (malformed XML) and ValueError (defusedxml security exceptions like EntitiesForbidden, DTDFForbidden)

modoboa/contacts/lib/carddav.py

  • Added a module-level _SECURE_PARSER = ET.XMLParser(resolve_entities=False, no_network=True) and a _safe_xml() wrapper
  • Replaced both ET.XML(xml) calls (in __process_report_result and _process_xml_props) with _safe_xml(xml)
  • Used lxml's built-in parser hardening rather than defusedxml.lxml (which is deprecated and will be removed in a future release)

Why not defusedxml.lxml?

defusedxml.lxml is deprecated per its own documentation. The lxml-recommended approach is to configure XMLParser with resolve_entities=False and no_network=True, which provides the same protections while keeping full lxml API compatibility (the code relies on iterchildren()/iterdescendants() which are lxml-specific).

No new dependencies

defusedxml is already listed in pyproject.toml (defusedxml>=0.6.0) and is already used in modoboa/dmarc/lib.py.

Tests

modoboa/autoconfig/tests.py

  • test_autodiscover_xxe_entity_not_resolved: POSTs an XML body with a <!ENTITY> declaration inside EMailAddress — verifies the entity is not resolved and the response is 404

modoboa/contacts/tests.py

  • CardDavSecurityTestCase.test_safe_xml_rejects_entity_expansion: verifies &xxe; entity reference is not expanded (text is None)
  • CardDavSecurityTestCase.test_safe_xml_parses_valid_document: verifies valid DAV XML still parses correctly with the secure parser

Replace unsafe xml.etree.ElementTree with defusedxml.ElementTree in
autoconfig/views.py to block entity expansion (XXE) and other XML
decoding attacks. For carddav.py, configure lxml with a hardened
XMLParser (resolve_entities=False, no_network=True) since defusedxml.lxml
is deprecated.

defusedxml is already a dependency (used in dmarc/lib.py) so no new
packages are required.

Tests added:
- autoconfig: XXE entity in EMailAddress is not resolved (404)
- carddav: entity expansion blocked, valid XML still parses
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.43%. Comparing base (6f0ff74) to head (1b936c9).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4094      +/-   ##
==========================================
+ Coverage   84.42%   84.43%   +0.01%     
==========================================
  Files         324      324              
  Lines       16382    16385       +3     
  Branches     2183     2183              
==========================================
+ Hits        13830    13835       +5     
+ Misses       1840     1838       -2     
  Partials      712      712              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tonioo
tonioo merged commit 2cacb47 into modoboa:master Jul 10, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make use of defusedxml

4 participants