UAF: xml.etree.ElementTree.Element.remove when concurrent mutations happen

[picnixz]

Crash report

What happened?

A UAF in Element.remove was fixed in #68279 but one can mutate the child's list during .remove and cause an OOB crash:

import xml.etree.ElementTree as ET

class EvilElement(ET.Element):
    def __eq__(self, other):
        base.clear()
        return False

base = ET.Element('a')
base.append(EvilElement('a'))
base.append(EvilElement('a'))
base.remove(ET.Element('b'))

Attacked code:

cpython/Modules/_elementtree.c

Lines 1648 to 1656 in dc76a4a

	for (i = 0; i < self->extra->length; i++) {
	if (self->extra->children[i] == subelement)
	break;
	rc = PyObject_RichCompareBool(self->extra->children[i], subelement, Py_EQ);
	if (rc > 0)
	break;
	if (rc < 0)
	return NULL;
	}

I think we need to introduce some state integer to check that there is no evil mutation (similar to what's being done for OrderedDict).

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Output from running 'python -VV' on the command line:

No response

Linked PRs

• gh-126033: Fix crash in _elementtree.c where evil tags/elements occurs #126079
• gh-126033: fix a crash in xml.etree.ElementTree.Element.remove when concurrent mutations happen #126124
• [3.13] gh-126033: fix UAF in xml.etree.ElementTree.Element.remove when concurrent mutations happen (GH-126124) #131929
• [3.12] gh-126033: fix UAF in xml.etree.ElementTree.Element.remove when concurrent mutations happen (GH-126124) #131930

[ZeroIntensity]

Hmm, there seem to be a lot of issues with PyObject_RichCompareBool and custom __eq__'s, right? It might be worth trying to fix things more globally there rather than on a case-by-case basis like this.

[picnixz]

I've roughly skimmed through the other occurrences and couldn't find UAFs or bad OOB. And I'd prefer fixing them one by one because the patch is not always the same.

[ZeroIntensity]

My main concern here is that while we can fix this as much as we want for the core, this looks like it's common enough that downstream is having this problem too.

[sobolevn]

Basically, we need to protect base from mutation during the iteration over it. An extra field maybe?

[picnixz]

See #126041 for that (the issue and PR may have passed under your radar :))