This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: list.sort crasher
Type: Stage:
Components: Interpreter Core Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: tim.peters Nosy List: arigo, greg_ball, gvanrossum, mwh, tim.peters
Priority: low Keywords:

Created on 2001-08-20 22:12 by greg_ball, last changed 2022-04-10 16:04 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
marshal-rexec-code.diff mwh, 2001-08-25 07:30 stop rexec-marshal producing code objects
Messages (12)
msg6099 - (view) Author: Gregory H. Ball (greg_ball) Date: 2001-08-20 22:12
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?)
msg6100 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2001-08-25 07:25
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.
msg6101 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2001-08-30 06:45
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.
msg6102 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2001-08-30 12:48
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.
msg6103 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2001-08-30 14:51
Logged In: YES 
user_id=6656

OK, done, in:

marshal.c version 1.67

Changed summary.
msg6104 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2001-09-04 18:43
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.
msg6105 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2001-12-11 23:04
Logged In: YES 
user_id=6380

Sigh. I wished I'd picked this up earlier, but I haven't.
After 2.2.
msg6106 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2002-03-03 18:27
Logged In: YES 
user_id=6656

Is there any realistic chance of anything happening on this
front in the 2.2.1 timeframe?
msg6107 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2002-03-03 22:35
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>.
msg6108 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2002-11-12 15:34
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
msg6109 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2002-11-12 21:46
Logged In: YES 
user_id=31435

Assigned to me.
msg6110 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2002-11-12 22:17
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>.
History
Date User Action Args
2022-04-10 16:04:21adminsetgithub: 35016
2001-08-20 22:12:56greg_ballcreate