[Python-bugs-list] [ python-Bugs-453523 ] list.sort crasher

noreply@sourceforge.net noreply@sourceforge.net
Tue, 12 Nov 2002 14:17:45 -0800


Bugs item #453523, was opened at 2001-08-20 18:12
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=453523&group_id=5470

Category: Python Interpreter Core
Group: None
>Status: Closed
>Resolution: Fixed
Priority: 1
Submitted By: Gregory H. Ball (greg_ball)
Assigned to: Tim Peters (tim_one)
Summary: list.sort crasher

Initial Comment:
The marshal module is on the default list of ok
builtin modules, but it can be used to crash the
interpreter.  

The new module, on the other hand, is not allowed.
To me the only obvious reason for this is that 
it provides a way to make new code objects, which can
then crash the interpreter.

But marshal also gives this ability.
Example is attached as a file.  Having imported
marshal,
I use marshal.loads() on a carefully constructed string
to get a corrupt code object.

To work out this string:

(in unrestricted mode)

def f(): pass

import marshal
badstring=marshal.dumps(f.func_code).replace('(\x01\x00\x00\x00N',
'(\x00\x00\x00\x00')

which when loaded gives a code object with co_consts =
().

Possible fixes:

Easy:  remove marshal from the list of approved
modules for restricted execution.  

Hard: Fix marshal (and perhaps new) by adding checks on
code objects before returning them.  Probably no way to
do this exhaustively.

Lateral thinking: prohibit 
exec <code object> in restricted mode?  (Currently
eval() also allows execution of code objects, so 
that would have to be changed too.)
I think this is a nice complement to the existing
features of restricted execution mode, which prevent
the attachment of a new code object to a function.


On the other hand, there's not much point fixing this
unless other methods of crashing the interpreter are
also hunted down...

>>> class C:
...     def __cmp__(self, other):
...             pop(0)
...             return 1
... 
>>> gl = [C() for i in '1'*20]
>>> pop=gl.pop
>>> gl.sort()
Segmentation fault (core dumped)


(should I submit this as a separate bug report?)


----------------------------------------------------------------------

>Comment By: Tim Peters (tim_one)
Date: 2002-11-12 17:17

Message:
Logged In: YES 
user_id=31435

Armin's patch has been applied for 2.3, so closing this as 
fixed.  Whether it's a bugfix candidate is debatable, since it 
can also change behavior of "working" code.  I changed the 
docs too so that it can longer be considered working code 
<wink>.

----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2002-11-12 16:46

Message:
Logged In: YES 
user_id=31435

Assigned to me.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2002-11-12 10:34

Message:
Logged In: YES 
user_id=4771

The list.sort() problem could be quickly solved by stealing
the ob_item array away from the list object itself at the
beginning of the sort.  The list object would appear to be
empty during the sort.  The ob_item array would be put back
into place at the end, with a check that the list is still
empty.  A possible drawback is that len() of a list being
sorted is 0, althought one might argue that this should
return the correct length.

The immutable list trick could be removed -- or kept around
for the error messages it provides, althought I'd vote
against it: Python and Python programmers generally assume
that the type is an immutable property of an object.

See patch http://www.python.org/sf/637176


----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2002-03-03 17:35

Message:
Logged In: YES 
user_id=31435

Assuming "this front" means the list.sort() crasher, not 
unless Guido raises the priority from 1 to, oh, at least 8 
<wink>.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-03-03 13:27

Message:
Logged In: YES 
user_id=6656

Is there any realistic chance of anything happening on this
front in the 2.2.1 timeframe?

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2001-12-11 18:04

Message:
Logged In: YES 
user_id=6380

Sigh. I wished I'd picked this up earlier, but I haven't.
After 2.2.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2001-09-04 14:43

Message:
Logged In: YES 
user_id=6380

I'm not so interested in the list.sort crasher, so I'm
lowering the priority and unassigning it.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2001-08-30 10:51

Message:
Logged In: YES 
user_id=6656

OK, done, in:

marshal.c version 1.67

Changed summary.


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2001-08-30 08:48

Message:
Logged In: YES 
user_id=6380

Michael's patch is fine -- the code-loading is not done
in restricted mode (but the execution is, because the
restricted globals are passed).  Michael, can you check
it in?

The list issue could be fixed by adding a PyList_Check()
call to each list method implementation (or at least to
the mutating ones).

But is it sufficient?  I believe there are plenty of other
ways to cause a crash -- stack overflow is one, and I
think marshal itself can also crash on corrupt input.

Greg's bug report points out that rexec is far from
sufficient to deter a real hacker.  Imagine that this
was used in a popular email program... :-(

If we can't deprecate rexec, perhaps we should add
very big warnings to the documentation that it can't
be trusted against truly hostile users.

----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2001-08-30 02:45

Message:
Logged In: YES 
user_id=31435

Reassigning to Guido.  The patch stops marshal from loading 
a code object when in restricted mode, but I have no feel 
for whether that's going to be a serious limitation for 
restricted mode (which I've never used for real).  For 
example, wouldn't this also stop regular old imports 
from .pyc files?  I'd hate for restricted users to be 
barred from importing tabnanny <wink>.

The list-crasher is a different issue, but I went over *my* 
limit for caring about trying to foil hostile users when we 
added the immutable list type.  That trick doesn't help 
here (the mutating mutable-list method is captured in a 
bound method object before the sort is invoked).

I suppose we could instead check that the list base address 
and length haven't changed after every compare -- but with 
N*log(N) compares, that's a significant expense the 
immutable list trick was trying to get away from.  Not 
worth it to me, but my native interest in competing with 
hostile users is admittedly undetectable.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2001-08-25 03:25

Message:
Logged In: YES 
user_id=6656

I think a reasonable approach to the first problem is to not 
let marshal load code objects when in restricted execution 
mode.

The second crasher you mention is much more worrying, to me.  
I think it blows the "immutable list trick" out of the water.

I'll attach a patch to marshal (from another machine) and 
assign to Tim to think about the list nagery.


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=453523&group_id=5470