The PyString_FromStringAndSize() function takes a pointer and signed
integer as input parameters however it fails to adequately check the
sanity of the integer argument. Because of the failure to check for
negative values and because it sums the integer with the size of the
PyStringObject structure it becomes possible for the allocator to take
either of the code paths in PyObject_MALLOC()-- both of which will
incorrectly allocate memory.

This may not seem like a big deal, but I'm posting this instead of
filing a bug for every place this screws you guys over.

if (0 > len || len > PYSSIZE_T_MAX/sizeof(PyStringObject)) 
        return NULL;

This is done already:
the second line in PyString_FromStringAndSize() is
    assert(size>=0);
You have to build python in debug mode though...

Oh, I realize this is not a real patch: no error is raised, and why
PYSSIZE_T_MAX/sizeof(PyStringObject), when the allocation is    
PyObject_MALLOC(sizeof(PyStringObject)+size)?

The problem with assert()'s is they require debugging to be enabled,
obviously, who compiles it that way?

You may not even want to worry about the second check, when you pass it
into the allocator it gets converted to an unsigned int which will cause
the allocator to either fail (32-bit) or allocate more memory than
expected-- either cause it handled/benign.

If you'd prefer I can file every place where this actually bites you
guys, I was just trying to be nice.

Adding a poc from 2586 to demonstrate my point, this causes a call to
the allocator requesting zero bytes.

As an addemdum, consider the following code (theres no assert, but it
wouldnt have helped you outside of debug builds anyways):


488 static PyObject *PySSL_SSLread(PySSLObject *self, PyObject *args)
489 {
490         PyObject *buf;
491         int count = 0;
492         int len = 1024;
[...]
496         if (!PyArg_ParseTuple(args, "|i:read", &len))
497                 return NULL;
498 
499         if (!(buf = PyString_FromStringAndSize((char *) 0, len)))
500                 return NULL;
[...]
521                 count = SSL_read(self->ssl, PyString_AsString(buf),
len);

I agree that PySSL_SSLread should check that its argument is >= 0.  I 
don't think this check belongs to PyString_FromStringAndSize.   It 
should be the responsibility of the caller to verify that the 
precondition size >= 0 is satisfied before calling PyString_FromStringAndSize.  Oftentimes the caller can ascertain size >= 
0 without an explicit check, for example, if size is the size of a 
buffer or length of a valid string object.

On the other hand, an external input such as the len argument to the 
read function should be checked before used.

I would also suggest changing the len type from int to Py_ssize_t.

I agree with Amaury that assert(size>=0) is sufficient in PyString_FromStringAndSize(), but its documentation should emphasize 
that the caller is responsible for assuring that the requested length is  
nonnegative.

I don't think the upper bound check is necessary: sizeof(PyStringObject) 
+ size will not wrap around as long as size >= 0.

While I think its a mistake that will cause security concerns in Python
for quite some time to come, thats fair enough.

I will refile all the places that this messes up individually

Do I need to create proof of concepts for each of these bugs, or can I
reference this ticket?

On Wed, Apr 9, 2008 at 1:20 PM, Justin Ferguson <report@bugs.python.org> wrote:
..
>  Do I need to create proof of concepts for each of these bugs, or can I
>  reference this ticket?
>

It would be best if you could provide a patch complete with additional
unit tests that fail in (or crash) unpatched python and demonstrate
the bugs.  Since the fixes are likely to be one-line changes, I would
say there is no need to create separate issues.  Just post a patch
here for а review.

From your other post, I understand that you are doing a security audit
of the python codebase.  Is this a manual effort (identifying unsafe
constructs and searching for them) or you use some kind of automation?

agreed, the assert in PyString_FromStringAndSize() should be changed to
a non-debug test.

Okay, so I'm not sure whose point of view takes precedence here?

Also, to answer the question asked, I'm doing this manually.

On Wed, Apr 9, 2008 at 3:08 PM, Justin Ferguson <report@bugs.python.org> wrote:

>  Okay, so I'm not sure whose point of view takes precedence here?

I don't have a strong view on this, but just a few points to consider:

1. If you change PyString_FromStringAndSize, you should change
PyBytes_FromStringAndSize and PyUnicode_FromStringAndSize for
consistency.

2. Non-debug check should probably set a ValueError exception and
return NULL.  Think what kind of message you can generate inside
*_FromStringAndSize that would be helpful to the end user.  The caller
is likely to be able to provide a much better diagnostic.

3. Maybe instead of setting a ValueError, a SystemError should be
raised.  In this case the calle will still be clearly responsible for
the pre-condition, but programming errors will not present security
concerns.

4. You have clearly identified several bugs that are not covered by
unittests.  If you fix them en-mass by making
PyString_FromStringAndSize more forgiving, we will miss an opportunity
to improve the test coverage. (If you take the SystemError approach,
this does not apply.)

> 1. If you change PyString_FromStringAndSize, you should change
> PyBytes_FromStringAndSize and PyUnicode_FromStringAndSize for
> consistency.

Okay, if we don't patch the API call itself, I will look at the users of
those two API calls as well, I haven't gone through much of the stuff in
Objects/ yet, I focused on the modules obviously and thus didn't know
about those calls.

> 4. You have clearly identified several bugs that are not covered by
> unittests.  If you fix them en-mass by making
> PyString_FromStringAndSize more forgiving, we will miss an opportunity
> to improve the test coverage. (If you take the SystemError approach,
> this does not apply.)

This is a valid and good point-- I don't mind either way

I think I like the SystemError approach for the same reasons.  It still
exposes the caller's bug but no longer does bad things.

Cool. Let's make this a SystemError (in both debug builds and non-debug
build). Greg, go ahead and fix it.

