NotificationsYou must be signed in to change notification settings
Fork32.2k
Star67.7k

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating index methods#132379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

bast0006 wants to merge7 commits intopython:main

base:main

Choose a base branch

frombast0006:bast0006/gh-91153

Open

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating index methods#132379

bast0006 wants to merge7 commits intopython:mainfrombast0006:bast0006/gh-91153

Conversation

Copy link

bast0006 commentedApr 10, 2025•
edited by bedevere-appbot
Loading

bytearray's__setitem__ implementation currently grabs a reference to its internal buffer before calling_getbyvalue to determine the index that needs assignment._getbyvalue can call into arbitrary python code via__index__ dunders, which could alter the internal buffer and leave said reference dangling.

A prior fix for this issue ensures that bounds checking occurs after_getbyvalue is called. However, python code is capable of resizing the bytearray, resulting in limited but still broken behavior.

This patch ensures that the reference to the internal buffer is fetched only after_getbyvalue is called to prevent it from being held while any python code is run.

Issue:UAF when writing to a bytearray with an element implementing__index__ with side-effects #91153

bast0006 added2 commits

March 29, 2025 07:36

pythonGH-91153: Handle _getbytevalue potentially invalidating bytearr…

71ae212

…ay allocation during item assignment

pythonGH-91153: Add additional cases totest_mutating_index

b2f7df5

Copy link

python-cla-botbot commentedApr 10, 2025•
edited
Loading

All commit authors signed the Contributor License Agreement.

bedevere-appbot added the awaiting review label

Apr 10, 2025

Copy link

bedevere-appbot commentedApr 10, 2025

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

bedevere-appbot mentioned this pull request

Apr 10, 2025

UAF when writing to a bytearray with an element implementing__index__ with side-effects#91153

Open

Copy link

Author

bast0006 commentedApr 10, 2025•
edited
Loading

I've signed the CLA. Let me know if a NEWS entry is required. I'm not sure it is, especially since there was already a previous change that this is a fix to.

picnixz self-requested a review

May 17, 2025 11:00

picnixz reviewed

May 17, 2025

View reviewed changes

Copy link

Member

picnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Can you add a NEWS entry please? It's still worth to mention that we fixed more crashes.

Lib/test/test_bytes.py OutdatedShow resolvedHide resolved

bast0006 added2 commits

May 17, 2025 18:13

pythonGH-91153: Move mutating index bytearray test to it's own separa…

bb96c2a

…te test

pythonGH-91153: add news entry

d29422b

Copy link

Author

bast0006 commentedMay 17, 2025

Done

bast0006 requested a review frompicnixz

May 17, 2025 23:25

ZeroIntensity reviewed

May 18, 2025

View reviewed changes

Misc/NEWS.d/next/Core_and_Builtins/2025-05-17-18-22-12.gh-issue-91153.ioA_83.rst OutdatedShow resolvedHide resolved

Lib/test/test_bytes.pyShow resolvedHide resolved

ZeroIntensity added needs backport to 3.13

bugs and security fixes

needs backport to 3.14bugs and security fixes labels

May 18, 2025

Nit cleanup

d3d1974

bast0006 force-pushed thebast0006/gh-91153 branch from3c1de75 tod3d1974Compare

May 18, 2025 03:53

picnixz reviewed

Jun 2, 2025

View reviewed changes

Lib/test/test_bytes.py OutdatedShow resolvedHide resolved

Misc/NEWS.d/next/Core_and_Builtins/2025-05-17-20-56-05.gh-issue-91153.afgtG2.rst OutdatedShow resolvedHide resolved

Objects/bytearrayobject.c OutdatedShow resolvedHide resolved

Objects/bytearrayobject.c

		@@ -805,6 +806,7 @@ bytearray_ass_subscript_lock_held(PyObject op, PyObject index, PyObject *value
		/* Delete slice */
		size_t cur;
		Py_ssize_t i;
		char* buf = PyByteArray_AS_STRING(self);

Copy link

Member

picnixzJun 2, 2025

Choose a reason for hiding this comment

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

We can also holdPyByteArray_GET_SIZE(self) temporarily here.

Copy link

Author

bast0006Jun 3, 2025

Choose a reason for hiding this comment

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

I agree, but I'm not as familiar with this system or C as I would want to be to feel comfortable making that change. Especially since the relevant code is mostly >10 years old and that change is not immediately required to solve the issue.

pythonGH-91153: Fixup comments and NEWS entry

18e6597

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

ZeroIntensity approved these changes

Jun 3, 2025

View reviewed changes

Lib/test/test_bytes.py Outdated

		@@ -1889,6 +1889,39 @@ def __index__(self):
		with self.assertRaises(IndexError):
		self._testlimitedcapi.sequence_setitem(b, 0, Boom())

		def test_mutating_index_inbounds(self):
		# See gh-91153

Copy link

Member

ZeroIntensityJun 3, 2025

Choose a reason for hiding this comment

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

Could you add a brief description of the issue?

Copy link

Author

bast0006Jun 3, 2025

Choose a reason for hiding this comment

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

Done

Objects/bytearrayobject.c

		char *buf = PyByteArray_AS_STRING(self);
		// Do not store a reference to the internal buffer since
		// index.__index__() or _getbytevalue() may alter 'self'.
		// See https://github.com/python/cpython/issues/91153.

Copy link

Member

ZeroIntensityJun 3, 2025

Choose a reason for hiding this comment

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

I don't think we need to fully link to it, just thegh- reference should be fine.

Suggested change

	// Seehttps://github.com/python/cpython/issues/91153.
	// Seegh-91153

Copy link

Author

bast0006Jun 3, 2025•
edited
Loading

Choose a reason for hiding this comment

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

picnixz suggested this above, and it's common for the c source to have complete links, so I think it's fine as-is.

Copy link

Member

picnixzJun 3, 2025

Choose a reason for hiding this comment

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

With complete links, I can open them from my IDE. It's much easier that way IMO

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

Jun 3, 2025

bast0006 force-pushed thebast0006/gh-91153 branch from5853b5c to50b7998Compare

June 3, 2025 20:53

pythonGH-91153: Add short clarifying comments to test

4b7ec9c

bast0006 force-pushed thebast0006/gh-91153 branch from50b7998 to4b7ec9cCompare

June 3, 2025 20:57

Labels

awaiting core review needs backport to 3.13

bugs and security fixes

needs backport to 3.14

bugs and security fixes

3 participants

Movatterモバイル変換

Uh oh!

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating __index__ methods#132379

Are you sure you want to change the base?

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating __index__ methods#132379

Uh oh!

Conversation

bast0006 commentedApr 10, 2025• edited by bedevere-appbotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

python-cla-botbot commentedApr 10, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bedevere-appbot commentedApr 10, 2025

Uh oh!

bast0006 commentedApr 10, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bast0006 commentedMay 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixzJun 2, 2025

Choose a reason for hiding this comment

Uh oh!

bast0006Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

ZeroIntensityJun 3, 2025

Choose a reason for hiding this comment

Uh oh!

bast0006Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

ZeroIntensityJun 3, 2025

Choose a reason for hiding this comment

Uh oh!

bast0006Jun 3, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixzJun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating index methods#132379

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating index methods#132379

bast0006 commentedApr 10, 2025•
edited by bedevere-appbot
Loading

python-cla-botbot commentedApr 10, 2025•
edited
Loading

bast0006 commentedApr 10, 2025•
edited
Loading

picnixz left a comment•
edited
Loading

bast0006Jun 3, 2025•
edited
Loading