classification
Title: Use-After-Free in PyString_FromStringAndSize() of stringobject.c
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: ammar2, benjamin.peterson, dyjakan, inada.naoki, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: needs review, patch

Created on 2016-12-20 15:25 by dyjakan, last changed 2017-02-06 08:32 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
buffer-use-after-free-fix.patch ammar2, 2016-12-26 20:17 review
buffer-use-after-free-fix.patch2 ammar2, 2016-12-29 18:45 review
buffer-use-after-free-3.patch serhiy.storchaka, 2017-01-25 20:17 review
Messages (9)
msg283700 - (view) Author: (dyjakan) Date: 2016-12-20 15:25
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
```
msg284044 - (view) Author: Ammar Askar (ammar2) * (Python triager) Date: 2016-12-26 20:17
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.
msg284061 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2016-12-27 04:43
LGTM
msg284289 - (view) Author: Ammar Askar (ammar2) * (Python triager) Date: 2016-12-29 18:45
Updated patch based on Rietveld review
msg284292 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-29 20:07
There a problem with PySlice_GetIndicesEx() (see issue27867). 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 issue27867.

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.
msg286277 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-25 20:17
Proposed patch fixes the issue. But it is hard to write a reliable patch.
msg286693 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-01 20:54
New changeset 8cfa6d3065b3 by Serhiy Storchaka in branch '2.7':
Issue #29028: Fixed possible use-after-free bugs in the subscription of the
https://hg.python.org/cpython/rev/8cfa6d3065b3
msg287087 - (view) Author: Ammar Askar (ammar2) * (Python triager) Date: 2017-02-06 07:11
Did you forget to close this or is this not fixed, Serhiy?
msg287103 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-06 08:32
I wanted first to finish issue27867 (expose new API as public). But this is not needed for this issue.
History
Date User Action Args
2017-02-06 08:32:27serhiy.storchakasetstatus: open -> closed
messages: + msg287103

dependencies: - various issues due to misuse of PySlice_GetIndicesEx
resolution: fixed
stage: patch review -> resolved
2017-02-06 07:11:03ammar2setmessages: + msg287087
2017-02-01 20:54:54python-devsetnosy: + python-dev
messages: + msg286693
2017-01-25 20:17:38serhiy.storchakasetfiles: + buffer-use-after-free-3.patch

messages: + msg286277
2017-01-03 12:54:32vstinnersetnosy: + vstinner
2016-12-29 20:08:03serhiy.storchakasetassignee: serhiy.storchaka
2016-12-29 20:07:21serhiy.storchakasetdependencies: + various issues due to misuse of PySlice_GetIndicesEx
messages: + msg284292
2016-12-29 19:52:36serhiy.storchakasetnosy: + serhiy.storchaka
2016-12-29 18:45:06ammar2setfiles: + buffer-use-after-free-fix.patch2

messages: + msg284289
2016-12-27 04:43:18inada.naokisetnosy: + inada.naoki
messages: + msg284061
2016-12-26 20:29:11ammar2setnosy: + benjamin.peterson
2016-12-26 20:17:52ammar2setfiles: + buffer-use-after-free-fix.patch

nosy: + ammar2
messages: + msg284044

keywords: + needs review, patch
stage: patch review
2016-12-20 15:25:31dyjakancreate