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
Traceback objects allow accessing frame objects without triggering audit hooks #86966
Comments
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 This issue was recently demonstrated in a security competition: |
traceback's Perhaps accessing a traceback object's |
I don't think that audit hooks should be seen as a way to build a robust 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: Line 250 in 7301979
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: Lines 156 to 159 in 7301979
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: --- Benchmarks follow this section, made using the commit I linked (and the parent commit without the patch for comparison). My takeaways from playing around:
Attached (c_audit_ext.zip) is the empty C audit hook I used for the benchmarks. ################################################################################ # Testing frame.f_code impact (no audit hook installed): 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 # Testing frame.f_code impact (C audit hook installed): 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 # Testing frame.f_code impact (pure Python audit hook installed): 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 ################################################################################ # Testing tb.tb_frame impact (no audit hook installed) 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 # Testing tb.tb_frame impact (C audit hook installed) 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 # Testing tb.tb_frame impact (pure Python audit hook installed) 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 ################################################################################ # 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 # Testing tb.tb_frame impact on traceback.format_tb (C audit hook installed) 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 # Testing tb.tb_frame impact on traceback.format_tb (pure Python audit hook installed) 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 |
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. |
|
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 I have also attached a |
I agree with Victor, we should not be attempting to build 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". 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:
And my answers for those criteria:
---- 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[] = { 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? |
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:
I think it could make sense to:
|
Sounds good to me. We can deprecate RESTRICTED with no intention to 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. |
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. |
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. |
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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: