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

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

Co-Authored-By: chilaxan 35645806+chilaxan@users.noreply.github.com
Fixes #92888

Co-Authored-By: chilaxan <35645806+chilaxan@users.noreply.github.com>

…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 <3659035+serhiy-storchaka@users.noreply.github.com>

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 <3659035+serhiy-storchaka@users.noreply.github.com>

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:21:23Z

Objects/memoryobject.c

 {
+    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:21:23Z

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:21:23Z

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:21:23Z

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:21:23Z

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

        m = memoryview(bytearray(b'\xff'*size))
        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.

Fix memoryview bad __index__ use after free

73ffb29

Co-Authored-By: chilaxan <35645806+chilaxan@users.noreply.github.com>

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

📜🤖 Added by blurb_it.

3581de7

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

stylistic nits

43c66d6

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

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

Fidget-Spinner added type-crash needs backport to 3.8 needs backport to 3.7 and removed needs backport to 3.7 needs backport to 3.8 needs backport to 3.9 type-security labels May 19, 2022

Fidget-Spinner added 2 commits May 20, 2022

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 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 <3659035+serhiy-storchaka@users.noreply.github.com>

serhiy-storchaka reviewed May 23, 2022

View 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

Add more tests by Serhiy

c818990

Co-Authored-By: Serhiy Storchaka <3659035+serhiy-storchaka@users.noreply.github.com>

Fix newline

42a706c

serhiy-storchaka reviewed May 23, 2022

View changes

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

serhiy-storchaka approved these changes May 23, 2022

View 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

bedevere-bot added the awaiting merge label May 23, 2022

Fix for tuple index

b650764

serhiy-storchaka reviewed May 23, 2022

View 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

Open

serhiy-storchaka approved these changes May 23, 2022

View changes

vstinner reviewed May 23, 2022

View changes

Address Victor's review

6890713

serhiy-storchaka reviewed May 30, 2022

View changes

re-add ba=None

d3edf74

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

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

Fidget-Spinner commented May 19, 2022

serhiy-storchaka commented May 23, 2022 •

edited

Fidget-Spinner commented May 23, 2022 •

edited

serhiy-storchaka commented May 23, 2022

Fidget-Spinner commented May 23, 2022

serhiy-storchaka left a comment

serhiy-storchaka left a comment

serhiy-storchaka commented May 23, 2022

Fidget-Spinner commented May 23, 2022

vstinner left a comment

vstinner May 23, 2022

serhiy-storchaka May 25, 2022

vstinner May 23, 2022

serhiy-storchaka May 25, 2022

vstinner May 23, 2022

Fidget-Spinner May 29, 2022

serhiy-storchaka May 29, 2022

vstinner May 23, 2022

vstinner May 23, 2022

serhiy-storchaka May 25, 2022

serhiy-storchaka May 30, 2022

		{
		CHECK_RELEASED_INT(self); /* See gh-92888 for why we need this here */

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

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

Are you sure you want to change the base?

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

Conversation

Fidget-Spinner commented May 19, 2022

serhiy-storchaka commented May 23, 2022 • edited

Fidget-Spinner commented May 23, 2022 • edited

serhiy-storchaka commented May 23, 2022

Fidget-Spinner commented May 23, 2022

serhiy-storchaka left a comment

serhiy-storchaka left a comment

serhiy-storchaka commented May 23, 2022

Fidget-Spinner commented May 23, 2022

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Fidget-Spinner commented May 23, 2022 •

edited