fix(security): harden XML loading against XXE and billion-laughs attacks

[KHDHaDi]

Invoice XML (ZUGFeRD/Factur-X/XRechnung), including XML extracted from third-party PDFs, was loaded via new XmlDocument(); doc.Load(stream) without explicit hardening. While external entity resolution is disabled by default on the targeted frameworks, DTD processing was not prohibited, leaving the door open to entity-expansion DoS ("billion laughs") and to regressions in XXE protection.

All paths that read untrusted XML now reject DTDs entirely (DtdProcessing.Prohibit) and use XmlResolver = null, which closes both XXE (local file disclosure, SSRF) and billion-laughs in one step. Valid invoices contain no DTD and are unaffected.

• ZUGFeRD/XmlSecurityHelper.cs: new central helper for hardened XML loading (DtdProcessing.Prohibit, XmlResolver = null, MaxCharactersFromEntities); leaves the caller-owned stream open.
• InvoiceDescriptor1Reader / 20Reader / 22UblReader / 23CIIReader: Load() now uses XmlSecurityHelper.LoadSecureDocument instead of XmlDocument.Load.
• PDF-embedded XML is covered as well, since InvoiceDescriptorPdfLoader delegates to InvoiceDescriptor.Load.
• ZUGFeRD.Test/XmlSecurityTests.cs: tests for external entity, external DTD and billion-laughs (all rejected) plus a legitimate-invoice round-trip that still succeeds.

Version auto-detection (_IsReadableByThisReaderVersion) does string matching only and parses no XML, so it is not an additional attack path.

Generated with Claude

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.4% Duplication on New Code

See analysis details on SonarQube Cloud

[stephanstapel]

Thanks for submitting this.
Is there a special reason why you are submitting this pull request?

It looks quite bloated just to set:

new XmlReaderSettings
{
  // Reject any DTD. This single setting blocks both external DTD/entity
  // resolution (XXE) and internal entity expansion (billion laughs).
  DtdProcessing = DtdProcessing.Prohibit,  
  // Belt-and-suspenders: never resolve external resources.
  XmlResolver = null,
  // Hard cap on characters produced from entity expansion. With DtdProcessing.Prohibit
  // no custom entities can exist, but this stays as defense-in-depth.
  MaxCharactersFromEntities = 1024
};

[KHDHaDi]

Hi,

I’ve been using your library for several years. Recently, a friend told me that their system experienced a deadlock caused by a malformed ZUGFeRD invoice. They tested their system by injecting this malformed invoice, so I started checking your library as well.

I had some trouble building it with Rider, so I asked Claude to take a look and fix it. The result is admittedly a bit bloated, but the important part is this:

DtdProcessing = DtdProcessing.Prohibit

This should prevent the deadlock.

I also liked the test cases, so I decided to share everything. You are completely free to delete the PR and implement the fix in a different way. I mainly wanted to demonstrate that a deadlock is possible when such a malformed ZUGFeRD invoice is processed.

Thank you very much for your work!

[KHDHaDi]

Just for Information, and I think you understand german: https://www.golem.de/news/zugferd-xrechnung-und-co-wie-elektronische-rechnungen-zum-sicherheitsrisiko-werden-2512-202959.html

[stephanstapel]

Thanks. I will take care of this but probably choose a different implementation. Thinking about wrapping XmlDocument (not simpler than your solution though).