python / cpython

The Python programming language
https://www.python.org
Other
62.64k stars 30.05k forks source link

Early auditing broken #81686

Open tiran opened 5 years ago

tiran commented 5 years ago
BPO 37505
Nosy @vstinner, @tiran, @zooba

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: ```python assignee = None closed_at = None created_at = labels = ['type-security', 'interpreter-core', '3.9', '3.10', '3.11'] title = 'Early auditing broken' updated_at = user = 'https://github.com/tiran' ``` bugs.python.org fields: ```python activity = actor = 'christian.heimes' assignee = 'none' closed = False closed_date = None closer = None components = ['Interpreter Core'] creation = creator = 'christian.heimes' dependencies = [] files = [] hgrepos = [] issue_num = 37505 keywords = [] message_count = 9.0 messages = ['347334', '347344', '347345', '347353', '347357', '347358', '347360', '347536', '347538'] nosy_count = 3.0 nosy_names = ['vstinner', 'christian.heimes', 'steve.dower'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'security' url = 'https://bugs.python.org/issue37505' versions = ['Python 3.9', 'Python 3.10', 'Python 3.11'] ```

tiran commented 5 years ago

I think that commit 838f26402de82640698c38ea9d2be65c6cf780d6 / bpo-36710 broke auditing for early events. I'm no longer seeing early events like cpython.PyInterpreterState_New. The first event is an import event without interpreter state.

vstinner commented 5 years ago

Oh. How can I reproduce this issue?

tiran commented 5 years ago

3.8.0b1 is also broken, so it may have been a different commit. I'm sure that I was able to see interpreter initialization with dtrace hooks.

# audit.stp probe process("/usr/lib64/libpython3.8.*").provider("python").mark("audit") { printf("%s\n", user_string($arg1)) }

$ sudo stap audit.stp -c python3.8
vstinner commented 5 years ago

I don't see how the first call to PyInterpreterState_New() can be audited: PySys_Audit() builds a tuple to call hooks, but there is no interpreter yet, so we cannot even build tuples.

Are you talking about the "main" interpreter, or sub-interpreters?

I'm surprised that it worked previously for the main interpreter.

zooba commented 5 years ago

Yeah, at some point we had to add an initialization check to the audit calls because of the tuple, so it essentially became a subinterpreter event but not the main one.

But I thought the dtrace call happened before that check? Looking through the linked commit, apparently not, but I don't actually recall right now why and I can't quite see enough context. Did we need the tuple there too?

vstinner commented 5 years ago

PySys_Audit() exit immediately if ts=NULL:

    /* Early exit when no hooks are registered */
    if (!should_audit(ts)) {
        return 0;
    }

It exits before calling:

    /* Dtrace USDT point */
    if (dtrace) {
        PyDTrace_AUDIT(event, (void *)eventArgs);
    }

where eventArgs is the tuple.

Do you really care of getting an audit event when the *main* interpreter is created? It doesn't sound like an attack vector to start Python, no? If you need an event "Python started", we can add one later, when the PySys_Audit() is usable.

tiran commented 5 years ago

The hooks are about auditing the behavior of an interpreter. It's not strictly tight to some attack scenario. I would find it useful to either get back the old behavior or to have some sort of event, which indicates the start of the auditing log.

tiran commented 5 years ago

Some audit events are designed to work before the interpreter is fully initialized or already shut down. These events pass in NULL instead of a PyObject*.

Python/pystate.c: if (PySys_Audit("cpython.PyInterpreterState_New", NULL) \< 0) { Python/pystate.c: if (PySys_Audit("cpython.PyInterpreterState_Clear", NULL) \< 0) { Python/sysmodule.c: PySys_Audit("cpython._PySys_ClearAuditHooks", NULL);

(They are also currently not documented)

zooba commented 5 years ago

Passing a NULL format string there means the same as passing NULL to PyObject_CallFunction(func, NULL) - no arguments, which results in an empty tuple being passed to the hooks.

Perhaps in the early cases we can pass NULL instead of a tuple? Maybe even assert when that case occurs and the event has parameters?