list.sort crasher

[gregball]

BPO	453523
Nosy	@mwhudson, @gvanrossum, @tim-one, @arigo
Files	marshal-rexec-code.diff: stop rexec-marshal producing code objects

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tim-one'
closed_at = <Date 2002-11-12.22:17:45.000>
created_at = <Date 2001-08-20.22:12:56.000>
labels = ['interpreter-core']
title = 'list.sort crasher'
updated_at = <Date 2002-11-12.22:17:45.000>
user = 'https://bugs.python.org/gregball'

bugs.python.org fields:

activity = <Date 2002-11-12.22:17:45.000>
actor = 'tim.peters'
assignee = 'tim.peters'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2001-08-20.22:12:56.000>
creator = 'greg_ball'
dependencies = []
files = ['124']
hgrepos = []
issue_num = 453523
keywords = []
message_count = 12.0
messages = ['6099', '6100', '6101', '6102', '6103', '6104', '6105', '6106', '6107', '6108', '6109', '6110']
nosy_count = 5.0
nosy_names = ['mwh', 'gvanrossum', 'tim.peters', 'arigo', 'greg_ball']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue453523'
versions = []

[gregball]

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?)

[mwhudson]

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.

[tim-one]

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.

[gvanrossum]

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.

[mwhudson]

Logged In: YES
user_id=6656

OK, done, in:

marshal.c version 1.67

Changed summary.

[gvanrossum]

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.

[gvanrossum]

Logged In: YES
user_id=6380

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

[mwhudson]

Logged In: YES
user_id=6656

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

[tim-one]

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

[arigo]

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

[tim-one]

Logged In: YES
user_id=31435

Assigned to me.

[tim-one]

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