| 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 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)  |
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) * |
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 . 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) * |
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) |
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) * |
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) * |
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) |
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) * |
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 . |
|
|
| msg6108 - (view) |
Author: Armin Rigo (arigo) * |
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) * |
Date: 2002-11-12 21:46 |
| Logged In: YES user_id=31435 Assigned to me. |
|
|
| msg6110 - (view) |
Author: Tim Peters (tim.peters) * |
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 . |
|
|