Skip to content

Commit 2666a06

Browse files
RobinJadoulgpshead
RobinJadoul
and
gpshead
authored
GH-115322: Add missing audit hooks (GH-115624)
Add extra audit hooks to catch C function calling from ctypes, reading/writing files through readline and executing external programs through _posixsubprocess. * Make audit-tests for open pass when readline.append_history_file is unavailable * Less direct testing of _posixsubprocess for audit hooks * Also remove the audit hook from call_cdeclfunction now that _ctypes_callproc does it instead. * reword the NEWS entry. * mention readline in NEWS * add versionchanged markers * fix audit_events.rst versionadded * doc lint --------- Co-authored-by: Gregory P. Smith <greg@krypto.org>
1 parent ce75351 commit 2666a06

File tree

9 files changed

+179
-36
lines changed

9 files changed

+179
-36
lines changed

‎Doc/library/audit_events.rst

+27-22
Original file line numberDiff line numberDiff line change
@@ -23,25 +23,30 @@ information on handling these events.
2323
The following events are raised internally and do not correspond to any
2424
public API of CPython:
2525

26-
+--------------------------+-------------------------------------------+
27-
| Audit event | Arguments |
28-
+==========================+===========================================+
29-
| _winapi.CreateFile | ``file_name``, ``desired_access``, |
30-
| | ``share_mode``, ``creation_disposition``, |
31-
| | ``flags_and_attributes`` |
32-
+--------------------------+-------------------------------------------+
33-
| _winapi.CreateJunction | ``src_path``, ``dst_path`` |
34-
+--------------------------+-------------------------------------------+
35-
| _winapi.CreateNamedPipe | ``name``, ``open_mode``, ``pipe_mode`` |
36-
+--------------------------+-------------------------------------------+
37-
| _winapi.CreatePipe | |
38-
+--------------------------+-------------------------------------------+
39-
| _winapi.CreateProcess | ``application_name``, ``command_line``, |
40-
| | ``current_directory`` |
41-
+--------------------------+-------------------------------------------+
42-
| _winapi.OpenProcess | ``process_id``, ``desired_access`` |
43-
+--------------------------+-------------------------------------------+
44-
| _winapi.TerminateProcess | ``handle``, ``exit_code`` |
45-
+--------------------------+-------------------------------------------+
46-
| ctypes.PyObj_FromPtr | ``obj`` |
47-
+--------------------------+-------------------------------------------+
26+
+----------------------------+-------------------------------------------+
27+
| Audit event | Arguments |
28+
+============================+===========================================+
29+
| _winapi.CreateFile | ``file_name``, ``desired_access``, |
30+
| | ``share_mode``, ``creation_disposition``, |
31+
| | ``flags_and_attributes`` |
32+
+----------------------------+-------------------------------------------+
33+
| _winapi.CreateJunction | ``src_path``, ``dst_path`` |
34+
+----------------------------+-------------------------------------------+
35+
| _winapi.CreateNamedPipe | ``name``, ``open_mode``, ``pipe_mode`` |
36+
+----------------------------+-------------------------------------------+
37+
| _winapi.CreatePipe | |
38+
+----------------------------+-------------------------------------------+
39+
| _winapi.CreateProcess | ``application_name``, ``command_line``, |
40+
| | ``current_directory`` |
41+
+----------------------------+-------------------------------------------+
42+
| _winapi.OpenProcess | ``process_id``, ``desired_access`` |
43+
+----------------------------+-------------------------------------------+
44+
| _winapi.TerminateProcess | ``handle``, ``exit_code`` |
45+
+----------------------------+-------------------------------------------+
46+
| _posixsubprocess.fork_exec | ``exec_list``, ``args``, ``env`` |
47+
+----------------------------+-------------------------------------------+
48+
| ctypes.PyObj_FromPtr | ``obj`` |
49+
+----------------------------+-------------------------------------------+
50+
51+
.. versionadded:: next
52+
The ``_posixsubprocess.fork_exec`` internal audit event.

‎Doc/library/ctypes.rst

+2-1
Original file line numberDiff line numberDiff line change
@@ -1779,7 +1779,8 @@ in :mod:`!ctypes`) which inherits from the private :class:`_CFuncPtr` class:
17791779

17801780
.. audit-event:: ctypes.call_function func_pointer,arguments foreign-functions
17811781

1782-
Some ways to invoke foreign function calls may raise an auditing event
1782+
Some ways to invoke foreign function calls as well as some of the
1783+
functions in this module may raise an auditing event
17831784
``ctypes.call_function`` with arguments ``function pointer`` and ``arguments``.
17841785

