Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(522)

Issue 20051: Minimal patch to secure the Python interpreter

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years ago by asktav
Modified:
15 years 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
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
GvR
I don't have any problems with these additions. I can see that they are necessary ...
15 years ago (2009-02-23 18:02:49 UTC) #1
Martin v. Löwis
Can you come up with a piece of documentation that documents the effects of PyEval_GetRestricted ...
15 years ago (2009-02-23 22:57:36 UTC) #2
asktav
> Can you come up with a piece of documentation that documents the effects of ...
15 years ago (2009-02-23 23:43:30 UTC) #3
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 ...
15 years ago (2009-02-25 16:13:26 UTC) #4
GvR
Some comments for discussion. Note that I am not judging this as sufficient to secure ...
15 years ago (2009-02-25 17:42:40 UTC) #5
asktav
Hey Guido, > Note that I am not judging this as sufficient to secure Python ...
15 years ago (2009-02-25 18:01:36 UTC) #6
asktav
15 years 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b