python / cpython

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

Missing audit hooks in several extension modules #115322

Open RobinJadoul opened 9 months ago

RobinJadoul commented 9 months ago

Bug report

Bug description:

Several extension modules don't fully emit the relevant audit events, leading to file read or process spawning without any traceability. In particular:

I'm happy to make a quick PR for these and adjust any specific event types to be more consistent or more uniquely identifiable.

Quick example in code:

import sys, ctypes, multiprocessing.util, readline

collected = []
def collector(event, *_args):
    collected.append(event)
sys.addaudithook(collector)

def test(fn, *args, **kw):
    collected.clear()
    fn(*args, **kw)
    if collected:  # Just check it's nonempty
        print("Success")
    else:
        print("Fail")

test(ctypes.memmove, 0, 0, 0)
test(ctypes.CFUNCTYPE(ctypes.py_object), ctypes._memmove_addr)
test(readline.read_history_file, __file__)
test(multiprocessing.util.spawnv_passfds, b"/bin/id", [], [])

CPython versions tested on:

3.11, 3.12, 3.13, CPython main branch

Operating systems tested on:

Linux

Linked PRs

gpshead commented 9 months ago
  • The _posixsubprocess.fork_exec function, and its only user in the standard library, multiprocessing.util.spawnv_passfds perform a

The primary user of that is subprocess, not just multiprocessing's spawn start method.

subprocess already has an audit hook of its own via sys.audit: https://github.com/python/cpython/blob/main/Lib/subprocess.py#L1827 Along those lines, it'd be simpler and consistent to just call sys.audit from within multiprocessing.util.spawnv_passfds, but I think your point of this issue was more to see it consistently done at the extension module API level?

There is symmetry in that the private _winapi.CreateProcess API also triggers an audit hook in addition to subprocess.Popen before it calls it so I'm okay with the concept of adding a _posixsubprocess.fork_exec audit event as your PR does.

RobinJadoul commented 9 months ago

Ah, yes, I have no idea how I missed subprocess when looking for callers...

It's indeed the goal to have some fewer "holes" or better coverage from the audit hooks here. That includes directly calling into the extension modules or callers that don't do their own hook (like the multiprocessing one). In general, the closer to the actual operation happening, the less chance to miss it.

The new event was indeed based on the winapi ones.

devdanzin commented 1 month ago

Should _gdbm.open() also emit an audit event? It can be used to overwrite a file, besides creating a new one.

RobinJadoul commented 1 month ago

Looks like a good catch that should probably be covered too. It may need a bit of translation to not confuse existing consumers of the open event though, as the flags and mode are noticeably different in _gdbm.open.