Skip to content

Conversation

hartwork
Copy link
Contributor

@hartwork hartwork commented Sep 26, 2025

Expose the XML Expat 2.7.2 mitigation APIs to disallow use of disproportional amounts of dynamic memory from within an Expat parser (see CVE-2025-59375 for instance).

The exposed APIs are available on Expat parsers, that is, parsers created by xml.parsers.expat.ParserCreate(), as:

  • parser.SetAllocTrackerActivationThreshold(threshold), and
  • parser.SetAllocTrackerMaximumAmplification(max_factor).

(cherry picked from commit f04bea4)

CC @picnixz


📚 Documentation preview 📚: https://cpython-previews--139367.org.readthedocs.build/

…2025-59375) (python#139234)

Expose the XML Expat 2.7.2 mitigation APIs to disallow use of
disproportional amounts of dynamic memory from within an Expat
parser (see CVE-2025-59375 for instance).

The exposed APIs are available on Expat parsers, that is,
parsers created by `xml.parsers.expat.ParserCreate()`, as:

- `parser.SetAllocTrackerActivationThreshold(threshold)`, and
- `parser.SetAllocTrackerMaximumAmplification(max_factor)`.

(cherry picked from commit f04bea4)
@picnixz
Copy link
Member

picnixz commented Sep 26, 2025

Ah I think we cross-posted but I actually want to simplify the future backports and next PR by using another generic handler (it will ease the diff). See #139366

@hartwork
Copy link
Contributor Author

Ah I think we cross-posted but I actually want to simplify the future backports and next PR by using another generic handler (it will ease the diff). See #139366

@picnixz I saw your message #139234 (comment) but did not expect impact on my wip 3.13 backport and not more than that fix. What should best be done with this pull request? Closing is alright with me if automation replaces my work after merge of #139366 — can it? Else I'm happy to adjust in here.

@picnixz
Copy link
Member

picnixz commented Sep 26, 2025

Closing is alright with me if automation replaces my work after merge

I'll try to make an automated backport. If it succeeds, then good. Otherwise we'll need something else :( However, I'm surprised by the segfaults?

@picnixz
Copy link
Member

picnixz commented Sep 26, 2025

(Segfaults could be because you didn't make clinic; and there are some directives that are not available in 3.13, see the 3.14 backport; maybe backporting the 3.14 PR would be easier actually)

@hartwork hartwork marked this pull request as draft September 26, 2025 17:03
@hartwork
Copy link
Contributor Author

@picnixz I'll wait for merge of #139366 and then get back to it. (It did not segfault on my local amd64.)

@picnixz
Copy link
Member

picnixz commented Sep 26, 2025

The address sanitizer says that there is a heap-UAF.

hartwork and others added 3 commits September 26, 2025 19:32
…on API (python#139366)

Fix some typos left in f04bea4,
and simplify some internal functions to ease maintenance of future
mitigation APIs.

(cherry picked from commit 68a1778)
@hartwork
Copy link
Contributor Author

@picnixz I have used the same technique as #139359 (review) here now to find any potential issues in the diff backported. The key thing that stands out is the difference in xmlparseobject versus PyObject and related casts, but I don't see obvious trouble there from a superficial look.

I'm attaching my three diffs if you would like to see my cross-diff comparison with your own eyes:

Looking from a different angle next…

@hartwork
Copy link
Contributor Author

@picnixz I found that…

Two (or arguably four) tests are failing:

  • test_set_maximum_amplification__fail_for_subparser
  • test_set_activation_threshold__fail_for_subparser

A parser instance is being freed twice.

The minimal reproducer for Python 3.13 (but not 3.14) is this:

class UseAfterFreeCrashDemoTest(unittest.TestCase):
    def test_use_after_free__crash(self):
        parser = expat.ParserCreate()
        subparser = parser.ExternalEntityParserCreate(None)

While interestingly adding del subparser works around the problem:

class NoUseAfterFreeHereTest(unittest.TestCase):
    def test_use_after_free__no_crash(self):
        parser = expat.ParserCreate()
        subparser = parser.ExternalEntityParserCreate(None)
        del subparser

Similarly, appending…

        del setter
        del subparser
        del parser

…to both original tests in here works around the issue. My understanding is that there is an unrelated object graph issue here that makes CPython free things in the wrong order. I'll add my workaround for a green CI for the moment, create a new separate issue about that, and hope for ideas from your side about how to proceed from there.

@picnixz
Copy link
Member

picnixz commented Sep 28, 2025

My understanding is that there is an unrelated object graph issue here that makes CPython free things in the wrong order

That's interesting. It could be the incremental GC which was deferred to Python 3.14 (https://docs.python.org/3.14/whatsnew/3.14.html#incremental-garbage-collection).

@picnixz
Copy link
Member

picnixz commented Sep 28, 2025

Anyway, we can correctly assume that we have a UAF. Let's create a separate issue (do you want to?)

@hartwork
Copy link
Contributor Author

Anyway, we can correctly assume that we have a UAF. Let's create a separate issue (do you want to?)

@picnixz I was already at it: #139400

@hartwork hartwork marked this pull request as ready for review September 28, 2025 17:54
@hartwork
Copy link
Contributor Author

hartwork commented Sep 28, 2025

@picnixz I have marked this PR here as ready for review now. It's the same as yours for 3.14 — #139359 — plus the workaround for #139400.

@picnixz
Copy link
Member

picnixz commented Sep 28, 2025

I would prefer waiting for the UAF to be fixed if it's ok for you. We usually don't like having a branch with temporary workarounds.

@hartwork
Copy link
Contributor Author

hartwork commented Sep 28, 2025

I would prefer waiting for the UAF to be fixed if it's ok for you. We usually don't like having a branch with temporary workarounds.

@picnixz good with me, I can revert the workaround later 👍

@hartwork hartwork marked this pull request as draft September 29, 2025 22:19
@hartwork hartwork marked this pull request as ready for review October 2, 2025 21:32
@picnixz
Copy link
Member

picnixz commented Oct 7, 2025

To have a good synchronization, we'll also delay 3.10 to 3.13 backports for their next release cycle (see #139359 (comment)).

@picnixz picnixz self-assigned this Oct 7, 2025
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.

2 participants