17851786
.. _ctypes-function-prototypes:

‎Doc/library/readline.rst

+23-3
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,12 @@ The following functions relate to the init file and user configuration:
7272

7373
Execute a readline initialization file. The default filename is the last filename
7474
used. This calls :c:func:`!rl_read_init_file` in the underlying library.
75+
It raises an :ref:`auditing event <auditing>` ``open`` with the file name
76+
if given, and :code:`"<readline_init_file>"` otherwise, regardless of
77+
which file the library resolves.
78+
79+
.. versionchanged:: next
80+
The auditing event was added.
7581

7682

7783
Line buffer
@@ -109,14 +115,24 @@ The following functions operate on a history file:
109115

110116
Load a readline history file, and append it to the history list.
111117
The default filename is :file:`~/.history`. This calls
112-
:c:func:`!read_history` in the underlying library.
118+
:c:func:`!read_history` in the underlying library
119+
and raises an :ref:`auditing event <auditing>` ``open`` with the file
120+
name if given and :code:`"~/.history"` otherwise.
121+
122+
.. versionchanged:: next
123+
The auditing event was added.
113124

114125

115126
.. function:: write_history_file([filename])
116127

117128
Save the history list to a readline history file, overwriting any
118129
existing file. The default filename is :file:`~/.history`. This calls
119-
:c:func:`!write_history` in the underlying library.
130+
:c:func:`!write_history` in the underlying library and raises an
131+
:ref:`auditing event <auditing>` ``open`` with the file name if given and
132+
:code:`"~/.history"` otherwise.
133+
134+
.. versionchanged:: next
135+
The auditing event was added.
120136

121137

122138
.. function:: append_history_file(nelements[, filename])
@@ -125,10 +141,14 @@ The following functions operate on a history file:
125141
:file:`~/.history`. The file must already exist. This calls
126142
:c:func:`!append_history` in the underlying library. This function
127143
only exists if Python was compiled for a version of the library
128-
that supports it.
144+
that supports it. It raises an :ref:`auditing event <auditing>` ``open``
145+
with the file name if given and :code:`"~/.history"` otherwise.
129146

130147
.. versionadded:: 3.5
131148

149+
.. versionchanged:: next
150+
The auditing event was added.
151+
132152

133153
.. function:: get_history_length()
134154
set_history_length(length)

‎Lib/test/audit-tests.py

+58
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,17 @@ def test_open(testfn):
195195
except ImportError:
196196
load_dh_params = None
197197

198+
try:
199+
import readline
200+
except ImportError:
201+
readline = None
202+
203+
def rl(name):
204+
if readline:
205+
return getattr(readline, name, None)
206+
else:
207+
return None
208+
198209
# Try a range of "open" functions.
199210
# All of them should fail
200211
with TestHook(raise_on_events={"open"}) as hook:
@@ -204,6 +215,14 @@ def test_open(testfn):
204215
(open, 3, "wb"),
205216
(open, testfn, "w", -1, None, None, None, False, lambda *a: 1),
206217
(load_dh_params, testfn),
218+
(rl("read_history_file"), testfn),
219+
(rl("read_history_file"), None),
220+
(rl("write_history_file"), testfn),
221+
(rl("write_history_file"), None),
222+
(rl("append_history_file"), 0, testfn),
223+
(rl("append_history_file"), 0, None),
224+
(rl("read_init_file"), testfn),
225+
(rl("read_init_file"), None),
207226
]:
208227
if not fn:
209228
continue
@@ -229,6 +248,14 @@ def test_open(testfn):
229248
(3, "w"),
230249
(testfn, "w"),
231250
(testfn, "rb") if load_dh_params else None,
251+
(testfn, "r") if readline else None,
252+
("~/.history", "r") if readline else None,
253+
(testfn, "w") if readline else None,
254+
("~/.history", "w") if readline else None,
255+
(testfn, "a") if rl("append_history_file") else None,
256+
("~/.history", "a") if rl("append_history_file") else None,
257+
(testfn, "r") if readline else None,
258+
("<readline_init_file>", "r") if readline else None,
232259
]
233260
if i is not None
234261
],
@@ -278,6 +305,37 @@ def test_mmap():
278305
assertEqual(hook.seen[0][1][:2], (-1, 8))
279306

280307

