pgcrypto & strong ciphers limitation
Stefan reported me that prcrypto regression test fails on solaris 10
with openssl support. I investigated this problem and the result is that
Solaris 10 delivers only support for short keys up to 128. Strong crypto
(SUNWcry and SUNWcryr packages) is available on web download pages. (It
is result of US crypto export policy.)
However, on default installation (which is commonly used) it is a
problem. Regression test cannot be fixed because it tests strong
ciphers, but there two very strange issue:
1) First issue is blowfish cipher. Because pgcrypto uses old interface
instead new "evp" it calls bf_set_key function which does not return any
output and cut key if it is too long. See
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/bf/bf_skey.c
line 84.
If user installs strong crypto he will not be able decrypt data which
has been encrypted before.
The fix of this issue is ugly, because there is not way how to verify
supported key length with old openssl API and only new API return err if
length is not supported.
2) AES ciphere crashes when key is longer. It happens because return
value from AES_set_encrypt_key is ignored and AES_encrypt is called with
uninitialized structure.
I attach patch which fix both issues, but main problem is there that old
openssl API is used and supported key lengths are hardcoded. I think we
can add to TODO list rewrite pgcrypto to use evp openssl interface.
Any comments?
Zdenek
Attachments:
openssl.difftext/x-patch; name=openssl.diffDownload
Index: openssl.c
===================================================================
RCS file: /projects/cvsroot/pgsql/contrib/pgcrypto/openssl.c,v
retrieving revision 1.30
diff -c -r1.30 openssl.c
*** openssl.c 4 Oct 2006 00:29:46 -0000 1.30
--- openssl.c 24 Jul 2007 11:20:02 -0000
***************
*** 380,385 ****
--- 380,399 ----
{
ossldata *od = c->ptr;
+ /* Test if key len is supported. BF_set_key silently cut large keys and it could be
+ be a problem when user transfer crypted data from one server to another. */
+ EVP_CIPHER_CTX ctx;
+ EVP_CIPHER_CTX_init(&ctx);
+ EVP_EncryptInit_ex(&ctx, EVP_bf_cbc(), NULL, NULL, NULL);
+ EVP_CIPHER_CTX_set_key_length(&ctx,klen);
+ if( !EVP_EncryptInit_ex(&ctx,NULL, NULL, key, NULL) )
+ {
+ EVP_CIPHER_CTX_cleanup(&ctx);
+ return PXE_KEY_TOO_BIG;
+ }
+ EVP_CIPHER_CTX_cleanup(&ctx);
+
+ /* Key len is supported. We can use it. */
BF_set_key(&od->u.bf.key, klen, key);
if (iv)
memcpy(od->iv, iv, BF_BLOCK);
***************
*** 692,705 ****
return 0;
}
! static void
ossl_aes_key_init(ossldata * od, int type)
{
if (type == AES_ENCRYPT)
! AES_set_encrypt_key(od->key, od->klen * 8, &od->u.aes_key);
else
! AES_set_decrypt_key(od->key, od->klen * 8, &od->u.aes_key);
! od->init = 1;
}
static int
--- 706,728 ----
return 0;
}
! static int
ossl_aes_key_init(ossldata * od, int type)
{
+ int err;
+ /* Strong key support could miss on some openssl installation, we must
+ check return value, from set key function.
+ */
if (type == AES_ENCRYPT)
! err = AES_set_encrypt_key(od->key, od->klen * 8, &od->u.aes_key);
else
! err = AES_set_decrypt_key(od->key, od->klen * 8, &od->u.aes_key);
!
! if (err == 0)
! od->init = 1;
! else
! od->init = 0;
! return err;
}
static int
***************
*** 711,717 ****
const uint8 *end = data + dlen - bs;
if (!od->init)
! ossl_aes_key_init(od, AES_ENCRYPT);
for (; data <= end; data += bs, res += bs)
AES_ecb_encrypt(data, res, &od->u.aes_key, AES_ENCRYPT);
--- 734,741 ----
const uint8 *end = data + dlen - bs;
if (!od->init)
! if( ossl_aes_key_init(od, AES_ENCRYPT) )
! return PXE_KEY_TOO_BIG;
for (; data <= end; data += bs, res += bs)
AES_ecb_encrypt(data, res, &od->u.aes_key, AES_ENCRYPT);
***************
*** 727,733 ****
const uint8 *end = data + dlen - bs;
if (!od->init)
! ossl_aes_key_init(od, AES_DECRYPT);
for (; data <= end; data += bs, res += bs)
AES_ecb_encrypt(data, res, &od->u.aes_key, AES_DECRYPT);
--- 751,758 ----
const uint8 *end = data + dlen - bs;
if (!od->init)
! if( ossl_aes_key_init(od, AES_DECRYPT) )
! return PXE_KEY_TOO_BIG;
for (; data <= end; data += bs, res += bs)
AES_ecb_encrypt(data, res, &od->u.aes_key, AES_DECRYPT);
***************
*** 741,748 ****
ossldata *od = c->ptr;
if (!od->init)
! ossl_aes_key_init(od, AES_ENCRYPT);
!
AES_cbc_encrypt(data, res, dlen, &od->u.aes_key, od->iv, AES_ENCRYPT);
return 0;
}
--- 766,774 ----
ossldata *od = c->ptr;
if (!od->init)
! if( ossl_aes_key_init(od, AES_ENCRYPT) )
! return PXE_KEY_TOO_BIG;
!
AES_cbc_encrypt(data, res, dlen, &od->u.aes_key, od->iv, AES_ENCRYPT);
return 0;
}
***************
*** 754,760 ****
ossldata *od = c->ptr;
if (!od->init)
! ossl_aes_key_init(od, AES_DECRYPT);
AES_cbc_encrypt(data, res, dlen, &od->u.aes_key, od->iv, AES_DECRYPT);
return 0;
--- 780,787 ----
ossldata *od = c->ptr;
if (!od->init)
! if( ossl_aes_key_init(od, AES_DECRYPT) )
! return PXE_KEY_TOO_BIG;
AES_cbc_encrypt(data, res, dlen, &od->u.aes_key, od->iv, AES_DECRYPT);
return 0;
On 7/24/07, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:
Stefan reported me that prcrypto regression test fails on solaris 10
with openssl support. I investigated this problem and the result is that
Solaris 10 delivers only support for short keys up to 128. Strong crypto
(SUNWcry and SUNWcryr packages) is available on web download pages. (It
is result of US crypto export policy.)
Ugh, deliberately broken OpenSSL...
However, on default installation (which is commonly used) it is a
problem. Regression test cannot be fixed because it tests strong
ciphers, but there two very strange issue:1) First issue is blowfish cipher. Because pgcrypto uses old interface
instead new "evp" it calls bf_set_key function which does not return any
output and cut key if it is too long. See
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/bf/bf_skey.c
line 84.If user installs strong crypto he will not be able decrypt data which
has been encrypted before.The fix of this issue is ugly, because there is not way how to verify
supported key length with old openssl API and only new API return err if
length is not supported.
NAK. The fix is broken because it uses EVP interface. EVP is not
a general-purpose interface because not all valid keys for cipher
pass thru it. Only key-lengths used in SSL will work...
Could you rework the fix that it uses the BF_* interface,
does a test-encoding with full-length key and compares it to
expected result. And does it just once, not on each call.
That should be put into separate function probably.
2) AES ciphere crashes when key is longer. It happens because return
value from AES_set_encrypt_key is ignored and AES_encrypt is called with
uninitialized structure.
ACK, error checking is good. But please return PXE_KEY_TOO_BIG
directly from ossl_aes_key_init.
I must admit the internal API for ciphers is clumsy and could
need rework to something saner. This shows here.
I attach patch which fix both issues, but main problem is there that old
openssl API is used and supported key lengths are hardcoded. I think we
can add to TODO list rewrite pgcrypto to use evp openssl interface.
pgcrypto _was_ written using EVP, but I needed to rewrite it
when I found out EVP supports only key lengths used in SSL.
--
marko
Marko Kreen wrote:
On 7/24/07, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:
However, on default installation (which is commonly used) it is a
problem. Regression test cannot be fixed because it tests strong
ciphers, but there two very strange issue:1) First issue is blowfish cipher. Because pgcrypto uses old interface
instead new "evp" it calls bf_set_key function which does not return any
output and cut key if it is too long. See
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/bf/bf_skey.cline 84.
If user installs strong crypto he will not be able decrypt data which
has been encrypted before.The fix of this issue is ugly, because there is not way how to verify
supported key length with old openssl API and only new API return err if
length is not supported.NAK. The fix is broken because it uses EVP interface. EVP is not
a general-purpose interface because not all valid keys for cipher
pass thru it. Only key-lengths used in SSL will work...
I'm not openssl expert, but if you look how to EVP call for setkey is
implemented you can see that finally is call BF_set_key. Only there is
one extra layer see
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/evp/e_bf.c
Could you rework the fix that it uses the BF_* interface,
does a test-encoding with full-length key and compares it to
expected result. And does it just once, not on each call.
OK. I can do, but it is not general solution. Because it will work only
in our case, because we know 128 is a restricted limit.
That should be put into separate function probably.
yes
2) AES ciphere crashes when key is longer. It happens because return
value from AES_set_encrypt_key is ignored and AES_encrypt is called with
uninitialized structure.ACK, error checking is good. But please return PXE_KEY_TOO_BIG
directly from ossl_aes_key_init.
OK.
I must admit the internal API for ciphers is clumsy and could
need rework to something saner. This shows here.I attach patch which fix both issues, but main problem is there that old
openssl API is used and supported key lengths are hardcoded. I think we
can add to TODO list rewrite pgcrypto to use evp openssl interface.pgcrypto _was_ written using EVP, but I needed to rewrite it
when I found out EVP supports only key lengths used in SSL.
Is it still correct? It seems that blowfish accepts all key range, but
How I mention I'm not openssl guru and documentation is very bad :(.
Zdenek
On 7/24/07, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:
Marko Kreen wrote:
NAK. The fix is broken because it uses EVP interface. EVP is not
a general-purpose interface because not all valid keys for cipher
pass thru it. Only key-lengths used in SSL will work...I'm not openssl expert, but if you look how to EVP call for setkey is
implemented you can see that finally is call BF_set_key. Only there is
one extra layer see
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/evp/e_bf.c
I glanced into evp.h for 0.9.7 and 0.9.6j and remembered that
there were 2 things EVP forced - key length and padding.
When I replied to you I remembered things bit wrong, there are
indeed way for changing key size even in 0.9.6, but not for
padding. EVP_CIPHER_CTX_set_padding() appers in only in 0.9.7.
I suspect as I could not work around forced padding I did not
research key size issue very deeply.
So we can revisit the issue when we are ready to drop
support for 0.9.6x.
Could you rework the fix that it uses the BF_* interface,
does a test-encoding with full-length key and compares it to
expected result. And does it just once, not on each call.OK. I can do, but it is not general solution. Because it will work only
in our case, because we know 128 is a restricted limit.
It _is_ a general solution if you test with a 448 bit key.
Using BF_ API but testing via EVP_ API is unobvious first,
in addition leaving the user depending whether the molesters
got all the details right.
When everything uses EVP then indeed, we can test via EVP.
I must admit the internal API for ciphers is clumsy and could
need rework to something saner. This shows here.I attach patch which fix both issues, but main problem is there that old
openssl API is used and supported key lengths are hardcoded. I think we
can add to TODO list rewrite pgcrypto to use evp openssl interface.pgcrypto _was_ written using EVP, but I needed to rewrite it
when I found out EVP supports only key lengths used in SSL.Is it still correct? It seems that blowfish accepts all key range, but
Yes, seems since 0.9.7 we could work with EVP.
How I mention I'm not openssl guru and documentation is very bad :(.
It's somewhat lacking, yes. User is forced to read their source
which isn't very nice either...
--
marko
Marko Kreen wrote:
On 7/24/07, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote:
Marko Kreen wrote:
NAK. The fix is broken because it uses EVP interface. EVP is not
a general-purpose interface because not all valid keys for cipher
pass thru it. Only key-lengths used in SSL will work...I'm not openssl expert, but if you look how to EVP call for setkey is
implemented you can see that finally is call BF_set_key. Only there is
one extra layer see
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/evp/e_bf.cI glanced into evp.h for 0.9.7 and 0.9.6j and remembered that
there were 2 things EVP forced - key length and padding.When I replied to you I remembered things bit wrong, there are
indeed way for changing key size even in 0.9.6, but not for
padding. EVP_CIPHER_CTX_set_padding() appers in only in 0.9.7.I suspect as I could not work around forced padding I did not
research key size issue very deeply.So we can revisit the issue when we are ready to drop
support for 0.9.6x.
the last openssl 0.9.6 release was in march 2004 and 0.9.7 is available
since early 2003 - I don't think dropping support for it in 8.3+ would
be unreasonable at all ...
Stefan
On 7/24/07, Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> wrote:
Marko Kreen wrote:
So we can revisit the issue when we are ready to drop
support for 0.9.6x.the last openssl 0.9.6 release was in march 2004 and 0.9.7 is available
since early 2003 - I don't think dropping support for it in 8.3+ would
be unreasonable at all ...
Now that I think about it, then yes, dropping 0.9.6 from 8.4
onwards should be no problem. Considering the code could need
good cleanup anyway, that may be a good moment for it.
--
marko
Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
Marko Kreen wrote:
So we can revisit the issue when we are ready to drop
support for 0.9.6x.
the last openssl 0.9.6 release was in march 2004 and 0.9.7 is available
since early 2003 - I don't think dropping support for it in 8.3+ would
be unreasonable at all ...
Any major rewrite of pgcrypto would be for 8.4 (or later) at this point.
I tend to agree that we could drop 0.9.6x support in that timeframe.
regards, tom lane
Just confirming, this should be applied to 8.3, right?
---------------------------------------------------------------------------
Zdenek Kotala wrote:
Stefan reported me that prcrypto regression test fails on solaris 10
with openssl support. I investigated this problem and the result is that
Solaris 10 delivers only support for short keys up to 128. Strong crypto
(SUNWcry and SUNWcryr packages) is available on web download pages. (It
is result of US crypto export policy.)However, on default installation (which is commonly used) it is a
problem. Regression test cannot be fixed because it tests strong
ciphers, but there two very strange issue:1) First issue is blowfish cipher. Because pgcrypto uses old interface
instead new "evp" it calls bf_set_key function which does not return any
output and cut key if it is too long. See
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/common/openssl/crypto/bf/bf_skey.c
line 84.If user installs strong crypto he will not be able decrypt data which
has been encrypted before.The fix of this issue is ugly, because there is not way how to verify
supported key length with old openssl API and only new API return err if
length is not supported.2) AES ciphere crashes when key is longer. It happens because return
value from AES_set_encrypt_key is ignored and AES_encrypt is called with
uninitialized structure.I attach patch which fix both issues, but main problem is there that old
openssl API is used and supported key lengths are hardcoded. I think we
can add to TODO list rewrite pgcrypto to use evp openssl interface.Any comments?
Zdenek
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
Just confirming, this should be applied to 8.3, right?
I think marko is working on an updated patch for this:
http://archives.postgresql.org/pgsql-hackers/2007-09/msg00386.php
without that the backend will coredump if ones uses string ciphers with
pgcrypto on a default solaris install so it seems like a thing we should
fix for 8.3.
Stefan
Stefan Kaltenbrunner wrote:
Bruce Momjian wrote:
Just confirming, this should be applied to 8.3, right?
I think marko is working on an updated patch for this:
http://archives.postgresql.org/pgsql-hackers/2007-09/msg00386.php
without that the backend will coredump if ones uses string ciphers with
pgcrypto on a default solaris install so it seems like a thing we should
fix for 8.3.
Yes, I also would like to have backport for 8.2 and 8.1. Because this
branch are also affected. (I think backport is easy there are no much
change between 8.1 and 8.3)
Zdenek