[pycrypto] ERROR: testRsaUnversionedSignAndVerify failed

[Hans-Peter Jansen]

Dear Jan,

I found this changelog entry in SuSEs python-crypto package:

* Thu Oct 19 2006 jmatejek at suse.cz
- minor fixes for better 64bit PEP353 compatibility

and below is a current discussion about this topic.

Am Freitag, 21. August 2009 schrieb Dwayne C. Litzenberger:
> On Thu, Aug 20, 2009 at 11:11:46AM +0200, Hans-Peter Jansen wrote:
> > openSUSE incorporated two patches to 2.0.1:
> >
> > This one, I ported to trunk:
> > --- src/hash_template.c~	2009-08-16 23:39:34.053841534 +0200
> > +++ src/hash_template.c	2009-08-20 10:16:05.877840748 +0200
> > @@ -111,13 +111,15 @@ ALG_hexdigest(ALGobject *self, PyObject
> >  	PyObject *value, *retval;
> >  	unsigned char *raw_digest, *hex_digest;
> >  	int i, j, size;
> > +	Py_ssize_t ssize;
> >
> >  	if (!PyArg_ParseTuple(args, ""))
> >  		return NULL;
> >
> >  	/* Get the raw (binary) digest value */
> >  	value = (PyObject *)hash_digest(&(self->st));
> > -	size = PyString_Size(value);
> > +	ssize = PyString_Size(value);
> > +	size = (ssize > INT_MAX) ? INT_MAX : ssize;
> >  	raw_digest = (unsigned char *) PyString_AsString(value);
> >
> >  	/* Create a new string */
> >
> > Dwayne, is this in order or just plain silly?
>
> Hmm...  What's the purpose of this patch?
>
> I'm assuming this patch is for 64-bit machines, but even with the patch
> applied, you will still have other problems if ssize > INT_MAX (or even
> INT_MAX/2).
>
> Here's the code (with that patch applied) in context:
>
>   	/* Get the raw (binary) digest value */
>   	value = (PyObject *)hash_digest(&(self->st));
> 	ssize = PyString_Size(value);
> 	size = (ssize > INT_MAX) ? INT_MAX : ssize;
> 	raw_digest = (unsigned char *) PyString_AsString(value);
>
>         /* Create a new string */
>         retval = PyString_FromStringAndSize(NULL, size * 2 );
>         hex_digest = (unsigned char *) PyString_AsString(retval);
>
>         /* Make hex version of the digest */
>         for(i=j=0; i<size; i++)
>         {
>                 char c;
>                 c = raw_digest[i] / 16; c = (c>9) ? c+'a'-10 : c + '0';
>                 hex_digest[j++] = c;
>                 c = raw_digest[i] % 16; c = (c>9) ? c+'a'-10 : c + '0';
>                 hex_digest[j++] = c;
>         }
>         Py_DECREF(value);
>         return retval;
>
> If ssize > INT_MAX, then the "size * 2" argument passed to
> PyString_FromStringAndSize() will overflow, causing one of the following
> things to happen:
>
> - If size*2 overflows to a negative number, PyString_FromStringAndSize
> will presumably return NULL, causing a null-pointer dereference in the
> subsequent code.
>
> - If size*2 overflows to a positive number, PyString_FromStringAndSize
> will allocate a smaller buffer than we expect, and the subsequent loop
> wil overflow this buffer.  Even if we fixed that, if size ended up larger
> than INT_MAX/2, we would *still* overflow the buffer, since j would wrap
> around to a negative number.
>
> None of PyCrypto's hash functions return anything close to INT_MAX bytes
> (SHA256 outputs a whopping 32 bytes), so I wonder about the rationale
> behind this patch.  Does it suppress some lint warning?

Jan, while it's hard to believe, that a hash_digest size will ever come near 
INT_MAX for ints (>= 32bit), it's going to overflow anyway 5 lines below.

How about limiting it to a sane value (say 1024)? The biggest hash digest 
size, I found is sha512() with 64 bytes. With 1024, we're raising the limit 
by factor 16. If hashes are going to get bigger then that, further 
adjustment is in order.

Dwayne?

> /me runs splint against the code
>
> No, I doubt it's that.  The C code isn't even close to being lint-clean.
>
> /my adds a TODO item...
>
> Thanks,
>  - Dwayne

Cordially,
Pete