Re: pgcrypto decrypt_iv() issue

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.

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).

But I'm bit unclear about fate of /contrib cleanup patches vs. 9.2,
so if they won't get in, it's ok to apply quick fixes to current tree,
it won't inconvinience me much.

--
marko