GeezerVison

I plan to also expose the billion laugh mitigation API, but in a separate PR.

@picnixz thanks for taking on this project — very much appreciated! 🙏

@hartwork I observed that the maximum allocation factor is only checked if we exceed the activation threshold. The online docs for Expat didn't explicitly mention it so I mentioned it in our docs.

@picnixz there is mention of the activation threshold in the final note of https://libexpat.github.io/doc/api/latest/#XML_SetAllocTrackerMaximumAmplification but I'm happy if the topic gets more attention in CPython docs.

I already started review and spotted a few things. I will likely submit a first round of review some time later today.

there is mention of the activation threshold in the final note of

I assume you are actually talking about this:

So if you do reduce the maximum allowed amplification, please make sure that the activation threshold is still big enough to not end up with undesired false positives (i.e. benign files being rejected).

I actually understood it only as "if you use a smaller factor then in practice, you might want to change the activation threshold" but I didn't understand it as "the amplification factor is only used if we exceed the threshold in the first place". I assumed from the note on XML_SetAllocTrackerActivationThreshold that the two values were totally independent and could have two different protections:

Note: For types of allocations that intentionally bypass tracking and limiting, please see XML_SetAllocTrackerMaximumAmplification above.

I already started review and spotted a few things. I will likely submit a first round of review some time later today.

Thank you! I'll hold the work on the billion laugh API as it'll be easier to add the API once this one is merged.

I assume you are actually talking about this:

So if you do reduce the maximum allowed amplification, please make sure that the activation threshold is still big enough to not end up with undesired false positives (i.e. benign files being rejected).

@picnixz I confirm, yes, exactly 👍

I actually understood it only as "if you use a smaller factor then in practice, you might want to change the activation threshold" but I didn't understand it as "the amplification factor is only used if we exceed the threshold in the first place". I assumed from the note on XML_SetAllocTrackerActivationThreshold that the two values were totally independent and could have two different protections:

I see. I think that means that upstream docs should be adjusted some way to make it easier to read that (or them) as interconnected. How about I try a related pull request upstream and you take the review seat there?

I'll hold the work on the billion laugh API as it'll be easier to add the API once this one is merged.

Understood, good plan 👍

@picnixz I went to past related pull requests…

• gh-115398: Expose Expat >=2.6.0 reparse deferral API (CVE-2023-52425) #115623

• gh-115398: Suggest use of hasattr with checking for 3.13 Expat API availability #116278

• gh-115399: Document CVE-2023-52425 under "XML vulnerabilities" #115400

…to look for open questions or things that may be missing still here. (That exposed the missing version checks in the new tests as already commented above.) And it gave me these three open questions:

• Do the sax and etree parsers using Expat need new knobs for this and if so, does it need to be in here? The related classes are:

  • xml.etree.ElementTree.XMLParser
  • xml.etree.ElementTree.XMLPullParser
  • xml.sax.expatreader.ExpatParser

• Is extending the docs for CVE-2025-59375 needed and if so, does it have to be in here?

  • Should the docs suggest getattr hasattr for the new method added and should it be done in here?

What do you think?

@picnixz any ideas about what to do about xml.sax and xml.etree?

Regarding hasattr, I can look into a pull request like #116278 if you want (or be in the review seat)?

any ideas about what to do about xml.sax and xml.etree?

I think we could just add a method for tuning the protections? maybe a single method for all protections? or a configuration object for that? I don't really know which one would be the best here.

Note: I think having the user calling parser.<method-with-a-really-long-name> wouldn't really be "pretty". I'm sad that we can't have some parameter called tune_lolz_protection() (the most pythonic I can think of is set_thresholds(expansion=..., memory=...) so that users don't need to remember very long method names.

@picnixz it's a pity that etree and sax are not reusing pyexpat internally (as can be seen in Modules/_elementtree.c). Or else we could just ask for the underlying parser, give the user access, and we'd already be done.

it's a pity that etree and sax are not reusing pyexpat internally

But they do I think? at least there is some ExpatReader in the sax module and I think there is some import pyexpat in etree. Or did I misunderstand?

@picnixz maybe we need to look at sax and etree separately. Other than sax, etree has a C and a pure-Python version of the same module that — I think — need to satisfy the same interface. The C version of etree does not seem to use xmlparseobject of pyexpat internally (despite the #include "pyexpat.h"). I'm only vaguely familiar with this territory. Past related pull request #115623 showcases addition of the same feature to both the C and the pure-Python version.

Indeed, for etree, we have a pure Python implementation and a C implementation. The C implementation directly uses pyexpat, but the pure Python one doesn't. Implementing the logic for the pure Python interface seems an overkill. It would essentially ask to mimic the allocation limitations and we would then need to compute ourselves the size of objects being allocated, which is not realistic for the pure Python implementation.

I would instead suggest that we don't do anything for this one, and that by default, it's left untouched (namely make those methods a no-op). For the C implementation, we would add new methods at the level of _elementree.XMLParser. Note that we use libexpat by using the C API that is exposed by the pyexpat (C) module.

For SAX, it's also a pure Python implementation (no C), so I suggest leaving the API empty as well. The pure Python implementation however seems to only use pyexpat (see https://github.com/python/cpython/blob/main/Lib/xml/sax/expatreader.py#L284-L295) so we could directly use the underlying C API as well (I don't think it was needed to re-implement the logic in pure Python since, AFAICT, the parser was always C-based for SAX).

I think checking for the existence of the underlying C API through hasattr would be the best.

GH-139527 is a backport of this pull request to the 3.12 branch.

GH-139529 is a backport of this pull request to the 3.11 branch.

GH-139532 is a backport of this pull request to the 3.10 branch.