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

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?

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

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.

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: https://github.com/python/cpython/blob/7301979b23406220510dd2c7934a21b41b647119/Objects/funcobject.c#L250

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: https://github.com/python/cpython/blob/7301979b23406220510dd2c7934a21b41b647119/Python/traceback.c#L156-L159

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.

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:
https://github.com/lunixbochs/cpython/commit/2334a00c833874b7a2427e88abc9b51315bb010c

---

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

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.

`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/)

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.

Aaaah, PR 24182 doesn't add a hook to object.__getattr__, but to the C getter functions on traceback and frame. That sounds more acceptable to me :-) These operations are uncommon and should not be part of "hot code" (critical for performance) unless you're doing something crazy :-p

I just found out that generator object variants have their own code attributes. I investigated the stdlib usage and it seems to be for debug / dis only, so adding these attributes shouldn't impact performance.

I updated the PR to now cover the following attributes:

PyTracebackObject.tb_frame
PyFrameObject.f_code
PyGenObject.gi_code
PyCoroObject.cr_code
PyAsyncGenObject.ag_code

I have also attached a `check_hooks.py` file which allows for quick visual inspection that all of the hooks are working (It prints each attribute name, then accesses it. Expected output is an AUDIT line after each attribute printed.)

I agree with Victor, we should not be attempting to build a sandbox.
https://www.python.org/dev/peps/pep-0578/#why-not-a-sandbox

Preventing access to global variables is next to impossible. Adding more and more hooks to prevent access to globals, merely adds the illusion of security. Sooner or later, someone will find a path to globals that would have a serious impact on performance to block.

We should assume that globals are accessible from user code, and write the audit function accordingly. Either in C or using a closure.

My personal motivation is not to unilaterally prevent access to globals, but to close a simpler gap in the audit system that affects a currently deployed high performance production system (which is not trying to be a sandbox). I am also already using a C audit hook for my purposes.

If you are referencing vstinner's first message, please remember to read their follow up https://bugs.python.org/msg384988 where they seem to have changed their mind in support of the patch.

The audit attributes I'm chasing here are fairly small in scope, and overwhelmingly only used in debug code. I believe adding them is in the spirit of the original PEP. I have also done extensive testing and CPython C and stdlib code analysis as part of this effort.

If you agree with the original PEP authors that __code__ and sys._getframe() are worth auditing, then I believe this is a natural extension of that concept. My patch improves upon the PEP by increasing the audit coverage to every way I can see of getting a frame and code object from basic CPython types.

This is a simple patch with clear performance metrics. I don't see any reason to expand the scope of this in the future unless CPython adds another basic object type along the same lines (e.g. a new async function type, a new traceback type, or a new frame type).

If the point of the proposed change is not to deny access to globals, then what is the point of it?

You say that this change is to "close a simpler gap in the audit system".
What it is that the audit system is supposed to prevent, that is currently possible, and that your proposed change would prevent?

I'm worried that we end up adding more and more hooks. Performance and maintainability will suffer.

My understanding as per the outline in PEP 551 as well as PEP 578, is that the audit system is meant primarily to observe the behavior of code rather than to have good sandbox coverage / directly prevent behavior.

I am using audit hooks to observe the behavior of third party Python, and I identified an indicator of shady behavior which includes code and frame object access (which includes sys._getframe(), and __code__, both of which are part of the original PEP 578).

I looked into it further and realized the CPython's auditing for those attributes/objects is superficial. I understand that auditing isn't perfect, and I'm not trying to change that. This patch just seems to me like a really basic and obvious extension of the existing __code__ and getframe audit points.

----

I ask that if your main hesitation is the impact of future audit hooks, we use this opportunity to establish a basic written precedent we can reference in the future about which kind of audit hook modifications are likely to be accepted without, say, another PEP.

One possible set of criteria:
 - The added hooks should be justified as functionally identical to something the existing PEP(s) suggested.
 - Performance should be measured and it should have very little impact on stdlib or general code.
 - The requester should be expected to justify the change, e.g. how it closes an obvious gap in an existing PEP 578 hook.

And my answers for those criteria:
 - These are functionally equivalent to the existing PEP 578 hooks for sys._getframe() and function.__code__ - they operate on similar types of objects and are used for accessing the exact same information.
 - Performance impact here appears to be only for debugging code, and performance impact on debugging code is infinitesimal when no audit hook is active.
 - I am auditing code for trivial usage of Python frames and code objects, and I can't do that sufficiently with the existing hooks (especially so now that I'm publicly documenting this gap).

----

If the primary complaint is maintenance burden, would it be preferable to add an attribute audit flag to PyMemberDef instead of using one-off PyGetSetDef functions? e.g.:

static PyMemberDef frame_memberlist[] = {
    {"f_code",          T_OBJECT,       OFF(f_code),      READONLY|AUDIT_ACCESS},
}

That would definitely simplify the implementation.

If these additions aren't worth it, I would almost recommend removing or deprecating the existing __code__ and sys._getframe() audit hooks instead, as I find them to be not very useful without this patch.

How's this for maintainable?

https://github.com/lunixbochs/cpython/commit/2bf1cc93d19a49cbed09b45f7dbb00212229f0a1

I'm fine with either approach, though adding the READ_RESTRICTED flag would also be fine.

The audit trailing leading to a bypass is very important, and traversing frames to find functions in their locals or closures is very useful.

This is nothing like a traditional sandbox, which we can't do because there are (few) legitimate uses and (many) existing users of our internal interpreter state inspection/mutation APIs. This is about tracing internal events - hence _audit_ events - so we want them for all equivalent ways to achieve the same internal inspection/mutation.

I agree that READ_RESTRICTED would work, and I'm strongly in support of refactoring my patch around that kind of flag, as it simplifies it quite a bit and the if statement is already there.

However, using the seemingly legacy RESTRICTED flag names for audit is confusing in my opinion:

- The audit subsystem does something entirely different from the long deprecated "Restricted execution" feature (removed in 3.0?)
- Nothing in the stdlib uses RESTRICTED that I can see.
- The documentation for RESTRICTED flags (Doc/extending/newtypes.rst) doesn't say anything about the audit system for READ_RESTRICTED, and talks about restricted mode as though it still exists.
- RESTRICTED only supports __getattr__ (PY_WRITE_RESTRICTED does nothing at all, and there is no delattr equivalent). This doesn't actually matter for this patch, it's just confusing in the context of audit, as there are `object.__setattr__` and `object.__delattr__` audit points but no corresponding flags.

I think it could make sense to:
1. Alias READ_RESTRICTED to a new READ_AUDIT flag and use the latter instead, as it is more clear.
2. Update the newtype docs to mention READ_AUDIT and remove documentation for the the unused RESTRICTED flags.
3. Deprecate the non-functional RESTRICTED flags if that's possible?
4. Only cross the setattr/delattr audit flag bridge if a future refactor calls for it.

> I think it could make sense to:
> 1. Alias READ_RESTRICTED to a new READ_AUDIT flag and use the latter instead, as it is more clear.
> 2. Update the newtype docs to mention READ_AUDIT and remove documentation for the the unused RESTRICTED flags.
> 3. Deprecate the non-functional RESTRICTED flags if that's possible?
> 4. Only cross the setattr/delattr audit flag bridge if a future refactor calls for it.

Sounds good to me. We can deprecate RESTRICTED with no intention to 
remove it, since it's documented.

Do you want to prepare a PR for this?

Just updated the PR with another much simpler attempt, using a new READ_AUDIT flag (aliased to READ_RESTRICTED, and newtypes documentation updated).

I re-ran timings for the new build, and in all cases they match or slightly beat my previous reported timings.

> Sounds good to me. We can deprecate RESTRICTED with no intention to 
remove it, since it's documented.
> Do you want to prepare a PR for this?

In case you missed it, the attached PR 24182 as of commit d3e998b is based on the steps I listed - I moved all of the proposed audited properties over to a new AUDIT_READ flag that is much simpler.

Thanks for the ping. I'll try and check in later this week to finish it up.

New changeset 9a2c2a9ec3140b6c54c9ef9d994311f114128ee3 by Ryan Hileman in branch 'master':
bpo-42800: add audit hooks for f_code and tb_frame (GH-24182)
https://github.com/python/cpython/commit/9a2c2a9ec3140b6c54c9ef9d994311f114128ee3

Can you please rename AUDIT_READ to PY_AUDIT_READ? We should avoid adding symbols without Py/PY prefix to the Python C API.

Sure, I can do that.

New changeset 87655e2cf58c543914ea05ebe5a0377441da1ef2 by Steve Dower in branch 'master':
bpo-42800: Rename AUDIT_READ to PY_AUDIT_READ (GH-25736)
https://github.com/python/cpython/commit/87655e2cf58c543914ea05ebe5a0377441da1ef2

The 3.9 backport is a bit different from what's in master, so would appreciate someone double-check it. It should go back to 3.8 just fine.

New changeset bb2f3ff7a8f0c3565ccc1946dba7e09a3f7dc209 by Steve Dower in branch '3.9':
bpo-42800: Add audit events for f_code and tb_frame (GH-24182)
https://github.com/python/cpython/commit/bb2f3ff7a8f0c3565ccc1946dba7e09a3f7dc209

New changeset 8ab272f0f3dd7da44f8e21d2a5a39c2ab39490d6 by Miss Islington (bot) in branch '3.8':
bpo-42800: Add audit events for f_code and tb_frame (GH-24182)
https://github.com/python/cpython/commit/8ab272f0f3dd7da44f8e21d2a5a39c2ab39490d6