classification
Title: Type confusion in partial_setstate and partial_call leads to memory corruption
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Ned Williamson, ncoghlan, python-dev, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-12-25 04:16 by Ned Williamson, last changed 2016-02-02 16:59 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
partialpoc2.py Ned Williamson, 2015-12-25 04:16
partial_setstate.patch serhiy.storchaka, 2015-12-25 11:07 review
Messages (6)
msg256976 - (view) Author: Ned Williamson (Ned Williamson) Date: 2015-12-25 04:16
static PyObject *
partial_setstate(partialobject *pto, PyObject *state)
{
    PyObject *fn, *fnargs, *kw, *dict;
    if (!PyArg_ParseTuple(state, "OOOO",
                          &fn, &fnargs, &kw, &dict))
        return NULL;
    Py_XDECREF(pto->fn);
    Py_XDECREF(pto->args);
    Py_XDECREF(pto->kw);
    Py_XDECREF(pto->dict);
    pto->fn = fn;
    pto->args = fnargs; //we control pto->args here

`partial_setstate` performs no checks on the objects
it is passed as an argument.

static PyObject *
partial_call(partialobject *pto, PyObject *args, PyObject *kw)
{
    PyObject *ret;
    PyObject *argappl = NULL, *kwappl = NULL;

    assert (PyCallable_Check(pto->fn));
    assert (PyTuple_Check(pto->args)); //assume pto->args is a tuple
                                       //assertion not present in release build
    assert (pto->kw == Py_None  ||  PyDict_Check(pto->kw));

    if (PyTuple_GET_SIZE(pto->args) == 0) {
        argappl = args;
        Py_INCREF(args);
    } else if (PyTuple_GET_SIZE(args) == 0) {
        argappl = pto->args; //partial function called with no arguments
        Py_INCREF(pto->args);
    } else {
        argappl = PySequence_Concat(pto->args, args);
        if (argappl == NULL)
            return NULL;
    }

    if (pto->kw == Py_None) {
        kwappl = kw;
        Py_XINCREF(kw);
    } else {
        kwappl = PyDict_Copy(pto->kw);
        if (kwappl == NULL) {
            Py_DECREF(argappl);
            return NULL;
        }
        if (kw != NULL) {
            if (PyDict_Merge(kwappl, kw, 1) != 0) {
                Py_DECREF(argappl);
                Py_DECREF(kwappl);
                return NULL;
            }
        }
    }
    ret = PyObject_Call(pto->fn, argappl, kwappl); //pto->fn called with non-tuple argappl

We can see that in the provided POC there is an increment on a user-controlled address (in this case, the literal refcount of a given "argument" is interpreted as a pointer), as `_PyEval_EvalCodeWithName` does not validate the type of `PyObject **args` either (I assume this is a fair assumption for `_PyEval_EvalCodeWithName`, and the bug simply lies in the unsafe partial code.

vagrant@vagrant-ubuntu-wily-64:/vagrant/Python-3.5.1$ gdb -q ./python.exe
...
(gdb) r partialpoc2.py
Starting program: /vagrant/Python-3.5.1/python.exe partialpoc2.py
...
Program received signal SIGSEGV, Segmentation fault.
_PyEval_EvalCodeWithName (_co=0x7ffff7045ae0, globals=<optimized out>, locals=locals@entry=0x0, args=args@entry=0x7ffff6fbc520, argcount=1280, kws=kws@entry=0x0, kwcount=0, defs=0x0, defcount=0,
    kwdefs=0x0, closure=0x0, name=0x0, qualname=0x0) at Python/ceval.c:3793
3793	            Py_INCREF(x);
(gdb) i r
rax            0x9b4b68	10177384
rbx            0x7ffff6fbc520	140737337083168
rcx            0x1	1
rdx            0x2	2
rsi            0x500	1280
rdi            0x0	0
rbp            0x0	0x0
rsp            0x7fffffffdb30	0x7fffffffdb30
r8             0x500	1280
r9             0x0	0
r10            0x7ffff74a6c58	140737342237784
r11            0x9b4b40	10177344
r12            0x0	0
r13            0x0	0
r14            0x7ffff6fb91e0	140737337070048
r15            0x7ffff7e1a048	140737352147016
rip            0x4fc771	0x4fc771 <_PyEval_EvalCodeWithName+961>
eflags         0x10202	[ IF RF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0
(gdb) x/3i $pc
=> 0x4fc771 <_PyEval_EvalCodeWithName+961>:	addq   $0x1,(%rsi)
   0x4fc775 <_PyEval_EvalCodeWithName+965>:	cmp    %edx,%r8d
   0x4fc778 <_PyEval_EvalCodeWithName+968>:	mov    %rsi,0x18(%rax,%rcx,8)
msg256984 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-25 11:07
There are other bugs in partial() that lead to crash, leak, or invalid behavior. Proposed patch fixes these bugs.
msg258196 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-01-14 10:38
I didn't do a full review of the C code changes, but the new test cases look good to me, and the changes specifically to partial_setstate also look good.
msg259360 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-02-02 05:23
This looks correct.
msg259398 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-02-02 15:49
Thank you Nick and Raymond for your reviews.
msg259401 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-02 16:46
New changeset 542b5744ddc3 by Serhiy Storchaka in branch '3.5':
Issue #25945: Fixed bugs in functools.partial.
https://hg.python.org/cpython/rev/542b5744ddc3

New changeset 33109176538d by Serhiy Storchaka in branch 'default':
Issue #25945: Fixed bugs in functools.partial.
https://hg.python.org/cpython/rev/33109176538d

New changeset 628ce2975e29 by Serhiy Storchaka in branch '2.7':
Issue #25945: Fixed bugs in functools.partial.
https://hg.python.org/cpython/rev/628ce2975e29
History
Date User Action Args
2016-02-02 16:59:49serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-02-02 16:46:27python-devsetnosy: + python-dev
messages: + msg259401
2016-02-02 15:49:55serhiy.storchakasetmessages: + msg259398
2016-02-02 05:23:42rhettingersetassignee: rhettinger -> serhiy.storchaka
messages: + msg259360
2016-01-14 10:38:08ncoghlansetmessages: + msg258196
2016-01-14 08:02:57serhiy.storchakasetassignee: serhiy.storchaka -> rhettinger
2016-01-14 07:27:01serhiy.storchakalinkissue26104 superseder
2016-01-03 02:03:00martin.panterlinkissue25944 superseder
2015-12-25 11:07:57serhiy.storchakasetfiles: + partial_setstate.patch
keywords: + patch
messages: + msg256984

stage: needs patch -> patch review
2015-12-25 07:08:11serhiy.storchakasetversions: + Python 2.7
nosy: + rhettinger, ncoghlan, serhiy.storchaka

assignee: serhiy.storchaka
components: + Extension Modules, - Library (Lib)
stage: needs patch
2015-12-25 04:17:17Ned Williamsonsetcomponents: + Library (Lib)
2015-12-25 04:16:41Ned Williamsoncreate