Invalid read in PyCode_New

[bradlarsen]

BPO	24618
Nosy	@serhiy-storchaka, @bradlarsen, @miss-islington
PRs	bpo-24618: Add a check in the code constructor. #8283 [3.7] bpo-24618: Add a check in the code constructor. (GH-8283) #8291 [3.6] bpo-24618: Add a check in the code constructor. (GH-8283) #8311
Files	invalid_read.py: POC as Python 3.4 script

^{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 = None
closed_at = <Date 2018-07-17.07:49:27.549>
created_at = <Date 2015-07-12.17:46:46.727>
labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
title = 'Invalid read in PyCode_New'
updated_at = <Date 2018-07-17.07:49:27.546>
user = 'https://github.com/bradlarsen'

bugs.python.org fields:

activity = <Date 2018-07-17.07:49:27.546>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-07-17.07:49:27.549>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2015-07-12.17:46:46.727>
creator = 'blarsen'
dependencies = []
files = ['39914']
hgrepos = []
issue_num = 24618
keywords = ['patch']
message_count = 4.0
messages = ['246658', '321710', '321713', '321802']
nosy_count = 3.0
nosy_names = ['serhiy.storchaka', 'blarsen', 'miss-islington']
pr_nums = ['8283', '8291', '8311']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue24618'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[bradlarsen]

PyCode_New can read invalid heap memory.

File Objects/codeobject.c:

    PyCodeObject *
    PyCode_New(int argcount, int kwonlyargcount,
               int nlocals, int stacksize, int flags,
               PyObject *code, PyObject *consts, PyObject *names,
               PyObject *varnames, PyObject *freevars, PyObject *cellvars,
               PyObject *filename, PyObject *name, int firstlineno,
               PyObject *lnotab)
    {
        PyCodeObject *co;
        unsigned char *cell2arg = NULL;
        Py_ssize_t i, n_cellvars;

    // ...

    n_cellvars = PyTuple_GET_SIZE(cellvars);

    // ...

        /* Create mapping between cells and arguments if needed. */
        if (n_cellvars) {
            Py_ssize_t total_args = argcount + kwonlyargcount +
                ((flags & CO_VARARGS) != 0) + ((flags & CO_VARKEYWORDS) != 0); // *** 1 ***

        // ...

            /* Find cells which are also arguments. */
            for (i = 0; i < n_cellvars; i++) {
                Py_ssize_t j;
                PyObject *cell = PyTuple_GET_ITEM(cellvars, i); 
                for (j = 0; j < total_args; j++) {
                    PyObject *arg = PyTuple_GET_ITEM(varnames, j);             // *** 2 ***
                    if (!PyUnicode_Compare(cell, arg)) {                       // *** 3 ***
                        cell2arg[i] = j;
                        used_cell2arg = 1;
                        break;
                    }   
                }   
            }
            
            // ...
        }

    // ...
}

1. total_args is determined from parameters that are user-controlled
   (see r_object in [Python/marshal.c](https://github.com/python/cpython/blob/main/Python/marshal.c), in the TYPE_CODE case,
   lines 1265--1277).
2. the varnames tuple is indexed with a value in the range [0, total_args),
   which could be larger than the range of valid indexes for varnames.
3. arg is now a bogus PyObject value, and causes a segfault in
   PyUnicode_Compare.

Environment:

    $ python3.4 --version
    Python 3.4.2

    $ uname -a
    Linux debian-8-amd64 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt9-3~deb8u1 (2015-04-24) x86_64 GNU/Linux

POC:

    from marshal import load
    from io import BytesIO

    payload = b'\xe3\x01\x00\xff\x00\x00\x00\x00\xfc\x01\x00\x00\x00\x02\x00\x00\x00C\x00\x00\x00s\n\x00\x00\x00k\x00\x00|\x00\x00\x83\x01\x00S)\x01N)\x01\xda\x03foo)\x01\xda\x01x\xa9\x01\xda|x\xa9x\xa9\x00\xe3\x01\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00C\x00\x00\x00s\n\x00\x00\x00t\x00\x00|\x00\x00\x83\x01\x00S)\x01N)\x01\xda\x03foo)\x01\xda\x01x\xa9\x00r\x03\x00\x00\x00\xfa\x0fmk_tesTgases.py\xda\x08<lambda>\x01\x00\x00\x00s\x00\x00\x00\x00r\x03\x00\x00\x00\xfa\x0fmk_testck_te\x00)\x01\xda\x01x\xa9x\xa9\x00\xe3\x01\x00\x00\x00\x00\x00\x80\x00\x01\x00\x00\x00\x02\x00\x00\x00C\x00\x00\x00s\n\x00\x00\x00t\x00\x00"\x00\x00\x83\x01\x00S)\x01N)\x01\xda\x03foo)\x01\xda\x01x\xa9\x00r\x03\x00\x00\x00\xfa\x0fmk_tMstgases\x11py\xda\x08<lambda>$\x00\x00\x12s\x00\x00\x00\x00r\x03\x00'
    load(BytesIO(payload))

[serhiy-storchaka]

New changeset bd47384 by Serhiy Storchaka in branch 'master':
bpo-24618: Add a check in the code constructor. (GH-8283)
bd47384

[miss-islington]

New changeset 5594f1d by Miss Islington (bot) in branch '3.7':
bpo-24618: Add a check in the code constructor. (GH-8283)
5594f1d

[serhiy-storchaka]

New changeset cf30d5c by Serhiy Storchaka in branch '3.6':
bpo-24618: Add a check in the code constructor. (GH-8283) (GH-8311)
cf30d5c