[pycrypto] ERROR: testRsaUnversionedSignAndVerify failed

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

/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

-- 
Dwayne C. Litzenberger <dlitz at dlitz.net>
 Key-signing key   - 19E1 1FE8 B3CF F273 ED17  4A24 928C EC13 39C2 5CF7