Use-After-Free in PyString_FromStringAndSize() of stringobject.c

[dyjakan]

BPO	29028
Nosy	@vstinner, @benjaminp, @methane, @serhiy-storchaka, @ammaraskar
Files	buffer-use-after-free-fix.patch buffer-use-after-free-fix.patch2 buffer-use-after-free-3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2017-02-06.08:32:27.430>
created_at = <Date 2016-12-20.15:25:31.725>
labels = ['interpreter-core', 'type-crash']
title = 'Use-After-Free in PyString_FromStringAndSize() of stringobject.c'
updated_at = <Date 2017-02-06.08:32:27.428>
user = 'https://bugs.python.org/dyjakan'

bugs.python.org fields:

activity = <Date 2017-02-06.08:32:27.428>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2017-02-06.08:32:27.430>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-12-20.15:25:31.725>
creator = 'dyjakan'
dependencies = []
files = ['46046', '46083', '46417']
hgrepos = []
issue_num = 29028
keywords = ['patch', 'needs review']
message_count = 9.0
messages = ['283700', '284044', '284061', '284289', '284292', '286277', '286693', '287087', '287103']
nosy_count = 7.0
nosy_names = ['vstinner', 'benjamin.peterson', 'methane', 'python-dev', 'serhiy.storchaka', 'ammar2', 'dyjakan']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue29028'
versions = ['Python 2.7']

[dyjakan]

Recently I started doing some research related to language interpreters
and I've stumbled upon a bug in current Python 2.7. I already contacted PSRT and we concluded that this doesn't have security implications.

Repro file looks like this:

class Index(object):
    def __index__(self):
        for c in "foobar"*n:
            a.append(c)
        return n * 4

for n in range(1, 100000, 100):
    a = bytearray("test"*n)
    buf = buffer(a)
    s = buf[:Index():1]

If you have ASAN build then you'll see this:

==29054== ERROR: AddressSanitizer: heap-use-after-free on address 0x60040000a233 at pc 0x4fab7f bp 0x7ffdbfec0b50 sp 0x7ffdbfec0b48
READ of size 1 at 0x60040000a233 thread T0
    #0 0x4fab7e (/home/ad/builds/python-2.7-asan/bin/python2.7+0x4fab7e)
    #1 0x6bbed4 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x6bbed4)
    #2 0x59d998 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x59d998)
    #3 0x5b53fe (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b53fe)
    #4 0x5b5a65 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b5a65)
    #5 0x637eac (/home/ad/builds/python-2.7-asan/bin/python2.7+0x637eac)
    #6 0x63b3af (/home/ad/builds/python-2.7-asan/bin/python2.7+0x63b3af)
    #7 0x4192d0 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x4192d0)
    #8 0x7f6da3cf0f44 (/lib/x86_64-linux-gnu/libc-2.19.so+0x21f44)
    #9 0x417c11 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x417c11)
0x60040000a233 is located 3 bytes inside of 5-byte region [0x60040000a230,0x60040000a235)
freed by thread T0 here:
    #0 0x7f6da49d455f (/usr/lib/x86_64-linux-gnu/libasan.so.0.0.0+0x1555f)
    #1 0x6c5388 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x6c5388)
    #2 0x5b15fb (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b15fb)
    #3 0x5b53fe (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b53fe)
    #4 0x6f59c2 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x6f59c2)
    #5 0x440bc8 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x440bc8)
    #6 0x44a712 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x44a712)
    #7 0x440bc8 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x440bc8)
    #8 0x52afeb (/home/ad/builds/python-2.7-asan/bin/python2.7+0x52afeb)
    #9 0x4391ab (/home/ad/builds/python-2.7-asan/bin/python2.7+0x4391ab)
    #10 0x5b5d35 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b5d35)
    #11 0x4ea936 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x4ea936)
    #12 0x6bbd20 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x6bbd20)
    #13 0x59d998 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x59d998)
    #14 0x5b53fe (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b53fe)
    #15 0x5b5a65 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b5a65)
    #16 0x637eac (/home/ad/builds/python-2.7-asan/bin/python2.7+0x637eac)
    #17 0x63b3af (/home/ad/builds/python-2.7-asan/bin/python2.7+0x63b3af)
    #18 0x4192d0 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x4192d0)
    #19 0x7f6da3cf0f44 (/lib/x86_64-linux-gnu/libc-2.19.so+0x21f44)