python trunk r62261 adds the checks and SystemError.

patches to cleanup modules that inadvertently allow negative values to
be passed into *_FromStringAndSize would still be appreciated.

release25-maint r62262

I'm closing this one.  Please open additional issues with patches and/or
pointers to bad callers of *_FromStringAndSize that let negative values
through.

[*sigh* i wish we didn't use a signed size type all over the place]

>  [*sigh* i wish we didn't use a signed size type all over the place]

What would you use for error returns then?

On Wed, Apr 9, 2008 at 8:18 PM, Guido van Rossum <report@bugs.python.org> wrote:

> >  [*sigh* i wish we didn't use a signed size type all over the place]
>
>  What would you use for error returns then?
>

res == (size_t)-1  or res == 0 && PyErr_Occured()

The use of signed integers in Python is (imho) the single largest threat
to the security of the interpreter. I'm probably preaching to the choir
there though. 

I really dislike have to return values and indicate error in the return
value, its really unclean and causes weirdness, for instance if you
follow PyArg_ParseTuple() down and you have an integer (or maybe it was
long?) argument, you can't actually get a value of -1 because one of the
string->int conversion routines uses that to indicate failure. The check
wrapped around it was something along the lines of:

if (-1 == retval && PyErr_Occurred())

In turn down the line somewhere (I didn't follow the code path), this
value got converted to 1, so for instance doing
__import__('zlib').decompressobj().flush(-1) wouldn't trigger that bug,
it would flush 1 byte, not UINT_MAX as I had expected

Greg, any reason why you kept the assert()?  IMO having the ValueError
even in debug mode is better.

The asserts can go.  I left them in figuring a crashing interpreter on a
debug build in someones sandbox or on a buildbox would get more
attention than a SystemError being raised.  I doubt that is a worthy
assumption on my part.

Both a crash and a SystemError are notable events.

shall I get rid of the asserts?

As for why i dislike signed size types... tons of reasons:
 * It wastes half the range of the integer.
 * It leads to security bugs.
 * on return values -1 and < 0 tests may be convenient to type but they 
   could just as easily compare to a known value defined as a constant; 
   all the things alexander belopolsky suggested.
 * sizes being passed -in- to a function never need to be negative
   meaning safe code requires extra checks like these.
 * sign extension of values going between registers of different sizes
   is needlessly expensive on some cpu architectures.  use unsigned
   types whenever possible for the best code.

anyways, I figure the Python C API is already set in stone using the
signed types so its too late to "fix" it without causing headaches
around the world.

Please do get rid of the asserts.

Regarding signed sizes, it's a historic accident (copied from Unix
system calls which tended to use plain ints or longs in the olden days),
but it may not be impossible to fix -- we've fixed other things
throughout the code (e.g. switching from int to Py_ssize_t).  It'll be a
monumental task though that's probably best tackled post-3.0.

asserts nuked.  r62271 and r62272

On Thu, Apr 10, 2008 at 3:48 PM, Guido van Rossum
<report@bugs.python.org> wrote:

Maybe as the first step we could get rid of the size sign abuse in
long integer objects.  I would suggest reserving a bit in the first
(or last) digit to carry the sign.

the redundant asserts were removed.  reclosing.

further discussion on signedness issues in the code base should take
place on the mailing list, future bugs or peps as appropriate.

Justin, is there any reproducer available for this issue?
Thanks!

Hello guys,

  if I didn't overlook something pretty obvious, this should work
with python-2.6, but it crashes.

Could you please have a look?

Thanks, Jan.
--
Jan iankko Lieskovsky