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 <ste...@kaltenbrunner.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 -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
