Thread: pgcrypto decrypt_iv() issue
We are using the raw decryption features of contrib/pgcrypto here to
decode certain AES128 encrypted data. However depending on the data to
decode and what statements have been executed in the same session before
we are seeing different (and sometimes completely wrong) answers
this is 9.1.2 on Debian/amd64 compiled with -enable-debug (note that the
the first and the third query do not result in the same answer):
mastermind@mastermind:~/playground$ ./pginst/bin/psql -p 5435 postgres
psql (9.1.2)
Type "help" for help.
postgres=# select
encode(decrypt_iv(decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66ff','hex')::bytea,
decode('636F6E73756D65723267726964303333','hex')::bytea,
decode('2D4C526240141e029696969696969696','hex')::bytea,
'aes-cbc/pad:none')::bytea, 'hex');
encode
----------------------------------------------------------------------------------------------------
00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
(1 row)
postgres=# select
encode(decrypt_iv(decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66','hex')::bytea,
decode('636F6E73756D65723267726964303333','hex')::bytea,
decode('2D4C526240141e029696969696969696','hex')::bytea,
'aes-cbc/pad:none')::bytea, 'hex');
encode
--------------------------------------------------------------------------------------------------
2f2f066d198694b1030b040369540d0004833c00000000042b3300000004ab3c000000002f2f2f2f2f2f2f2f2f2f2f2f
(1 row)
postgres=# select
encode(decrypt_iv(decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66ff','hex')::bytea,
decode('636F6E73756D65723267726964303333','hex')::bytea,
decode('2D4C526240141e029696969696969696','hex')::bytea,
'aes-cbc/pad:none')::bytea, 'hex');
encode
----------------------------------------------------------------------------------------------------
2f2f066d198694b1030b040369540d0004833c00000000042b3300000004ab3c000000002f2f2f2f2f2f2f2f2f2f2f2f00
(1 row)
on some other 9.1.1 installation(standard debian package) we get:
test=# select
encode(decrypt_iv(decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66ff','hex')::bytea,
decode('636F6E73756D65723267726964303333','hex')::bytea,
decode('2D4C526240141e029696969696969696','hex')::bytea,
'aes-cbc/pad:none')::bytea, 'hex'); encode
----------------------------------------------------------------------------------------------------
00000000230000002a0a000000c69327907f00000000000000000000000000000000000000000000000000000000000000
(1 row)
test=# select
encode(decrypt_iv(decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66ff','hex')::bytea,
decode('636F6E73756D65723267726964303333','hex')::bytea,
decode('2D4C526240141e029696969696969696','hex')::bytea,
'aes-cbc/pad:none')::bytea, 'hex'); encode
----------------------------------------------------------------------------------------------------
19000000ffffffff64000000000000000300000003000000ffffffff000000000000000000000000000000000000000000
(1 row)
test=# select
encode(decrypt_iv(decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66ff','hex')::bytea,
decode('636F6E73756D65723267726964303333','hex')::bytea,
decode('2D4C526240141e029696969696969696','hex')::bytea,
'aes-cbc/pad:none')::bytea, 'hex'); encode
----------------------------------------------------------------------------------------------------
907f0000000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000
(1 row)
on 8.3 we even managed to produce(slightly different version of the
query) the following once:
select decrypt_iv(data, key, iv, 'aes-cbc/pad:none') from (select
decode('c89a929fa8dbefaa88609ea3b637d783ce926b8a0985ae02a100c0f89d8a8e5aca793d1519f5829a61b9fbd1d582fb66ff','hex')::bytea
as data, decode('636F6E73756D65723267726964303333','hex')::bytea as key,
decode('2D4C526240141e029696969696969696','hex')::bytea as iv) as c;
decrypt_iv
---------------------------------------------------------------------------------------------
/lib/postgresql/8.3/lib/pgcrypto.so\000\012\000\000\000\200\000\000\000\000\000\000\000\000
(1 row)
which seems even more of a "wtf"?!
Stefan
On 01/23/2012 08:40 PM, Stefan Kaltenbrunner wrote: > We are using the raw decryption features of contrib/pgcrypto here to > decode certain AES128 encrypted data. However depending on the data to > decode and what statements have been executed in the same session before > we are seeing different (and sometimes completely wrong) answers > > this is 9.1.2 on Debian/amd64 compiled with -enable-debug (note that the > the first and the third query do not result in the same answer): > > [examples snipped] > > which seems even more of a "wtf"?! 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. Stefan
Stefan Kaltenbrunner <stefan@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?
regards, tom lane
On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote: > Stefan Kaltenbrunner <stefan@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. 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
Marko Kreen <markokr@gmail.com> writes:
> 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.
I think we should fix and back-patch these two specific bugs. The
openssl.c change sounds like it might be something for HEAD only.
regards, tom lane
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@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
On Fri, Jan 27, 2012 at 12:13:21PM -0500, Tom Lane wrote: > Marko Kreen <markokr@gmail.com> writes: > > 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. > > I think we should fix and back-patch these two specific bugs. The > openssl.c change sounds like it might be something for HEAD only. Now I looked more in-depth and seems my comments were off - error detection for encrypt()/decrypt() happens in px.c not in internal.c/openssl.c. Latter ones simply validate internal APIs. So attached patch should be enough to fix the issue. And it should be quite backportable. -- marko
Attachment
On Fri, Jan 27, 2012 at 7:34 PM, Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> wrote: > On 01/27/2012 04:20 PM, Marko Kreen wrote: >> On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote: >> Yeah, it should be fixed. =A0But 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 conten= t. You *will* get random memory content. If your app is exploitable with invalid data, you *will* get exploited. The decrypt() checks are more for developer convenience than anything more serious. Please fix your app to survive invalid data... --=20 marko
On Fri, Jan 27, 2012 at 18:54, Marko Kreen <markokr@gmail.com> wrote: > On Fri, Jan 27, 2012 at 7:34 PM, Stefan Kaltenbrunner > <stefan@kaltenbrunner.cc> wrote: >> On 01/27/2012 04:20 PM, Marko Kreen wrote: >>> On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote: >>> Yeah, it should be fixed. =A0But 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 conte= nt. > > You *will* get random memory content. =A0If your app is exploitable with > invalid data, you *will* get exploited. =A0The decrypt() checks are > more for developer convenience than anything more serious. Hold on. I hope there's some misunderstanding here. I hope you are you saying that feeding random data to the decrypt functions should be expected to return random data out of previously free()d areas? Surely you're not? Obviouly, if you send in invalid data or an invalid key, it will decrypt into incorrect data, that goes without saying. But it should still be the same block size and not contain random unrelated memory blocks, shouldn' it? --=20 =A0Magnus Hagander =A0Me: http://www.hagander.net/ =A0Work: http://www.redpill-linpro.com/
On Fri, Jan 27, 2012 at 8:00 PM, Magnus Hagander <magnus@hagander.net> wrot= e: > On Fri, Jan 27, 2012 at 18:54, Marko Kreen <markokr@gmail.com> wrote: >> On Fri, Jan 27, 2012 at 7:34 PM, Stefan Kaltenbrunner >> <stefan@kaltenbrunner.cc> wrote: >>> On 01/27/2012 04:20 PM, Marko Kreen wrote: >>>> On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote: >>>> Yeah, it should be fixed. =A0But 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 cont= ent. >> >> You *will* get random memory content. =A0If your app is exploitable with >> invalid data, you *will* get exploited. =A0The decrypt() checks are >> more for developer convenience than anything more serious. > > Hold on. I hope there's some misunderstanding here. > > I hope you are you saying that feeding random data to the decrypt > functions should be expected to return random data out of previously > free()d areas? Surely you're not? > > Obviouly, if you send in invalid data or an invalid key, it will > decrypt into incorrect data, that goes without saying. But it should > still be the same block size and not contain random unrelated memory > blocks, shouldn' it? Yes, it should not contain unrelated data. --=20 marko
On 01/27/2012 07:06 PM, Marko Kreen wrote: > On Fri, Jan 27, 2012 at 8:00 PM, Magnus Hagander <magnus@hagander.net> wrote: >> On Fri, Jan 27, 2012 at 18:54, Marko Kreen <markokr@gmail.com> wrote: >>> On Fri, Jan 27, 2012 at 7:34 PM, Stefan Kaltenbrunner >>> <stefan@kaltenbrunner.cc> wrote: >>>> On 01/27/2012 04:20 PM, Marko Kreen wrote: >>>>> On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote: >>>>> 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. >>> >>> You *will* get random memory content. If your app is exploitable with >>> invalid data, you *will* get exploited. The decrypt() checks are >>> more for developer convenience than anything more serious. >> >> Hold on. I hope there's some misunderstanding here. >> >> I hope you are you saying that feeding random data to the decrypt >> functions should be expected to return random data out of previously >> free()d areas? Surely you're not? >> >> Obviouly, if you send in invalid data or an invalid key, it will >> decrypt into incorrect data, that goes without saying. But it should >> still be the same block size and not contain random unrelated memory >> blocks, shouldn' it? > > Yes, it should not contain unrelated data. hmm - see the last example I had in my original report - not sure i consider the path to pgcrypto.so "related" data and I most definitly do not expect to get that back from sending invalid data to the database... Stefan
On Fri, Jan 27, 2012 at 8:10 PM, Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> wrote: > On 01/27/2012 07:06 PM, Marko Kreen wrote: >> On Fri, Jan 27, 2012 at 8:00 PM, Magnus Hagander <magnus@hagander.net> w= rote: >>> On Fri, Jan 27, 2012 at 18:54, Marko Kreen <markokr@gmail.com> wrote: >>>> On Fri, Jan 27, 2012 at 7:34 PM, Stefan Kaltenbrunner >>>> <stefan@kaltenbrunner.cc> wrote: >>>>> On 01/27/2012 04:20 PM, Marko Kreen wrote: >>>>>> On Fri, Jan 27, 2012 at 01:37:11AM -0500, Tom Lane wrote: >>>>>> Yeah, it should be fixed. =A0But 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 com= ing >>>>> from external untrusted systems - so if the data is (completely) inva= lid >>>>> really want to get a proper error message instead of random memory co= ntent. >>>> >>>> You *will* get random memory content. =A0If your app is exploitable wi= th >>>> invalid data, you *will* get exploited. =A0The decrypt() checks are >>>> more for developer convenience than anything more serious. >>> >>> Hold on. I hope there's some misunderstanding here. >>> >>> I hope you are you saying that feeding random data to the decrypt >>> functions should be expected to return random data out of previously >>> free()d areas? Surely you're not? >>> >>> Obviouly, if you send in invalid data or an invalid key, it will >>> decrypt into incorrect data, that goes without saying. But it should >>> still be the same block size and not contain random unrelated memory >>> blocks, shouldn' it? >> >> Yes, it should not contain unrelated data. > > hmm - see the last example I had in my original report - not sure i > consider the path to pgcrypto.so "related" data and I most definitly do > not expect to get that back from sending invalid data to the database... Yeah, the bug causes return of palloced but uninitialized data. That needs to be fixed. --=20 marko
Marko Kreen <markokr@gmail.com> writes:
> On Fri, Jan 27, 2012 at 12:13:21PM -0500, Tom Lane wrote:
>> I think we should fix and back-patch these two specific bugs. The
>> openssl.c change sounds like it might be something for HEAD only.
> Now I looked more in-depth and seems my comments were off - error
> detection for encrypt()/decrypt() happens in px.c not in
> internal.c/openssl.c. Latter ones simply validate internal APIs.
> So attached patch should be enough to fix the issue.
Applied, thanks.
regards, tom lane