Movatterモバイル変換

bedevere-appbot mentioned this pull request

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

Closed

picnixz added the topic-XML label

picnixz requested a review fromvstinner

April 26, 2025 13:58

picnixz added the needs backport to 3.13bugs and security fixes label

picnixz requested a review fromserhiy-storchaka

April 26, 2025 14:35

serhiy-storchaka reviewed

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Copy link

MemberAuthor

picnixz commentedApr 26, 2025

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

Copy link

Member

serhiy-storchaka commentedApr 26, 2025

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

Copy link

MemberAuthor

picnixz commentedApr 26, 2025•
edited
Loading

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 added2 commits

April 27, 2025 11:48

always protect against possible mutations in__deepcopy__

721481d

remove bad assertion

cef5146

picnixz requested a review fromserhiy-storchaka

April 27, 2025 10:16

Copy link

MemberAuthor

picnixz commentedApr 27, 2025

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

picnixz commented

Apr 27, 2025

Modules/_elementtree.c OutdatedShow resolvedHide resolved

Update Modules/_elementtree.c

8297c8a

JelleZijlstra reviewed

Modules/_elementtree.c OutdatedShow resolvedHide resolved

JelleZijlstra reviewed

Modules/_elementtree.cShow resolvedHide resolved

usePy_CLEAR to be on the safe side

7f8ed28

JelleZijlstra approved these changes

bedevere-appbot added awaiting merge and removed awaiting core review labels

serhiy-storchaka reviewed

May 1, 2025

Modules/_elementtree.c OutdatedShow resolvedHide resolved

avoid killing deepcopy() optimization

c5badac

picnixz requested a review fromserhiy-storchaka

May 7, 2025 12:36

picnixz commented

May 7, 2025

Modules/_elementtree.c Outdated

		@@ -899,6 +905,8 @@ deepcopy(elementtreestate st, PyObject object, PyObject *memo)

		if (Py_REFCNT(object) == 1) {
		if (PyDict_CheckExact(object)) {
		// Exact dictionaries do not execute arbitrary code as it's

Copy link

MemberAuthor

picnixzMay 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@serhiy-storchaka is this assumption correct? namely, here I don't need to increfobject temporarily right?

Copy link

Member

serhiy-storchakaMay 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

PyDict_Next() does not use__iter__, so this comment is redundant.PyDict_Next() does not call any user code.

picnixz added the needs backport to 3.14bugs and security fixes label

May 7, 2025

serhiy-storchaka reviewed

May 9, 2025

Modules/_elementtree.c Outdated

		@@ -899,6 +905,8 @@ deepcopy(elementtreestate st, PyObject object, PyObject *memo)

		if (Py_REFCNT(object) == 1) {
		if (PyDict_CheckExact(object)) {
		// Exact dictionaries do not execute arbitrary code as it's

Copy link

Member

serhiy-storchakaMay 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

PyDict_Next() does not use__iter__, so this comment is redundant.PyDict_Next() does not call any user code.

Modules/_elementtree.c OutdatedShow resolvedHide resolved

Modules/_elementtree.cShow resolvedHide resolved

Merge remote-tracking branch 'upstream/main' into fix/xml/uaf-deepcop…

f56a368

…y-133009

Copy link

MemberAuthor

picnixz commentedMay 9, 2025

Actually, it's much more tricky than what I thought. Even with the INCREF/DECREF and the additional checks, the following still crashes, but not duringdeepcopy() but during de-allocation:

classEvil(ET.Element):def__deepcopy__(self,memo):root.append(ET.Element('y'))root.append(ET.Element('z'))returnselfY=Evil('y')root=ET.Element('a')root.extend([Evil('x'),ET.Element('t'),Y])c=deepcopy(root)print(list(c))print("ok")assert0

So I'll need a bit more work.

Copy link

Member

serhiy-storchaka commentedMay 9, 2025

#133010 (comment) should help. It is good that you already have a reproducer.

Copy link

Member

serhiy-storchaka commentedMay 9, 2025

Ah, you have a different issue -- growing children, not attributes. The solution should be the same -- either ignore new items or resize the array in process.

Copy link

MemberAuthor

picnixz commentedMay 9, 2025•
edited
Loading

The solution should be the same -- either ignore new items or resize the array in process.

Yes, actually I found a way to fix it in the meantime (but the same can be said)

handle more evil mutations

d585b09

serhiy-storchaka approved these changes

May 9, 2025

Lib/test/test_xml_etree.py

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM. 👍

		root = ET.Element('a')
		evil = X('x')
		root.extend([evil, ET.Element('y')])
		if is_python_implementation():

Copy link

Member

serhiy-storchakaMay 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We could also make the C implementation raising RuntimeError. It is fine either way.

Copy link

MemberAuthor

picnixzMay 10, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'll postpone this for a future PR as I want to backport this one to 3.13 and 3.14 first.

picnixz merged commit116a9f9 intopython:main

39 checks passed

Copy link

miss-islington-appbot commentedMay 10, 2025

Thanks@picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

picnixz deleted the fix/xml/uaf-deepcopy-133009 branch

May 10, 2025 07:32

bedevere-appbot removed the awaiting merge label

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

pythongh-133009: fix UAF in `xml.etree.ElementTree.Element.__deepcopy…

32edeb3

…__` (pythonGH-133010)(cherry picked from commit116a9f9)Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

pythongh-133009: fix UAF in `xml.etree.ElementTree.Element.__deepcopy…

94eeb29

…__` (pythonGH-133010)(cherry picked from commit116a9f9)Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Copy link

bedevere-appbot commentedMay 10, 2025

GH-133805 is a backport of this pull request to the3.14 branch.

bedevere-appbot removed the needs backport to 3.14bugs and security fixes label

Copy link

bedevere-appbot commentedMay 10, 2025

GH-133806 is a backport of this pull request to the3.13 branch.

bedevere-appbot removed the needs backport to 3.13bugs and security fixes label

picnixz added a commit that referenced this pull request

[3.13]gh-133009: fix UAF in `xml.etree.ElementTree.Element.__deepcop…

9718880

…y__` (GH-133010) (#133806)gh-133009: fix UAF in `xml.etree.ElementTree.Element.__deepcopy__` (GH-133010)(cherry picked from commit116a9f9)Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz added a commit that referenced this pull request