GH-115322: Add missing audit hooks #115624
Conversation
Add extra audit hooks to catch C function calling from ctypes, reading/writing files through readline and executing external programs through _posixsubprocess.
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Modules/_posixsubprocess.c
Outdated
| @@ -1200,6 +1200,17 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, | |||
| goto cleanup; | |||
| } | |||
|
|
|||
| /* Does using the original objects here pose a problem by | |||
There was a problem hiding this comment.
I'd edit the comment to not be a question, just something like: "NOTE: user-defined classes may be able to present a different value to this function vs to the audit hook. This is not an intended use case. Attempting to prevent this would add complexity or incur a performance penalty."
Lib/test/audit-tests.py
Outdated
| args = [b'x', b'y'] | ||
| exec_list = [b'xxx', b'yyy'] | ||
| env = [b'A=A'] | ||
| # Based upon `multiprocessing.spawnv_passfds` to figure out arguments |
There was a problem hiding this comment.
Could you please call multiprocessing.util.spawnv_passfds for the test instead? It becomes a maintenance issue to have any more direct callers of this non-human-friendly internal API any time we change it.
There was a problem hiding this comment.
That makes sense, it doesn't exercise the passing of the env_list like before, but I can see the maintainability burden being a lot more important here.
gpshead
added
type-bug
GH-115322: Add missing audit hooks
I've left out the potential
ctypes.cdata/functionevent for now, as constructing a function without notice is not as badwhen you cannot call it without audit hook anymore. Calling some functions in
ctypes(as far as I can see:(w)string_at) now have two associated hooks, going first through the genericctypes.call_functionand then their preexisting specific hook (ctypes.(w)string_at). It would be nice to associated actypes.call_functionwith its function name, but this isn't possible in general (as you could technically call into arbitrary addresses), and I couldn't find a nice way to do it even in the "nice" case.For
_posixsubprocess.fork_exec, I instead opted to introduce a new event of its own, as wrangling the slightly weird setup withexecutable_listand passingenvas a list rather than the usual dict makes it more annoying to (cheaply) map onto the semantics of the existing events. If constructing an explicit dict or passing an env list to those would be prefered, it should be possible to assume the worst case for theexecutable_listand do a hook for each element.In the "normal" case, going through
multiprocessing.util.spawnv_passfds, only a single element would be passed either way.readlinehas a few of its own problems, mostly when dealing with filename arguments being set toNone. For the*_history_filefunctions, I opted to hardcode the~/.historypath that can also be found in the documentation, even though the logic inside readline is slightly more elaborate, e.g. finding a different path when on windows, and expanding out the home directory. Forread_init_file(None), the logic is even more convoluted, and pointing out the exact path would require replicating it and keeping up with any possible future updates. Instead, I chose to represent this by<readline_init_file>.📚 Documentation preview 📚: https://cpython-previews--115624.org.readthedocs.build/