Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
dyjakan mannequin opened this issue Dec 20, 2016 · 9 comments
Closed

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

dyjakan mannequin opened this issue Dec 20, 2016 · 9 comments
Assignees
@serhiy-storchaka
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@dyjakan
Copy link
Mannequin

dyjakan mannequin commented Dec 20, 2016

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
    Copy link
    Mannequin Author

    dyjakan mannequin commented Dec 20, 2016

    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

    @dyjakan dyjakan mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Dec 20, 2016
    @ammaraskar
    Copy link
    Member

    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
    Copy link
    Member

    methane commented Dec 27, 2016

    LGTM

    @ammaraskar
    Copy link
    Member

    Updated patch based on Rietveld review

    @serhiy-storchaka
    Copy link
    Member

    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 serhiy-storchaka self-assigned this Dec 29, 2016
    @serhiy-storchaka
    Copy link
    Member

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 1, 2017

    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
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @methane @ammaraskar @serhiy-storchaka