Minimal patch to secure the Python interpreter - Code Review (original) (raw)

| Can't Edit Can't Publish+Mail Start Review Created: 16 years, 2 months ago by asktav Modified: 16 years, 2 months ago Reviewers: GvR, Martin v. Löwis CC: tav Visibility: Public. | Patch Set 1# Total comments: 1 Patch Set 2 : Post-challenge version of the secure python patch.# Total comments: 12 Created: 16 years, 2 months ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -8 lines) Patch M Objects/codeobject.c View 1 chunk +6 lines, -0 lines 0 comments Download M Objects/frameobject.c View 7 chunks +33 lines, -5 lines 8 comments Download M Objects/genobject.c View 1 1 chunk +5 lines, -3 lines 4 comments Download M Objects/typeobject.c View 1 1 chunk +6 lines, -0 lines 0 comments Download Messages Total messages: 7 Expand All Messages | Collapse All Messages GvR I don't have any problems with these additions. I can see that they are necessary ... 16 years, 2 months ago (2009-02-23 18:02:49 UTC)#1 I don't have any problems with these additions. I can see that they are necessary to have any chance of creating a secure sandbox. While I have no particularly strong indication that these are the *only* necessary changes, I am willing to use these as a starting point for a challenge on app engine. Once these are accepted for core Python I am willing to get these into the App Engine runtime, and then Tav can run a contest. Sign in to reply to this message. Martin v. Löwis Can you come up with a piece of documentation that documents the effects of PyEval_GetRestricted ... 16 years, 2 months ago (2009-02-23 22:57:36 UTC)#2 Can you come up with a piece of documentation that documents the effects of PyEval_GetRestricted (i.e. what attributes get restricted how, in a tabular form, plus what additional restrictions get applied)? From scanning through the code, it seems that its mostly attributes only. I think that could go into Doc/c-api/reflection.rst (it should also explain builtins test) http://codereview.appspot.com/20051/diff/1/3 File Objects/typeobject.c (right): http://codereview.appspot.com/20051/diff/1/3#newcode2251 Line 2251: The only issue I see here is that this is not C89 (i.e. mixes code and declarations); that's easily corrected, of course. Sign in to reply to this message. asktav > Can you come up with a piece of documentation that documents the effects of ... 16 years, 2 months ago (2009-02-23 23:43:30 UTC)#3 > Can you come up with a piece of documentation that documents the effects of > PyEval_GetRestricted (i.e. what attributes get restricted how, in a tabular > form, plus what additional restrictions get applied)? From scanning through the > code, it seems that its mostly attributes only. Sure. I agree that documentation would really help more people understand what's actually going on. And will hopefully clear the way down the line for clarity on which restrictions are actually needed. Question though: If I slave away at the documentation, will you please accept the patch? ;p -- Cheers, tav Sign in to reply to this message. asktav Hey Martin/Guido, I've written some documentation as asked: * http://tav.espians.com/paving-the-way-to-securing-the-python-interpreter.html I've also updated the patch ... 16 years, 2 months ago (2009-02-25 16:13:26 UTC)#4 Hey Martin/Guido, I've written some documentation as asked: * http://tav.espians.com/paving-the-way-to-securing-the-python-interpreter.html I've also updated the patch to incorporate some lessons from the security challenge. Let me know what else I need to do. -- Cheers, tav Sign in to reply to this message. GvR Some comments for discussion. Note that I am not judging this as sufficient to secure ... 16 years, 2 months ago (2009-02-25 17:42:40 UTC)#5 Some comments for discussion. Note that I am not judging this as sufficient to secure Python -- I think your challenge has shown that's not a black-and-white story. But most of the added restrictions seem harmless to me. http://codereview.appspot.com/20051/diff/6/1007 File Objects/frameobject.c (right): http://codereview.appspot.com/20051/diff/6/1007#newcode18 Line 18: {"f_back", T_OBJECT, OFF(f_back), RO|RESTRICTED}, Why make f_back restricted? Since all frame attributes are restricted why not allow access to the whole chain of frames? http://codereview.appspot.com/20051/diff/6/1007#newcode19 Line 19: {"f_code", T_OBJECT, OFF(f_code), RO|RESTRICTED}, I'm still unclear why you'd want to hide access to the code object -- especially since it has useful info like the filename. http://codereview.appspot.com/20051/diff/6/1007#newcode22 Line 22: {"f_lasti", T_INT, OFF(f_lasti), RO|RESTRICTED}, Technically this isn't needed. http://codereview.appspot.com/20051/diff/6/1007#newcode85 Line 85: return NULL; The line number would be useful, and I don't see how it could be a secret. http://codereview.appspot.com/20051/diff/6/1006 File Objects/genobject.c (right): http://codereview.appspot.com/20051/diff/6/1006#newcode316 Line 316: RO|RESTRICTED}, Given the restrictions on frames, do you still need this? http://codereview.appspot.com/20051/diff/6/1006#newcode319 Line 319: RO|RESTRICTED}, Again, why do you restrict the code object? (I guess I am arguing for lifting the restriction on func_code/__code__ too. :-) Sign in to reply to this message. asktav Hey Guido, > Note that I am not judging this as sufficient to secure Python ... 16 years, 2 months ago (2009-02-25 18:01:36 UTC)#6 Hey Guido, > Note that I am not judging this as sufficient to secure Python Sure. I'm simply pitching this as a bug fix for the current set of restrictions in Python. (Thanks Martin! =) > I think your challenge has shown that's not a black-and-white > story. It was fun though! ;p > But most of the added restrictions seem harmless to me. Thanks. As to the various restrictions. I just made frame objects totally opaque -- as is currently the case with function objects. This seemed to be the simplest and safest approach forward. At a later point, I'd like to actually go through and simplify the list and drop most of the restrictions -- including most of the existing ones. -- Cheers, tav Sign in to reply to this message. asktav Hey Martin, I've responded to Guido's comments. The reason I restricted all the aspects of ... 16 years, 2 months ago (2009-02-25 21:00:56 UTC)#7 Hey Martin, I've responded to Guido's comments. The reason I restricted all the aspects of the frame object is to make it similarly opaque as function objects are in the current code base. I'll remove the needless restrictions on f_back, f_lasti and lineno. Martin, in the last 24 hours, there have been 1,293 unique downloads of safelite.py and there have been no reporting of exploits in that period. So unless someone is working away secretly and plans to surprise us all, I doubt that we will learn anything new on this front for the forseeable future. I don't think anyone besides me is interested. And the only thing that came out of the security challenge -- besides people's ingenuity -- is that traceback objects are accessible outside of sys.exc_info/last_traceback. And thus the restriction on frame objects. There are only a finite number of builtin objects and thus attributes in the Python interpreter. I will now spend some time and go through each of them again -- whether they are directly accessible or not -- and see if they leak global state. I had previously ignored objects that I had thought were not directly accessible, like traceback/frame objects. This should ensure a pretty solid approach. Would that be good enough for you? -- Cheers, tav http://codereview.appspot.com/20051/diff/6/1007 File Objects/frameobject.c (right): http://codereview.appspot.com/20051/diff/6/1007#newcode18 Line 18: {"f_back", T_OBJECT, OFF(f_back), RO|RESTRICTED}, Sure. http://codereview.appspot.com/20051/diff/6/1007#newcode19 Line 19: {"f_code", T_OBJECT, OFF(f_code), RO|RESTRICTED}, See the comment on genobject.c http://codereview.appspot.com/20051/diff/6/1007#newcode22 Line 22: {"f_lasti", T_INT, OFF(f_lasti), RO|RESTRICTED}, Sure. http://codereview.appspot.com/20051/diff/6/1007#newcode85 Line 85: return NULL; Sure. http://codereview.appspot.com/20051/diff/6/1006 File Objects/genobject.c (right): http://codereview.appspot.com/20051/diff/6/1006#newcode316 Line 316: RO|RESTRICTED}, Good point. But I'd like to keep this restriction in there, just in case... http://codereview.appspot.com/20051/diff/6/1006#newcode319 Line 319: RO|RESTRICTED}, code_object.co_consts Sign in to reply to this message. Expand All Messages | Collapse All Messages | | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- |

Issue 20051: Minimal patch to secure the Python interpreter
Created 16 years, 2 months ago by asktav
Modified 16 years, 2 months ago
Reviewers: GvR, Martin v. Löwis
Base URL:
Comments: 13