Re: pgcrypto decrypt_iv() issue

On 01/27/2012 04:20 PM, Marko Kreen wrote:
> On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote:
>> Stefan Kaltenbrunner <stefan(at)kaltenbrunner(dot)cc> writes:
>>> from some looking at the code in pgcrypto.c it seems to me that the
>>> coding pattern in most functions there only checks for errors from the
>>> corresponding initialization function, in the case of say decrypt_iv()
>>> that means only the IV and the key are actually "validated" because that
>>> is what the init function sees(it never sees that data!), if the actual
>>> decrypt call fails (because the data is maybe a bit weird^broken) it
>>> will happily ignore that and return random data.
>>
>> Yeah. In pg_decrypt() we have
>>
>> err = px_combo_init(c, (uint8 *) VARDATA(key), klen, NULL, 0);
>> if (!err)
>> err = px_combo_decrypt(c, (uint8 *) VARDATA(data), dlen,
>> (uint8 *) VARDATA(res), &rlen);
>>
>> but in pg_decrypt_iv() it's just
>>
>> err = px_combo_init(c, (uint8 *) VARDATA(key), klen,
>> (uint8 *) VARDATA(iv), ivlen);
>> if (!err)
>> px_combo_decrypt(c, (uint8 *) VARDATA(data), dlen,
>> (uint8 *) VARDATA(res), &rlen);
>>
>> It looks to me like the result of px_combo_decrypt should be assigned to
>> "err" here. If I make that change, the test case you provide is
>> rejected:
>>
>> ERROR: decrypt_iv error: Data not a multiple of block size
>>
>> but the module's regression tests all still pass, indicating that this
>> sort of case isn't tested.
>>
>> pg_encrypt_iv() has the identical usage error with respect to
>> px_combo_encrypt.
>>
>> Marko, does this look right to you?
>
> Yeah, it should be fixed. But note that "random data" is part of
> decrypt() spec - the validation it can do is a joke.
>
> Its more important to do proper checks in encrypt() to avoid invalid
> stored data, but there the recommended modes (CBC, CFB) can work
> with any length data, so even there the impact is low.

I agree - but in my case the input to those functions is actually coming
from external untrusted systems - so if the data is (completely) invalid
really want to get a proper error message instead of random memory content.

>
> pgcrypto.c is easily fixable and internal.c has proper checks.
> But openssl.c does not. And I have a bigger openssl.c cleanup
> pending. So I would prefer to add missing checks to cleaned-up
> openssl.c and post them together (soonish).

hmm so openssl.c has similiar "issues" but you only want to fix them
together with a cleaned larger patch? sounds a bit of a problem for a
backpatch...

Stefan