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

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

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.

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.

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.

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

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

gh-126033: fix UAF in `xml.etree.ElementTree.Element.remove` when con…

pythongh-126033: fix UAF in `xml.etree.ElementTree.Element.remove` wh…

[3.13] gh-126033: fix UAF in `xml.etree.ElementTree.Element.remove` w…

[3.12] gh-126033: fix UAF in `xml.etree.ElementTree.Element.remove` w…

Navigation Menu

Repository navigation

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

Crash report

What happened?

CPython versions tested on:

Operating systems tested on:

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

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	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;
	}

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

Description

Crash report

What happened?

CPython versions tested on:

Operating systems tested on:

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

Linked PRs

Activity

ZeroIntensity commented on Oct 27, 2024

picnixz commented on Oct 27, 2024

picnixz commented on Oct 27, 2024

ZeroIntensity commented on Oct 27, 2024

sobolevn commented on Oct 28, 2024

picnixz commented on Oct 28, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

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