gh-144984: Fix crash in ExternalEntityParserCreate() error paths

[raminfp]

Fix crash when ExternalEntityParserCreate() hits an error path
(allocation failure). Py_DECREF(new_parser) calls xmlparse_dealloc()
on a partially-initialized object where handlers is NULL, causing a
NULL pointer dereference in clear_handlers. Additionally,
Py_CLEAR(parent) in dealloc already decrements the parent's refcount,
making the subsequent Py_DECREF(self) a double-decrement.

• Add NULL guard in clear_handlers
• Set parent = NULL before Py_DECREF(new_parser) in each error path

• Issue: NULL deref + Double Py_DECREF in pyexpat ExternalEntityParserCreate() #144984

[picnixz]

LGTM, though let's quickly ask @hartwork on that since he wrote that PR.

[picnixz]

@hugovk What's our policy about fixing security issues? should we backport them as far as the original security fix went?

[benediktjohannes]

@picnixz very interesting question! I asked that question to myself as well one time in #144118 where it was indirectly answered because this was only backported to the versions that still recieve bug fixes and not the ones which recieve security updates, but I'm not quite sure whether this is the best solution. And maybe my comment just wasn't seen anymore, but this would at least be consistent. So if this is also backported, I suggest that #144118 should be as well.

[benediktjohannes]

I hope this helps a little bit! 👍

[benediktjohannes]

@picnixz very interesting question! I asked that question to myself as well one time in #144118 where it was indirectly answered because this was only backported to the versions that still recieve bug fixes and not the ones which recieve security updates, but I'm not quite sure whether this is the best solution. And maybe my comment just wasn't seen anymore, but this would at least be consistent. So if this is also backported, I suggest that #144118 should be as well.

"And one other question: How do we handle this concerning backports? Theoretically this is a bug fix, so it should be ported back to only a few supported versions (those recieving bug fixes and not those only recieving security updates) while this seems kind of interesting to me because the other patch (the denial of these chars) was a security update and for this reason applied backward also to those versions only getting security updates and this included this kind of "bug" and for this reason a "old version" now has a new bug through a new update which is not any more patched if we don't apply this to the ones only recieving security updates which is maybe a little bit a of a problem because those using older versions probably do this because they want them to be stable and in this case we "added a bug" / removed a feature and we won't apply it back to them. So will we also merge this to 3.10 and so on?" was my original comment on the other issue (so that you don't have to search for this)

[picnixz]

This is a bit different. I agree that the change with wsgiref should have been backport as much as possible because it's breaking older versions more than it should have. Here, the fix is not that important because it's anyway in an error path and the worst we can have is likely just a crash. But this only occurs when we don't have enough memory. Crashing is usually not good, but assessing whether something should be considered a security fix is really on a case-by-case basis and left at the discretion of core developers and the PSRT.

So, for this specific PR, I don't mind if it's not backported prior to 3.13. For your other PR, I'm going to raise your concers on the issue directly (which was closed before the backports were done).

[raminfp]

I agree that whether this should be considered a security fix depends on whether the issue is exploitable and has meaningful security impact, as mentioned. If it is not exploitable and only results in a crash in an error path, then not backporting it to security-only branches seems reasonable.

Why is PSRT delaying the response? (@picnixz )

[benediktjohannes]

This is a bit different. I agree that the change with wsgiref should have been backport as much as possible because it's breaking older versions more than it should have. Here, the fix is not that important because it's anyway in an error path and the worst we can have is likely just a crash. But this only occurs when we don't have enough memory. Crashing is usually not good, but assessing whether something should be considered a security fix is really on a case-by-case basis and left at the discretion of core developers and the PSRT.

So, for this specific PR, I don't mind if it's not backported prior to 3.13. For your other PR, I'm going to raise your concers on the issue directly (which was closed before the backports were done).

Thank you!

[picnixz]

Why is PSRT delaying the response

They are not delaying the response. My mention to the PSRT was essentially to indicate that decisions about security issues is the responsibility of maintainers and the PSRT, contributors (they may contribute to the discussion but eventually maintainers decide whether or not to consider it a security issue).

Now, as I said, this is not really a security issue here. The original issue was more important (that is, subparsers outliving their parents and the original crash could be reproduced easily with pure Python code), but this specific one is really in error paths so it's hard to exploit as it requires a memory allocation failure. If memory allocation fails usually this means that we already in a bad environment, and other pieces of the program could also be affected.

[hugovk]

@hugovk What's our policy about fixing security issues? should we backport them as far as the original security fix went?

If it's just a bugfix, only to bugfix branches.

If there's a problem with the original security fix such that it's still exploitable, and the bugfix is also needed to make it work, then it can probably go back to security branches.

See https://devguide.python.org/developer-workflow/development-cycle/#security-branches.

[hartwork]

I understand both the issue and the fix now, looks good! @raminfp thanks for the report and the fix, @picnixz thanks for the CC 👍