gh-92888: Fix memoryview bad `index` use after free #92946

Fidget-Spinner · 2022-05-19T08:50:03Z

Co-Authored-By: chilaxan [email protected]
Fixes #92888

Co-Authored-By: chilaxan <[email protected]>

…r/cpython into fix_memoryview_auf

serhiy-storchaka · 2022-05-23T08:11:40Z

I have other approach, but your approach is more compatible, so it is safer to use it backports. I'll experiment with other approach in the developing branch only.

You can use the following test as a base:

    def test_use_released_memory(self):
        size = 128
        def release():
            m.release()
            nonlocal ba
            ba = bytearray(size)
        class MyIndex:
            def __index__(self):
                release()
                return 4
        class MyFloat:
            def __float__(self):
                release()
                return 4.25
        class MyBool:
            def __bool__(self):
                release()
                return True

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        with self.assertRaises(ValueError):
            m[MyIndex()]

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        self.assertEqual(list(m[:MyIndex()]), [255] * 4)

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        self.assertEqual(list(m[MyIndex():8]), [255] * 4)

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        m[MyIndex()] = 42
        self.assertEqual(ba[:8], b'\0'*8)

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        m[:MyIndex()] = b'spam'
        self.assertEqual(ba[:8], b'\0'*8)

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        m[MyIndex():8] = b'spam'
        self.assertEqual(ba[:8], b'\0'*8)

        ba = None
        m = memoryview(bytearray(b'\xff'*size))
        m[0] = MyIndex()
        self.assertEqual(ba[:8], b'\0'*8)

        for fmt in 'bhilqnBHILQN':
            ba = None
            m = memoryview(bytearray(b'\xff'*size)).cast(fmt)
            m[0] = MyIndex()
            self.assertEqual(ba[:8], b'\0'*8)

        for fmt in 'fd':
            ba = None
            m = memoryview(bytearray(b'\xff'*size)).cast(fmt)
            m[0] = MyFloat()
            self.assertEqual(ba[:8], b'\0'*8)

        ba = None
        m = memoryview(bytearray(b'\xff'*size)).cast('?')
        m[0] = MyBool()
        self.assertEqual(ba[:8], b'\0'*8)

Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst

Fidget-Spinner · 2022-05-23T09:48:57Z

@serhiy-storchaka thanks taking the time to review and writing those really comprehensive tests. One part of your test fails on my branch: specifically m[MyIndex()] = 42 raises ValueError. That's supposed to happen right? (Actually, most of the asserts seem to fail on my branch :()

serhiy-storchaka · 2022-05-23T10:22:08Z

Yes, it is expected that some lines raise exceptions with your changes. Just add assertRaises() if needed.

Co-Authored-By: Serhiy Storchaka <[email protected]>

Fidget-Spinner · 2022-05-23T10:57:52Z

Great, thanks to your tests I found that my fix didn't cover slices. It now does.

serhiy-storchaka

Yet few tests.

Lib/test/test_memoryview.py

Co-Authored-By: Serhiy Storchaka <[email protected]>

Lib/test/test_memoryview.py

serhiy-storchaka

👍

Lib/test/test_memoryview.py

serhiy-storchaka · 2022-05-23T14:58:17Z

I am sorry for being so pedantic. 🤦‍♂️

Fidget-Spinner · 2022-05-23T15:46:14Z

I am sorry for being so pedantic. man_facepalming

I didn't feel that you were. Those are errors I ought to have caught anyways. Thanks for the detailed review as always.

vstinner

I like your approach to fix memory_ass_sub(). Here is a first review.

Objects/memoryobject.c

vstinner · 2022-05-23T20:10:07Z

Objects/memoryobject.c

