gh-133009: fix UAF in xml.etree.ElementTree.Element.__deepcopy__

[picnixz]

• Issue: UAF: xml.etree.ElementTree.Element.__deepcopy__ when concurrent mutations happen #133009

[serhiy-storchaka]

This is not enough. "tag", "text" and "tail" can be set to different value during deepcopying. If it was the last reference, the deepcopy() argument can be destroyed, and the reference can became handling.

The safe way is to increase the reference count of the deepcopy() argument before calling any code that can release the GIL.

[picnixz]

I actually tried to do something with tag etc, but I wasn't able to crash the interpreter.

[serhiy-storchaka]

String literals are interned and saved in the constants list. You need to use something having a single reference. Try 'tag'.upper().

[picnixz]

The safe way is to increase the reference count of the deepcopy() argument before calling any code that can release the GIL.

I've actually removed this code because I thought it wasn't needed, but I'll check with an evil non-interned string tomorrow.

[picnixz]

I'll try to add more tests later as well.

[JelleZijlstra]

Not directly related but the Py_REFCNT(object) == 1 in deepcopy() below is likely unsafe for similar reasons as #132070. I don't really understand what that check is doing, though: it seems what it does could work too if the refcnt > 1.

[picnixz]

Oh I don't know. I'm leaving on Monday evening for ~10 days. Maybe we can track the issue separately?

[JelleZijlstra]

Yes definitely a separate issue! Just noticed it while reading code around your change.

[serhiy-storchaka]

If the refcnt > 1, we need to check in memo, and use a reference to already made copy if it is found. We need also to update memo. It is complicated and adds a lot of overhead.