308+
def test_ctypes_call_function():
309+
import ctypes
310+
import _ctypes
311+
312+
with TestHook() as hook:
313+
_ctypes.call_function(ctypes._memmove_addr, (0, 0, 0))
314+
assert ("ctypes.call_function", (ctypes._memmove_addr, (0, 0, 0))) in hook.seen
315+
316+
ctypes.CFUNCTYPE(ctypes.c_voidp)(ctypes._memset_addr)(1, 0, 0)
317+
assert ("ctypes.call_function", (ctypes._memset_addr, (1, 0, 0))) in hook.seen
318+
319+
with TestHook() as hook:
320+
ctypes.cast(ctypes.c_voidp(0), ctypes.POINTER(ctypes.c_char))
321+
assert "ctypes.call_function" in hook.seen_events
322+
323+
with TestHook() as hook:
324+
ctypes.string_at(id("ctypes.string_at") + 40)
325+
assert "ctypes.call_function" in hook.seen_events
326+
assert "ctypes.string_at" in hook.seen_events
327+
328+
329+
def test_posixsubprocess():
330+
import multiprocessing.util
331+
332+
exe = b"xxx"
333+
args = [b"yyy", b"zzz"]
334+
with TestHook() as hook:
335+
multiprocessing.util.spawnv_passfds(exe, args, ())
336+
assert ("_posixsubprocess.fork_exec", ([exe], args, None)) in hook.seen
337+
338+
281339
def test_excepthook():
282340
def excepthook(exc_type, exc_value, exc_tb):
283341
if exc_type is not RuntimeError:

‎Lib/test/test_audit.py

+8
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,14 @@ def test_cantrace(self):
8080
def test_mmap(self):
8181
self.do_test("test_mmap")
8282

83+
def test_ctypes_call_function(self):
84+
import_helper.import_module("ctypes")
85+
self.do_test("test_ctypes_call_function")
86+
87+
def test_posixsubprocess(self):
88+
import_helper.import_module("_posixsubprocess")
89+
self.do_test("test_posixsubprocess")
90+
8391
def test_excepthook(self):
8492
returncode, events, stderr = self.run_python("test_excepthook")
8593
if not returncode:

‎Misc/NEWS.d/next/Security/2024-02-18-02-53-25.gh-issue-115322.Um2Sjx.rst

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
The underlying extension modules behind :mod:`readline`:, :mod:`subprocess`,
2+
and :mod:`ctypes` now raise audit events on previously uncovered code paths
3+
that could lead to file system access related to C function calling and
4+
external binary execution.

‎Modules/_ctypes/callproc.c

+6-8
Original file line numberDiff line numberDiff line change
@@ -1198,6 +1198,12 @@ PyObject *_ctypes_callproc(ctypes_state *st,
11981198
void **avalues;
11991199
PyObject *retval = NULL;
12001200

1201+
// Both call_function and call_cdeclfunction call us:
1202+
if (PySys_Audit("ctypes.call_function", "nO",
1203+
(Py_ssize_t)pProc, argtuple) < 0) {
1204+
return NULL;
1205+
}
1206+
12011207
n = argcount = PyTuple_GET_SIZE(argtuple);
12021208
#ifdef MS_WIN32
12031209
/* an optional COM object this pointer */
@@ -1673,10 +1679,6 @@ call_function(PyObject *self, PyObject *args)
16731679
&_parse_voidp, &func,
16741680
&PyTuple_Type, &arguments))
16751681
return NULL;
1676-
if (PySys_Audit("ctypes.call_function", "nO",
1677-
(Py_ssize_t)func, arguments) < 0) {
1678-
return NULL;
1679-
}
16801682

16811683
ctypes_state *st = get_module_state(self);
16821684
result = _ctypes_callproc(st,
@@ -1710,10 +1712,6 @@ call_cdeclfunction(PyObject *self, PyObject *args)
17101712
&_parse_voidp, &func,
17111713
&PyTuple_Type, &arguments))
17121714
return NULL;
1713-
if (PySys_Audit("ctypes.call_function", "nO",
1714-
(Py_ssize_t)func, arguments) < 0) {
1715-
return NULL;
1716-
}
17171715

17181716
ctypes_state *st = get_module_state(self);
17191717
result = _ctypes_callproc(st,

‎Modules/_posixsubprocess.c

+13
Original file line numberDiff line numberDiff line change
@@ -1203,6 +1203,19 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
12031203
goto cleanup;
12041204
}
12051205

1206+
/* NOTE: user-defined classes may be able to present different
1207+
* executable/argument/env lists to the eventual exec as to this hook.
1208+
* The audit hook receives the original object, so would nevertheless
1209+
* be able to detect weird behaviour, hence we do not add extra
1210+
* complexity or performance penalties to attempt to avoid this. */
1211+
if (PySys_Audit("_posixsubprocess.fork_exec",
1212+
"OOO",
1213+
executable_list,
1214+
process_args,
1215+
env_list) < 0) {
1216+
goto cleanup;
1217+
}
1218+
12061219
/* This must be the last thing done before fork() because we do not
12071220
* want to call PyOS_BeforeFork() if there is any chance of another
12081221
* error leading to the cleanup: code without calling fork(). */

‎Modules/readline.c

+38-2
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,21 @@ readline_read_init_file_impl(PyObject *module, PyObject *filename_obj)
254254
if (filename_obj != Py_None) {
255255
if (!PyUnicode_FSConverter(filename_obj, &filename_bytes))
256256
return NULL;
257+
if (PySys_Audit("open", "OCi", filename_obj, 'r', 0) < 0) {
258+
return NULL;
259+
}
257260
errno = rl_read_init_file(PyBytes_AS_STRING(filename_bytes));
258261
Py_DECREF(filename_bytes);
259-
} else
262+
} else {
263+
/* We have the choice to either try to exactly reproduce the
264+
* logic to find the filename, ignore it, or provide a dummy value.
265+
* In contract to the history file manipulations, there's no
266+
* clear default to choose. */
267+
if (PySys_Audit("open", "sCi", "<readline_init_file>", 'r', 0) < 0) {
268+
return NULL;
269+
}
260270
errno = rl_read_init_file(NULL);
271+
}
261272
if (errno)
262273
return PyErr_SetFromErrno(PyExc_OSError);
263274
disable_bracketed_paste();
@@ -286,10 +297,19 @@ readline_read_history_file_impl(PyObject *module, PyObject *filename_obj)
286297
if (filename_obj != Py_None) {
287298
if (!PyUnicode_FSConverter(filename_obj, &filename_bytes))
288299
return NULL;
300+
if (PySys_Audit("open", "OCi", filename_obj, 'r', 0) < 0) {
301+
return NULL;
302+
}
289303
errno = read_history(PyBytes_AS_STRING(filename_bytes));
290304
Py_DECREF(filename_bytes);
291-
} else
305+
} else {
306+
/* Use the documented default filename here,
307+
* even though readline expands it different internally. */
308+
if (PySys_Audit("open", "sCi", "~/.history", 'r', 0) < 0) {
309+
return NULL;
310+
}
292311
errno = read_history(NULL);
312+
}
293313
if (errno)
294314
return PyErr_SetFromErrno(PyExc_OSError);
295315
Py_RETURN_NONE;
@@ -322,9 +342,17 @@ readline_write_history_file_impl(PyObject *module, PyObject *filename_obj)
322342
if (!PyUnicode_FSConverter(filename_obj, &filename_bytes))
323343
return NULL;
324344
filename = PyBytes_AS_STRING(filename_bytes);
345+
if (PySys_Audit("open", "OCi", filename_obj, 'w', 0) < 0) {
346+
return NULL;
347+
}
325348
} else {
326349
filename_bytes = NULL;
327350
filename = NULL;
351+
/* Use the documented default filename here,
352+
* even though readline expands it different internally. */
353+
if (PySys_Audit("open", "sCi", "~/.history", 'w', 0) < 0) {
354+
return NULL;
355+
}
328356
}
329357
errno = err = write_history(filename);
330358
int history_length = FT_ATOMIC_LOAD_INT_RELAXED(_history_length);
@@ -371,9 +399,17 @@ readline_append_history_file_impl(PyObject *module, int nelements,
371399
if (!PyUnicode_FSConverter(filename_obj, &filename_bytes))
372400
return NULL;
373401
filename = PyBytes_AS_STRING(filename_bytes);
402+
if (PySys_Audit("open", "OCi", filename_obj, 'a', 0) < 0) {
403+
return NULL;
404+
}
374405
} else {
375406
filename_bytes = NULL;
376407
filename = NULL;
408+
/* Use the documented default filename here,
409+
* even though readline expands it different internally. */
410+
if (PySys_Audit("open", "sCi", "~/.history", 'a', 0) < 0) {
411+
return NULL;
412+
}
377413
}
378414
errno = err = append_history(
379415
nelements - libedit_append_replace_history_offset, filename);

0 commit comments

Comments
 (0)