[3.9] gh-139400: Make sure that parent parsers outlive their subparsers in pyexpat (GH-139403)
#139614
Conversation
…rs in `pyexpat` (python#139403) * Modules/pyexpat.c: Disallow collection of in-use parent parsers. Within libexpat, a parser created via `XML_ExternalEntityParserCreate` is relying on its parent parser throughout its entire lifetime. Prior to this fix, is was possible for the parent parser to be garbage-collected too early. (cherry picked from commit 6edb2dd)
|
Note: actually I didn't see but the NEWS entry was put inside Core & Builtins, but this should have been put either into Library or Security (I don't know where previous bugfixes like that were put). I'm sorry to ask this, but would you be willing to amend the backports? (at least for 3.9-3.12). For 3.13+ we can have more commits, but for security-only branches, we would need the RM for approval for the amendment. |
| int i; | ||
| for (i = 0; handler_info[i].name != NULL; i++) | ||
| Py_VISIT(op->handlers[i]); | ||
| Py_VISIT(op->parent); |
There was a problem hiding this comment.
Note: if the type is a static type, then it's fine not to visit Py_TYPE(op). Here it's therefore correct because the type is a static type and not a heap type.
There was a problem hiding this comment.
@picnixz I'm assuming your are referring to static PyTypeObject Xmlparsetype here? Good to know, thanks for checking!
There was a problem hiding this comment.
Yes, this one. Static types are now only used for built-in types such as exceptions, numbers, and standard objects. Types in extension modules are now mostly heap types.
@picnixz I can sure amend them. Let's try it out in one of them and then I'll copy the approach to the others after approval. I'll need to leave for a few hours now though, I'll probably be able to pick this up before midnight UTC+2. Which should I put it then — Library or Security? Maybe past entries in Git can help with that decision. See you later 👋 |
|
The UAF itself is easy to trigger, but except for a DoS (because the interpeter crashed) we can't really do more. So we can move it to "Library" (note that we don't really have a good convention here... sometimes it's in Security, sometimes in Library). I think we should move something to security when it's more annoying (note that we also didn't backport some UAFs fixes in older branches even if it touched XMLs because it was mostly if someone messed up with concurrent list mutations, not with the GC). @sethmlarson Where do you think this change should go? Security or Library? |
picnixz
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @picnixz for commit f72a088 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F139614%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
|
I want to check the refleaks here as well because the type is static. |
@picnixz @sethmlarson I'm not sure sure if the order of freeing interconnected Expat parsers in CPython can be controlled by an attacker, but I should note that heap use-after-free can go up to arbitrary code execution, in general. So we can err on the "likely not" or the "we can maybe not rule it out" side of things. I will move it to Security for a start now to err on the latter side, and I'm happy to move it to Library tomorrow if Security feels totally out of place to you. |
|
I agree that we can have UAF can be used to do arbitrary code execution but I still fail to see how one could easily turn this one into an RCE (freeing interconnected expat parser likely need a direct access to interpreter, or the code being executed, not just with some untrusted input I'd say). Now, since you moved it into "Security" for all other PRs, I'm fine with keeping it there (in some sense, fixing the UAF is also a way to avoid a footgun with perfectly legitimate code). |
|
Yes, if possible! TiA |
pyexpatrelated to.ExternalEntityParserCreate#139400