Skip to content

Missing audit hooks in several extension modules #115322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RobinJadoul opened this issue Feb 12, 2024 · 4 comments
Open

Missing audit hooks in several extension modules #115322

RobinJadoul opened this issue Feb 12, 2024 · 4 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@RobinJadoul
Copy link
Contributor

RobinJadoul commented Feb 12, 2024

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:

  • Calling a _ctypes.CFuncPtr does not emit ctypes.call_function. When combined with some known addresses, this can result in arbitrary functions in libc or python getting called. Such addresses could come from id, ctypes.pythonapi._handle, passing a byref pointer to ctypes.cast, or probably still several other methods. Coincidentally, the ctypes.cast method would by audited by the same ctypes.call_function once it is present. The downside is that it may also result in multiple audit hooks for functions like ctypes.string_at that have their own specialized audit hook event too.
  • Related, and maybe debatable, but constructing a _ctypes.CFuncPtr might fall under the audit event ctypes.cdata, as it is in spirit (though not in implementation) similar to calling a .from_address. An option might be to introduce ctypes.cdata/function similar to ctypes.cdata/buffer for this.
  • The readline module can open and read a file through readline.read_history_file without having an open audit hook. Together with readline.get_history_item, this can lead to unaudited file reads. A similar situation exists for some other functions in this library.
  • The _posixsubprocess.fork_exec function, and its only user in the standard library, multiprocessing.util.spawnv_passfds perform a fork + exec without any audit hooks. One would expect either os.fork and os.exec or the functionally similar os.posix_spawn here.

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

@RobinJadoul RobinJadoul added the type-bug An unexpected behavior, bug, or error label Feb 12, 2024
RobinJadoul added a commit to RobinJadoul/cpython that referenced this issue Feb 18, 2024
Add extra audit hooks to catch C function calling from ctypes,
reading/writing files through readline and executing external
programs through _posixsubprocess.
@gpshead gpshead self-assigned this Feb 22, 2024
@gpshead
Copy link
Member

gpshead commented Feb 22, 2024

  • 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
Copy link
Contributor Author

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.

gpshead added a commit to RobinJadoul/cpython that referenced this issue May 7, 2024
gpshead added a commit to RobinJadoul/cpython that referenced this issue May 11, 2024
@devdanzin
Copy link
Contributor

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

@RobinJadoul
Copy link
Contributor Author

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.

gpshead added a commit that referenced this issue Apr 13, 2025
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 <[email protected]>
gpshead added a commit to gpshead/cpython that referenced this issue Apr 14, 2025
…rms.

It was using a signed conversion to communicate the function id (pointer) value.
gpshead added a commit that referenced this issue Apr 14, 2025
…H-132496)

* GH-115322: fix ctypes call_function audit hook on 32-bit platforms.

It was using a signed conversion to communicate the function id (pointer) value.
@picnixz picnixz added the extension-modules C modules in the Modules dir label Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants