Traceback objects allow accessing frame objects without triggering audit hooks

[ammaraskar]

BPO	42800
Nosy	@vstinner, @tiran, @markshannon, @zooba, @ammaraskar, @miss-islington, @lunixbochs
PRs	bpo-42800: add audit hooks for f_code and tb_frame #24182 bpo-42800: Rename AUDIT_READ to PY_AUDIT_READ #25736 [3.9] bpo-42800: Add audit events for f_code and tb_frame (GH-24182) #25737 [3.8] bpo-42800: Add audit events for f_code and tb_frame (GH-24182) #25849
Files	c_audit_ext.zip check_hooks.py: visual test for audit hook changes

^{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:

assignee = 'https://github.com/zooba'
closed_at = <Date 2021-05-03.19:52:38.694>
created_at = <Date 2021-01-01.00:16:58.630>
labels = ['type-security', '3.8', 'library', '3.9', '3.10']
title = 'Traceback objects allow accessing frame objects without triggering audit hooks'
updated_at = <Date 2021-05-03.19:52:38.694>
user = 'https://github.com/ammaraskar'

bugs.python.org fields:

activity = <Date 2021-05-03.19:52:38.694>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2021-05-03.19:52:38.694>
closer = 'steve.dower'
components = ['Library (Lib)']
creation = <Date 2021-01-01.00:16:58.630>
creator = 'ammar2'
dependencies = []
files = ['49729', '49755']
hgrepos = []
issue_num = 42800
keywords = ['patch', 'security_issue']
message_count = 29.0
messages = ['384143', '384704', '384705', '384706', '384710', '384713', '384719', '384733', '384749', '384988', '385401', '385410', '385425', '385429', '385440', '385450', '385512', '385517', '385518', '385519', '387547', '387560', '392359', '392360', '392361', '392365', '392370', '392804', '392806']
nosy_count = 7.0
nosy_names = ['vstinner', 'christian.heimes', 'Mark.Shannon', 'steve.dower', 'ammar2', 'miss-islington', 'lunixbochs2']
pr_nums = ['24182', '25736', '25737', '25849']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue42800'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[ammaraskar]

It is possible to access all the frame objects in the interpret without triggering any audit hooks through the use of exceptions. Namely, through the traceback's tb_frame property. Ordinarily one would trigger the "sys._current_frames" or "sys._getframe" event but this code path bypasses those.

There is already precedent for raising events for certain sensitive properties such as __code__ in funcobject. (through a "object.getattr" event) so perhaps this property should be protected in a similar way.

This issue was recently demonstrated in a security competition:

• https://github.com/hstocks/ctf_writeups/blob/master/2020/hxp/audited/README.md
• https://github.com/fab1ano/hxp-ctf-20/blob/master/audited/README.md

[lunixbochs]

traceback's tb_code attribute also allows you to bypass the object.__getattr__ audit event for __code__.

Perhaps accessing a traceback object's tb_code and tb_frame should both raise an object.__getattr__ event?

[vstinner]

I don't think that audit hooks should be seen as a way to build a robust sandbox.
https://www.python.org/dev/peps/pep-0578/#why-not-a-sandbox

[vstinner]

Even if no audit hook is registered, adding an audit event on a function has a cost on runtime performance. object.__getattr__() is a core Python function, if an event is added, we should properly measure the performance overhead to decide if it's acceptable or not.

[lunixbochs]

I'm definitely not proposing to hook all of object.__getattr__, as my intuition says that would be very slow. I simply refer to "object.__getattr__" as the event name used by a couple of rare event audit hooks. This is how getting __code__ is emitted:

cpython/Objects/funcobject.c

Line 250 in 7301979

func_get_code(PyFunctionObject *op, void *Py_UNUSED(ignored))

However, there's not much point in the sys._getframe and func.__code__ family of audit hooks right now as tracebacks expose the same information (and may even do so accidentally). I am personally interested in these hooks for non sandbox reasons in a production application that cares about perf, FWIW.

I think this would be implemented by extending the traceback object's getters to include tb_code and tb_frame:

cpython/Python/traceback.c

Lines 156 to 159 in 7301979

	static PyGetSetDef tb_getsetters[] = {
	{"tb_next", (getter)tb_next_get, (setter)tb_next_set, NULL, NULL},
	{NULL} /* Sentinel */
	};

I project it won't have any noticeable perf impact (especially if the audit hook is written in C), as most reasons to inspect a traceback object will be exceptional and not in critical paths.

I'd be happy to write a proposed patch if that would help.

[lunixbochs]

Oops, by tb_code I meant traceback.tb_frame.f_code. So you can get to a frame from traceback.tb_frame (without triggering audit) or sys._getframe (which has an audit hook already), and you can get to __code__ from a frame via frame.f_code (without triggering audit).

Here's a patch for both frame.f_code and traceback.tb_frame:
lunixbochs@2334a00

---

Benchmarks follow this section, made using the commit I linked (and the parent commit without the patch for comparison). My takeaways from playing around:

1. You probably shouldn't install a Python audit hook if you care about performance.
2. C audit hook performance impact shows up in microbenchmarking but only have a small impact on real workloads (see the traceback.format_tb benchmark at the end).
3. Performance impact of this change when you _don't_ have an audit hook installed is very small.
4. This seems to mostly impact debugging and test code. A quick check of the stdlib shows:

• traceback.tb_frame usage seems to be entirely in debugger, traceback, and testing code: https://github.com/python/cpython/search?l=Python&p=3&q=tb_frame
• frame.f_code primarily has similar debug use (dis, warnings, profiling, inspect): https://github.com/python/cpython/search?l=Python&p=3&q=f_code

Attached (c_audit_ext.zip) is the empty C audit hook I used for the benchmarks. python3 setup.py build_ext builds a c_audit module which registers an empty audit hook on import.

################################################################################
#### frame.f_code object.__getattr__ audit hook

# Testing frame.f_code impact (no audit hook installed):
./python.exe -m timeit -s 'frame = sys._getframe()' -- 'frame.f_code'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 19.1 nsec per loop
20000000 loops, best of 5: 18.7 nsec per loop
20000000 loops, best of 5: 19.1 nsec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 17 nsec per loop
20000000 loops, best of 5: 16.7 nsec per loop
20000000 loops, best of 5: 17 nsec per loop

# Testing frame.f_code impact (C audit hook installed):
python.exe -m timeit -s 'import c_audit; frame = sys._getframe()' -- 'frame.f_code'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
5000000 loops, best of 5: 66.1 nsec per loop
5000000 loops, best of 5: 66.1 nsec per loop
5000000 loops, best of 5: 66.5 nsec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 16.9 nsec per loop
20000000 loops, best of 5: 16.9 nsec per loop
20000000 loops, best of 5: 16.8 nsec per loop

# Testing frame.f_code impact (pure Python audit hook installed):
./python.exe -m timeit -s 'frame = sys._getframe(); sys.addaudithook(lambda *a: None)' -- 'frame.f_code'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
500000 loops, best of 5: 1.02 usec per loop
500000 loops, best of 5: 1.04 usec per loop
500000 loops, best of 5: 1.02 usec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 16.8 nsec per loop
20000000 loops, best of 5: 17.1 nsec per loop
20000000 loops, best of 5: 16.8 nsec per loop

################################################################################
#### tb.tb_frame object.__getattr__ audit hook

# Testing tb.tb_frame impact (no audit hook installed)
./python.exe -m timeit -s "$(echo -e "try: a\nexcept Exception as e: tb = e.__traceback__")" -- 'tb.tb_frame'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 19.2 nsec per loop
20000000 loops, best of 5: 18.9 nsec per loop
20000000 loops, best of 5: 18.9 nsec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 17 nsec per loop
20000000 loops, best of 5: 16.7 nsec per loop
20000000 loops, best of 5: 16.8 nsec per loop

# Testing tb.tb_frame impact (C audit hook installed)
./python.exe -m timeit -s "$(echo -e "import c_audit\ntry: a\nexcept Exception as e: tb = e.__traceback__")" -- 'tb.tb_frame'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
5000000 loops, best of 5: 64.8 nsec per loop
5000000 loops, best of 5: 64.8 nsec per loop
5000000 loops, best of 5: 64.8 nsec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 16.7 nsec per loop
20000000 loops, best of 5: 16.9 nsec per loop
20000000 loops, best of 5: 16.9 nsec per loop

# Testing tb.tb_frame impact (pure Python audit hook installed)
./python.exe -m timeit -s "$(echo -e "sys.addaudithook(lambda *a: None)\ntry: a\nexcept Exception as e: tb = e.__traceback__")" -- 'tb.tb_frame'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
500000 loops, best of 5: 1.04 usec per loop
500000 loops, best of 5: 1.02 usec per loop
500000 loops, best of 5: 1.04 usec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
20000000 loops, best of 5: 16.9 nsec per loop
20000000 loops, best of 5: 16.9 nsec per loop
20000000 loops, best of 5: 17.2 nsec per loop

################################################################################
#### tb.tb_frame object.__getattr__ audit hook, traceback.format_tb()

# Testing tb.tb_frame impact on traceback.format_tb (no audit hook installed)
with patch 2334a00c833874b7a2427e88abc9b51315bb010c
100000 loops, best of 5: 3 usec per loop
100000 loops, best of 5: 3.03 usec per loop
100000 loops, best of 5: 3 usec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
100000 loops, best of 5: 2.99 usec per loop
100000 loops, best of 5: 3.02 usec per loop
100000 loops, best of 5: 2.99 usec per loop

# Testing tb.tb_frame impact on traceback.format_tb (C audit hook installed)
./python.exe -m timeit -s "$(echo -e "import c_audit\nimport traceback\ntry: a\nexcept Exception as e: tb = e.__traceback__")" -- 'traceback.format_tb(tb)'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
100000 loops, best of 5: 3.13 usec per loop
100000 loops, best of 5: 3.13 usec per loop
100000 loops, best of 5: 3.12 usec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
100000 loops, best of 5: 3.06 usec per loop
100000 loops, best of 5: 3.06 usec per loop
100000 loops, best of 5: 3.05 usec per loop

# Testing tb.tb_frame impact on traceback.format_tb (pure Python audit hook installed)
./python.exe -m timeit -s "$(echo -e "sys.addaudithook(lambda *a: None)\nimport traceback\ntry: a\nexcept Exception as e: tb = e.__traceback__")" -- 'traceback.format_tb(tb)'

with patch 2334a00c833874b7a2427e88abc9b51315bb010c
50000 loops, best of 5: 5.1 usec per loop
50000 loops, best of 5: 5.18 usec per loop
50000 loops, best of 5: 5.06 usec per loop

without patch 2334a00c833874b7a2427e88abc9b51315bb010c
100000 loops, best of 5: 3 usec per loop
100000 loops, best of 5: 3 usec per loop
100000 loops, best of 5: 3 usec per loop

[zooba]

That's the same patch that I'd write, and I agree, we should hook this.

If the fields are documented anywhere, we should add the audit event data to get them into the table in the docs. Otherwise, that patch looks good to me.

[ammaraskar]

tb_frame is documented under https://docs.python.org/3/reference/datamodel.html

Special read-only attributes: tb_frame points to the execution frame of the current level

tb_code can similarly be documented here and the note about the audit event can be added. Thanks for the patch Ryan, would you like to add the documentation change Steve suggested and create a pull request out of it and contribute it? (https://devguide.python.org/pullrequest/)

[lunixbochs]

PR submitted, waiting on CLA process.

I added documentation at the field sites, but the audit event table generation does not handle attributes or object.__getattr__ very well at all, so I'm not updating the audit table for now.

The `.. audit-event:: object.__getattr__ obj,name frame-objects` sphinx directive right now just inserts a canned string """Raises an :ref:`auditing event <auditing>` object.__getattr__ with arguments obj,name.""", which would need additional boilerplate to describe these attributes properly. It also only adds a footnote style link to the audit table under __getattr__, and even moves object.__getattribute__ from the first [1] link position to a later number which is IMO is more confusing than not even linking them.

I think to make attributes look good in the table we would need a special sphinx directive for audited object.__getattr__ attributes, for example by modifying the template generator to fit each attribute on its own line under object.__getattr__ in the table.

For now I did not use the audit-event sphinx directive and manually inserted strings like this near the original attribute description in the docs: """Accessing ``f_code`` raises an :ref:`auditing event <auditing>` ``object.__getattr__`` with arguments ``obj`` and ``"f_code"``."""

I think audit table improvements should be handled in a separate issue, and by someone more familiar with that part of the doc generator, as cleaning it up looks like maybe a bigger scope than the current contribution.