previously allocated by thread T0 here:
    #0 0x7f6da49d455f (/usr/lib/x86_64-linux-gnu/libasan.so.0.0.0+0x1555f)
    #1 0x6c7b3d (/home/ad/builds/python-2.7-asan/bin/python2.7+0x6c7b3d)
    #2 0x6ca853 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x6ca853)
    #3 0x522ddd (/home/ad/builds/python-2.7-asan/bin/python2.7+0x522ddd)
    #4 0x440bc8 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x440bc8)
    #5 0x59f1ca (/home/ad/builds/python-2.7-asan/bin/python2.7+0x59f1ca)
    #6 0x5b53fe (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b53fe)
    #7 0x5b5a65 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x5b5a65)
    #8 0x637eac (/home/ad/builds/python-2.7-asan/bin/python2.7+0x637eac)
    #9 0x63b3af (/home/ad/builds/python-2.7-asan/bin/python2.7+0x63b3af)
    #10 0x4192d0 (/home/ad/builds/python-2.7-asan/bin/python2.7+0x4192d0)
    #11 0x7f6da3cf0f44 (/lib/x86_64-linux-gnu/libc-2.19.so+0x21f44)
Shadow bytes around the buggy address:
  0x0c00ffff93f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9410: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9420: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c00ffff9430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 00 04
=>0x0c00ffff9440: fa fa fd fa fa fa[fd]fa fa fa fd fa fa fa fd fa
  0x0c00ffff9450: fa fa fd fd fa fa fd fa fa fa fd fa fa fa 00 fa
  0x0c00ffff9460: fa fa 06 fa fa fa fd fa fa fa fd fa fa fa fd fd
  0x0c00ffff9470: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fa
  0x0c00ffff9480: fa fa fd fd fa fa fd fa fa fa 00 fa fa fa fd fa
  0x0c00ffff9490: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==29054== ABORTING

[ammaraskar]

The proposed patch fixes this, not sure if a regression test is appropriate here.

Here's a more minimal example that demonstrates the exact problem:

class Index():
    def __index__(self):
        global a
        a.append("2")
        return 999

a = bytearray(b"1")
buf = buffer(a)
s = buf[:1:Index()] 
# buf[Index():x:x] or buf[x:x:Index()] will also crash

The problem doesn't show up when doing buffer[x:Index()] or [Index():x] because this syntax calls the sq_slice method implemented by buffer object which is passed the indexes as numbers.

However when using slice notation with three arguments, the equivilant of these lines of code is executed:

slice_object = slice(x, Index(), x)
buffer[slice_object]

During the buffer[slice_object], a call is made in the slice object to find the indexes of the slice. This calls into the index method of the Index class which mutates the underlying storage behind the buffer. However, buffer's subscript method stores the underyling storage in a local variable before calling the GetIndices method (assuming the object won't be mutated) which means that when it returns, it returns a pointer to an older portion of memory.

I took a quick look at listobject, stringobject, unicodeobject, tupleobject and bytearrayobject's subscript methods and it seems they all only access their members after the call to PySlice_GetIndices, so I think they should be fine.

memoryview objects cause a BufferError: Existing exports of data: object cannot be re-sized error so Py3 should be fine.

[methane]

LGTM

[ammaraskar]

Updated patch based on Rietveld review

[serhiy-storchaka]

There a problem with PySlice_GetIndicesEx() (see bpo-27867). Buffer length shouldn't be evaluated before PySlice_GetIndicesEx() since it can call user code that can change buffer length. This issue can't be solved without first solving bpo-27867.

get_buf() is called twice. First for getting the size, and later in buffer_item() or after PySlice_GetIndicesEx() for getting a pointer. I think it can be called once.

Ammar, please write a unittest for this issue. It should also cover bugs in the first two versions of the patch.

[serhiy-storchaka]

Proposed patch fixes the issue. But it is hard to write a reliable patch.

[python-dev]

New changeset 8cfa6d3065b3 by Serhiy Storchaka in branch '2.7':
Issue bpo-29028: Fixed possible use-after-free bugs in the subscription of the
https://hg.python.org/cpython/rev/8cfa6d3065b3

[ammaraskar]

Did you forget to close this or is this not fixed, Serhiy?

[serhiy-storchaka]

I wanted first to finish bpo-27867 (expose new API as public). But this is not needed for this issue.