feat: add optional defusedxml backend for XML parsing

[pmolfese]

WIP - Work in progress

Adds mffpy.set_backend() to allow switching the XML parsing backend
from lxml (default) to defusedxml, which guards against XML security
vulnerabilities.

• Default remains lxml with recover=True behavior unchanged
• mffpy.set_backend('defusedxml') opts in to defusedxml parsing
• set_backend('defusedxml') raises ImportError with a clear message
  if defusedxml is not installed (pip install defusedxml)
• Calling XML.from_file(..., recover=True) with the defusedxml backend
  emits a UserWarning (recover is silently ignored, since defusedxml
  does not support it)
• If defusedxml raises a ParseError, the exception message includes a
  hint to switch back to lxml for broken-but-recoverable files
• pytest --xml-backend=defusedxml runs the full test suite under the
  defusedxml backend; lxml-specific tests (recover=True behavior) are
  skipped automatically

[pmolfese]

I thought it would be worth it to start discussion about this with @damian5710 as our local BEL representative and then the folks on the MNE side of things: @larsoner @drammock @scott-huberty about this possibility of a patch to allow defusedxml as XML parser. It's a fairly clean modification to allow defusedxml to be used instead of the current of the current lxml for XML reading (both in Reader() and in XML.from_file()).

That said... I'm convinced it's not necessary given that lxml has many protections from the likely XML exploits (XXE, Billion laughs / entity expansion, or DTD fetching) since mffpy's use of lxml currently has no options that would allow them to run enabled (e.g., no resolve_entities, load_dtd, etc.).

I think we're all against complexity without a direct need. The questions in my mind are:

1. is this necessary for MNE to let mffpy be an (optional) dependency
2. are the threats warranting the use of defusedxml warranted here given that lxml is not stdlib and the mffpy implementation here is quite secure.
3. long shot: is it worth MNE continuing to use defusedxml when lxml is more full featured, faster, and has the blessing of security from defusedxml maintainers (see: https://github.com/tiran/defusedxml/blob/main/README.txt)

lxml is safe against most attack scenarios. lxml uses ``libxml2`` for
parsing XML. The library has builtin mitigations against billion laughs and
quadratic blowup attacks. The parser allows a limit amount of entity
expansions, then fails. lxml also disables network access by default.
libxml2 `lxml FAQ`_ lists additional recommendations for safe parsing,
for example counter measures against compression bombs.

My vote would be that this is unnecessary. Separately I do think we should change the mffpy write() functions to use lxml as well. But that's a separate PR for a future me.

[drammock]

Thanks for starting the conversation @pmolfese. TL;DR: so far to me there's no clear/obvious best path. Below I say what my preferences are but I'm open to further discussion / willing to change my mind.

For reference, MNE's switch from standard library's xml module to defusedxml was triggered by a bandit scan; at the time, both bandit and the stdlibrary docs recommended defusedxml as the preferred fix (that has since changed). Having just (briefly) looked into the current state of things, it seems like:

• defusedxml isn't very actively maintained, though since stdlib's xml isn't changing, there's likely little need for defusedxml to change with it. Last defusedxml commit was late 2023, to update testing to use py3.13
• lxml is very fast (Cython) and actively maintained. Nowadays it can be just as secure as defusedxml, but not all its defaults are secure 1 2.
• whether Python stdlib xml module is secure depends on the version of the C library libexpat; on linux, cpython often ends up linked to the system version of libexpat (which may be old/insecure) even though cpython bundles a secure one (presumably for installs where the system library is missing)

On the MNE side, there are competing constraints. On one hand, we're generally interested in shrinking our dependency stack, so if a dependency like mffpy is already using lxml, that's a reason for us to also use lxml (all else being equal). On the other hand, the main vulnerability that MNE users might conceivably encounter is a malicious file posing as a real dataset, which they download and try to open; 1 claims that lxml by default allows entity expansion for local files, which opens the door to insecure usage if MNE or mffpy mess up the implementation.

Do folks have a sense of whether the superior speed of lxml is a relevant consideration? I haven't benchmarked lxml vs defusedxml, but IIUC .mff is a directory-based format where the data samples are in binary files, and the XML is mostly used for metadata? If that's right, I would assume that the XML files involved will always be fairly small and the speed difference would be negligible. That, combined with the not-quite-all-the-way-secure-by-default status of lxml would push me toward the following ranked preferences:

1. MNE sticks with defusedxml, and mffpy switches to use defusedxml too.
2. MNE sticks with defusedxml, and mffpy adds a backend option (lxml or defusedxml)
3. mffpy sticks with lxml, and we check/trust that y'all are using lxml safely. MNE (maybe, eventually) switches to also use lxml
4. mffpy sticks with lxml, and MNE sticks with defusedxml (AKA "do nothing")

If we end up on option 2, it would be great if mffpy also provided a way (e.g., a project.optional_dependencies mechanism in pyproject.toml?) to install mffpy without pulling in lxml. This is not critical but would avoid unnecessary download/install of a package that doesn't get used at runtime.

further reading:

• https://discuss.python.org/t/status-of-defusedxml-and-recommendation-in-docs/34762/21
• https://docs.python.org/3/library/xml.html#xml-security

[pmolfese]

Thanks for that great overview and historical context @drammock! To my surprise, it looks like there's relatively little XML use across the MNE ecosystem! Because of the relatively small XML footprint in MNE, I think my preference would be (Potential choice 3) to keep mffpy on lxml and put effort towards moving MNE towards lxml, a project that I'm willing to put some effort into.

If we go with choices 3 or 4, then mffpy could explicitly attempt (i.e. requires testing) to go even more strict in lxml settings for maximum security, which seems to be:

parser = ET.XMLParser(
    recover=recover, #the default
    resolve_entities=False, #not the default
    load_dtd=False, #the default
    no_network=True, #the default
)

As we wait for anyone else to chime in, I think a quick sub-topic topic to bring up now is that it would make senes for the GSoC project to use the mffpy way of doing things (instead of mixing mffpy and defusedxml), which should be stable no matter what updates happen to mffpy or MNE in the future.

[drammock]

To my surprise, it looks like there's relatively little XML use across the MNE ecosystem!

yep, only a few of the file formats we read directly use XML.

keep mffpy on lxml and put effort towards moving MNE towards lxml, a project that I'm willing to put some effort into.

Let's see what @larsoner thinks. One other point to consider is that the wheel for defusedxml is ~25kB, while lxml is >4MB. So if you don't actually need lxml's runtime speed (because the files are always small) or its more featureful API, there's a case to be made for switching to defusedxml for faster download and import times (on top of the "all defaults are maximally secure" posture taken by the package).

it would make senes for the GSoC project to use the mffpy way of doing things

agreed; the whole point of us switching to use mffpy is to avoid needing to parse the files directly with our own code.

[larsoner]

To me option (3) seems like the most future compatible given the maintenance status of lxml and defusedxml

[pmolfese]

Closing this as consensus seems to be "not needed"