[ python-Bugs-1153075 ] PyXxx_Check(x) trusts x->ob_type->tp_mro

Mon Mar 14 12:25:06 CET 2005

Bugs item #1153075, was opened at 2005-02-27 21:55
Message generated for change (Comment added) made by arigo
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1153075&group_id=5470

Category: Python Interpreter Core
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Armin Rigo (arigo)
Assigned to: Armin Rigo (arigo)
Summary: PyXxx_Check(x) trusts x->ob_type->tp_mro

Initial Comment:
The functions PyInt_Check(), PyString_Check(),
PyList_Check() etc. are used all over the core to check
which typecasts are safe, from PyObject* to the various
PyXxxObject*.

But the macros themselves are implemented by
inspecting the "tp_mro" tuple of the incoming object's
type.  As the latter can be completely controlled by the
user, an object can pretend to inherit from anything and
pass the PyXxx_Check() checks of its choice, even if
its memory layout is actually completely wrong.

See attached example.

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

>Comment By: Armin Rigo (arigo)
Date: 2005-03-14 11:25

Message:
Logged In: YES 
user_id=4771

I tried to convince myself that this check always accepts the 
default mro, but there are more implicit assumptions around 
than I can handle in a quick review...

Instead, I modified the patch so that a debug build of Python 
always does the check in mro_internal().  This results in a 
shallow problem: the basestring type has tp_basicsize==0, 
which makes the computation of its solid_base trigger an 
assertion in extra_ivars().  If I hack around this problem, then 
I cannot quickly find an example of built-in mro that triggers a 
TypeError, but it doesn't mean there isn't one...

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

Comment By: Michael Hudson (mwh)
Date: 2005-03-06 18:47

Message:
Logged In: YES 
user_id=6656

I think the attached fixes all this.

What it does: check the return value of PySequence_Tuple (duh).  Check 
that the returned sequence contains only types or old-style classes.  
Checks that the solid_base of each type in the returned sequence is a 
supertype of the solid_base of type being built.  Adds a few tests.

The error messages are a bit inscrutable.  I find it hard to care.

Assigning to Armin for the first look.  Might want to get Guido to look at it 
too.

When Armin and I talked about this on IRC we found a few oddities in the 
general area, but I don't know if it's worth opening a new bug report for 
them...

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

Comment By: Michael Hudson (mwh)
Date: 2005-03-03 09:50

Message:
Logged In: YES 
user_id=6656

Certainly some more checking of what the user-defined MRO contains 
would be good -- check the attached, which dumps core at class definition 
time...

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

Comment By: Armin Rigo (arigo)
Date: 2005-03-03 09:34

Message:
Logged In: YES 
user_id=4771

The PyXxx_Check() macros are #defined with
PyObject_TypeCheck().  Should we change all these #defines to
use a new PyObject_LayoutCheck() or something, and document
to C extension writers that they should also switch to the
new function?  More generally, should we review *all* usages
of PyObject_TypeCheck() and decide if they really meant that
or PyObject_LayoutCheck()?  Do we care about types that are
solid subclasses of 'int' but don't have it in their MRO? 
Should we just forget the whole idea and sanity-check the
user-defined mro, which after all would just add another
strange undocumented condition to typeobject.c? :-)

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

Comment By: Michael Hudson (mwh)
Date: 2005-03-02 20:11

Message:
Logged In: YES 
user_id=6656

Hmm, yeah, that works.  It wasn't totally clear to me that the user couldn't 
maliciously influence tp_base, but I don't think they can...

A helper function is presumably in order, unless PyType_IsSubtype should 
be changed.

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

Comment By: Armin Rigo (arigo)
Date: 2005-03-02 17:03

Message:
Logged In: YES 
user_id=4771

To solve the problem, as hinted in the title of this tracker, I 
think that PyXxx_Check() should simply not trust any mro.  
What PyInt_Check() really means at the C level is to check if 
an object memory layout is compatible with PyIntObject.  This 
is easy to figure out by walking the "solid base" chain, 
tp_base.

As a side note, PyPy gives the same error as Python 2.2.  
However, both in PyPy and in Python 2.2, you can still build 
an instance of the strange class X as follows:

>>> x = object.__new__(X)

Still, all the methods of x are resolved via the dict type.  In 
PyPy we get a clean TypeError because the methods thus 
found don't apply to non-dict objects.  In Python 2.2 you can 
crash the interpreter just as in more recent Python releases, 
e.g. with x[5]=6.

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

Comment By: Michael Hudson (mwh)
Date: 2005-03-01 20:18

Message:
Logged In: YES 
user_id=6656

I wonder if Guido owes Armin a beer (read typeobject.c around line 5230).  
Well, not really, but it's that code that is making the difference.

However, reversing the order of dict and object is sufficient to make 2.2 
crash (I presume, it crashes CVS HEAD with the __new__ short-circuiting 
code chopped out which gives the error jelper observes on the original).

I wonder what to do about this.  Removing the ability to customize the mro 
from user code is one obvious approach -- I don't know how much code 
uses this feature though.

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

Comment By: Jeff Epler (jepler)
Date: 2005-03-01 19:15

Message:
Logged In: YES 
user_id=2772

Not sure if this is relevant, but the example given didn't
crash 2.2:
$ python2.2 bug.py
Traceback (most recent call last):
  File "bug.py", line 9, in ?
    x = X()
TypeError: dict.__new__(X) is not safe, use object.__new__()

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

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