About vulnerabilities in Cpython native code

[urnotmax]

BPO	46280
Nosy	@rhettinger, @terryjreedy, @vstinner, @ericvsmith, @serhiy-storchaka, @pablogsal, @miss-islington, @nanjekyejoannah
PRs	bpo-46280: Minor code improvement flagged in subissues #324 and #325. #30445 bpo-46280: Fix tracemalloc_copy_domain() #30591 [3.10] bpo-46280: Fix tracemalloc_copy_domain() (GH-30591) #30592 [3.9] bpo-46280: Fix tracemalloc_copy_domain() (GH-30591) #30593
Files	CPython3.10.0_vulnerability_report.txt

^{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 = None
created_at = <Date 2022-01-06.13:07:52.809>
labels = ['type-security', 'build', '3.10', '3.11']
title = 'About vulnerabilities in Cpython native code'
updated_at = <Date 2022-01-14.04:35:25.101>
user = 'https://bugs.python.org/urnotmax'

bugs.python.org fields:

activity = <Date 2022-01-14.04:35:25.101>
actor = 'miss-islington'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Build']
creation = <Date 2022-01-06.13:07:52.809>
creator = 'urnotmax'
dependencies = []
files = ['50544']
hgrepos = []
issue_num = 46280
keywords = ['patch']
message_count = 26.0
messages = ['409841', '409854', '409859', '409868', '409884', '409895', '409902', '409904', '409907', '409909', '409911', '409912', '409918', '409921', '409922', '409923', '409925', '409926', '409927', '409929', '409930', '409933', '410539', '410540', '410541', '410542']
nosy_count = 8.0
nosy_names = ['rhettinger', 'terry.reedy', 'vstinner', 'eric.smith', 'serhiy.storchaka', 'pablogsal', 'miss-islington', 'nanjekyejoannah']
pr_nums = ['30445', '30591', '30592', '30593']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue46280'
versions = ['Python 3.10', 'Python 3.11']

[urnotmax]

I am currently doing some research on the security of CPython. I used the open source vulnerability analysis engine, Infer(https://fbinfer.com/), to scan the native code of CPython 3.10.0.

The scan results show that there are still a number of vulnerabilities in the CPython native code, such as Null dereference, Uninitialized variable, Resource/Memory leak, etc. Moreover, I found that some of the vulnerabilities are related to Python/C API. I enclose the vulnerability report for your reference.

Based on the research of the result, I tried to design a tool to automatically detect and repair vulnerabilities in CPython and make this tool available. See:

https://github.com/PVMPATCH/PVMPatch

Python is my favourite programming language. I sincerely hope that I can help Python become stronger and safer. I hope this discovery can be useful for you to develop Python in the future.

[ericvsmith]

Thank you for posting this.

Some of these look like false positives.

For example:
#263
Parser/string_parser.c:670: error: Uninitialized Value
The value read from parenstack[_] was never initialized.
668. }
669. nested_depth--;
670. int opening = (unsigned char)parenstack[nested_depth];
^
671. if (!((opening == '(' && ch == ')') ||
672. (opening == '[' && ch == ']') ||

I don't see how this could be an uninitialized read, although I'm willing to be wrong.

If your tool can produce patches to fix reported problems, I suggest that you create PRs for specific issues, so they can be reviewed individually. There's no way we'd review a single patch for all 673 issues that were identified.

Also, looking at the first one:
#0
Objects/clinic/bytearrayobject.c.h:50: error: Dead Store
The value written to &noptargs (type long) is never used.
48. goto exit;
49. }
50. if (!--noptargs) {
^
51. goto skip_optional_pos;
52. }

We've discussed this before. The consensus last time was to leave code like this in place, in case other code was added after this that refers to the same pointer. Our assumption is that compilers will remove the unneeded store. Is it possible to remove Dead Stores from the output, and/or produce a separate output with just Dead Stores? I don't see how a Dead Store can be a vulnerability.

[pablogsal]

Based on the research of the result, I tried to design a tool to automatically detect and repair vulnerabilities in CPython and make this tool available. See:

You mention here that your tool automatically "repairs" the code. Could you submit a sample PR with the repairs that your tool does so we can evaluate it?

[zware]

As an aside, there's an issue with Roundup where a username composed of all digits causes it to think that name is a user ID in the nosy list. I recommend changing your username to include a non-digit character so that others can interact with you on your issues without constantly removing you from the nosy list.

[nanjekyejoannah]

You mention here that your tool automatically "repairs" the code. Could you >submit a sample PR with the repairs that your tool does so we can evaluate it?

I second this.

I obviously, am assuming good intentions from the author, not experimenting on us.

[pablogsal]

I don't see how this could be an uninitialized read, although I'm willing to be wrong.

It can be uninitialized if the parenstack[nested_depth] value is itself initialized, which can happen if the memory block pointed by parenstack has not been initialized to some value after malloc'ed and parenstack[nested_depth] never got a value.

But yeah, a lot of hypotheticals here that I am sure are not possible in the actual code.

[rhettinger]

#244 is a false positive. The value of new_state[i] on line 454 was initialized on line 442 with: new_state[i] = (uint32_t)element.

#387 is also a false positive. There is an assertion on the previous line that the item != NULL. That assertion passes because item is the result of a call to deque_popleft() which only returns NULL when the size is zero; however, deque_del_item() is not ever called with size 0 and it has an assertion to that effect. The two callers are deque_remove() and deque_ass_item(). Both calls are guarded by regular tests (not assertions) so that they are only called when the index >= 0 and index < len(deque).

[ericvsmith]

I don't want to belabor this, but hey, it's in f-strings! And if it's an actual problem I'd like to fix it.

It can be uninitialized if the parenstack[nested_depth] value is itself initialized, which can happen if the memory block pointed by parenstack has not been initialized to some value after malloc'ed and parenstack[nested_depth] never got a value.

parenstack is allocated on the stack, not that that changes the discussion much.

It looks like everywhere that parenstack[nested_depth] is read, it's already been written to.

Anyway, (if I'm right,) this makes my points that a) there are false positives, and b) we should have separate issues for each actual problem.

[rhettinger]

#324 and #325 are false positives. The result variable is initialized in the preceding lines:

    if (len_a == length) {
        left = *((volatile const unsigned char**)&a);
        result = 0;
    }
    if (len_a != length) {
        left = b;
        result = 1;
    }

While the code is correct, the second test should be changed to "else".

[rhettinger]

#382 is false positive. The "iterable" variable is only accessed when known to not be NULL.

    # Line 970
    if (iterable != NULL) {
        if (set_update_internal(so, iterable)) {
            Py_DECREF(so);
            return NULL;
        }
    }

[rhettinger]

#420 and #421 are false positives. The value of "c" is initialized a few lines before use.

        for (;;) {
            c = tok_nextc(tok);
            ...
        }
        ...
        tok_backup(tok, c);
        if (c == '#' || c == '\n' || c == '\\') {
           ...
        }