-copy_single(const Py_buffer *dest, const Py_buffer *src)
+copy_single(PyMemoryViewObject *self, const Py_buffer *dest, const Py_buffer *src)
 {
+    CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */


Can you move the check after the equiv_structure() test?

Would not access to Py_buffer of the released object cause reading after free?

Objects/memoryobject.c

vstinner · 2022-05-23T20:16:21Z

Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst

@@ -0,0 +1,2 @@
+Fix ``memoryview`` use after free when accessing the backing buffer in certain cases.


I propose to mention more explicitly that the protection is about released views:

If a memoryview is released while getting or setting an item, raise an exception. For example, converting an object to an index can execute arbitrary Python code which can indirectly release the memoryview.

Not always an exception is raised.

The bug was in reading or wring the freed memory. Now it is prevented -- you either get an exception or free the memory after reading. @Fidget-Spinner's description is more correct.

I am going to address such inconsistency in a separate issue.

Lib/test/test_memoryview.py

vstinner · 2022-05-23T20:17:28Z

Lib/test/test_memoryview.py

+        def release():
+            m.release()
+            nonlocal ba
+            ba = bytearray(size)


That's useless, no?

No, we need it for tests below that tests indexing into ba[].

We allocate a bytearray of the same size as the bytearray just released in memoryview in hope that it will be allocated at the same memory. It helps to check that we do nor read/write a freed memory.

vstinner · 2022-05-23T20:18:41Z

Lib/test/test_memoryview.py

+                return True
+
+        ba = None
+        m = memoryview(bytearray(b'\xff'*size))


In my PR, I tried to make the code more generic to test more cases: https://github.com/python/cpython/pull/93127/files#diff-d41c6bb40a1e03fea5a20d15c4077413e0ddde65651147922b625b03a66a2f16R399:

tp = self.rw_type b = self.rw_type(self._source) view = self._view(b)

vstinner · 2022-05-23T20:20:57Z

Lib/test/test_memoryview.py

+        ba = None
+        m = memoryview(bytearray(b'\xff'*size))
+        with self.assertRaises(ValueError):
+            m[MyIndex()]


This test is very long. Can you try to factorize similar code and use loop with subTest(), and put pack operations in one test method and unpack in another test method?

Then we will need to duplicate the definitions of internal classes.

The tested code is so different, that it is difficult to use a loop. And I think that the result will be more complicated.

serhiy-storchaka · 2022-05-30T08:12:26Z

Lib/test/test_memoryview.py

        with self.assertRaises(ValueError):
            m[MyIndex()]

-        ba = None


It is necessary. The old bytearray should be deallocated before creating a new bytearray, so all bytearrays will be allocated at the same place.

vstinner · 2022-06-17T14:53:28Z

I closed my PR #93127 since this one seems to be more complete.

Fidget-Spinner · 2022-06-17T14:55:10Z

@vstinner do you have any major objections? Otherwise, I plan to merge.

vstinner · 2022-06-17T15:11:25Z

My reviews on the tests were just remarks, not objections.

miss-islington · 2022-06-17T15:14:57Z

Thanks @Fidget-Spinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

miss-islington · 2022-06-17T15:15:04Z

Sorry, @Fidget-Spinner, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 11190c4ad0d3722b8d263758ac802985131a5462 3.10

bedevere-bot · 2022-06-17T15:15:05Z

GH-93948 is a backport of this pull request to the 3.11 branch.

bedevere-bot · 2022-06-17T15:18:57Z

GH-93950 is a backport of this pull request to the 3.10 branch.

Fix memoryview bad __index__ use after free

73ffb29

Co-Authored-By: chilaxan <[email protected]>

bedevere-bot added the awaiting core review label May 19, 2022

📜🤖 Added by blurb_it.

3581de7

Fidget-Spinner added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels May 19, 2022

stylistic nits

43c66d6

Fidget-Spinner added type-security A security issue needs backport to 3.9 labels May 19, 2022

Fidget-Spinner requested a review from serhiy-storchaka May 19, 2022 11:07

Fidget-Spinner added type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.8 and removed needs backport to 3.7 type-security A security issue labels May 19, 2022

Fidget-Spinner added 2 commits May 20, 2022 18:52

Merge branch 'fix_memoryview_auf' of https://github.com/Fidget-Spinne…

18b921f

…r/cpython into fix_memoryview_auf

Fix whitespace

2f3c04f

serhiy-storchaka reviewed May 23, 2022

View reviewed changes

Misc/NEWS.d/next/Security/2022-05-19-08-53-07.gh-issue-92888.TLtR9W.rst Outdated Show resolved Hide resolved

Use more comprehensive tests by Serhiy

a250b02

Co-Authored-By: Serhiy Storchaka <[email protected]>

serhiy-storchaka reviewed May 23, 2022

View reviewed changes

Lib/test/test_memoryview.py Show resolved Hide resolved

Lib/test/test_memoryview.py Show resolved Hide resolved

Fidget-Spinner and others added 2 commits May 23, 2022 21:49

Add more tests by Serhiy

c818990

Co-Authored-By: Serhiy Storchaka <[email protected]>

Fix newline

42a706c

serhiy-storchaka reviewed May 23, 2022

View reviewed changes

Lib/test/test_memoryview.py Outdated Show resolved Hide resolved

serhiy-storchaka approved these changes May 23, 2022

View reviewed changes

Lib/test/test_memoryview.py Outdated Show resolved Hide resolved

Lib/test/test_memoryview.py Outdated Show resolved Hide resolved

bedevere-bot removed the awaiting core review label May 23, 2022

Fix for tuple index

b650764

serhiy-storchaka reviewed May 23, 2022

View reviewed changes

Lib/test/test_memoryview.py Outdated Show resolved Hide resolved

Lib/test/test_memoryview.py Outdated Show resolved Hide resolved

Address review

47bd91f

Fidget-Spinner mentioned this pull request May 23, 2022

Use After Free when assigning into a memoryview #92888

Closed

serhiy-storchaka approved these changes May 23, 2022

View reviewed changes

vstinner reviewed May 23, 2022

View reviewed changes

Address Victor's review

6890713

serhiy-storchaka reviewed May 30, 2022

View reviewed changes

re-add ba=None

d3edf74

vstinner mentioned this pull request Jun 17, 2022

gh-92888: memory_ass_sub() calls CHECK_RELEASED_INT() twice #93127

Closed

Fidget-Spinner merged commit 11190c4 into python:main Jun 17, 2022

bedevere-bot removed the awaiting merge label Jun 17, 2022

Fidget-Spinner deleted the fix_memoryview_auf branch June 17, 2022 15:14

bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 17, 2022

miss-islington assigned Fidget-Spinner Jun 17, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 17, 2022

		@@ -0,0 +1,2 @@
		Fix ``memoryview`` use after free when accessing the backing buffer in certain cases.

Uh oh!

gh-92888: Fix memoryview bad __index__ use after free #92946

gh-92888: Fix memoryview bad __index__ use after free #92946

Conversation

Fidget-Spinner commented May 19, 2022

Uh oh!

serhiy-storchaka commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented May 23, 2022

Uh oh!

Fidget-Spinner commented May 23, 2022

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented May 23, 2022

Uh oh!

Fidget-Spinner commented May 23, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 17, 2022

Uh oh!

Fidget-Spinner commented Jun 17, 2022

Uh oh!

vstinner commented Jun 17, 2022

Uh oh!

miss-islington commented Jun 17, 2022

Uh oh!

miss-islington commented Jun 17, 2022

Uh oh!

bedevere-bot commented Jun 17, 2022

Uh oh!

bedevere-bot commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

gh-92888: Fix memoryview bad `index` use after free #92946

gh-92888: Fix memoryview bad `index` use after free #92946

serhiy-storchaka commented May 23, 2022 •

edited

Loading

Fidget-Spinner commented May 23, 2022 •

edited

Loading