Minimal patch to secure the Python interpreter

[GvR, 2009-02-23 18:02:49]

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.

[Martin v. Löwis, 2009-02-23 22:57:36]

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.

[asktav, 2009-02-23 23:43:30]

> 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

[asktav, 2009-02-25 16:13:26]

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

[GvR, 2009-02-25 17:42:40]

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. :-)

[asktav, 2009-02-25 18:01:36]

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

[asktav, 2009-02-25 21:00:56]

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