Обсуждение: pgcrypto compilation error due to stack-allocated EVP_CIPHER_CTX

Hi,

It appears openssl has removed the public definition of EVP_CIPHER_CTX
leading to pgcrypto failing with:

/home/andres/src/postgresql/contrib/pgcrypto/openssl.c:253:17: error: field ‘evp_ctx’ has incomplete type
EVP_CIPHER_CTXevp_ctx;                ^~~~~~~
 
/home/andres/src/postgresql/contrib/pgcrypto/openssl.c: In function ‘bf_check_supported_key_len’:
/home/andres/src/postgresql/contrib/pgcrypto/openssl.c:373:17: error: storage size of ‘evp_ctx’ isn’t known
EVP_CIPHER_CTXevp_ctx;                ^~~~~~~
 
/home/andres/src/postgresql/contrib/pgcrypto/openssl.c:373:17: warning: unused variable ‘evp_ctx’ [-Wunused-variable]
make[3]: *** [openssl.o] Error 1

seems we need to allocate using EVP_CIPHER_CTX_new() instead.

Am I the only one seing this?

It looks like EVP_CIPHER_CTX_new() has been available for a long time:
commit b40228a61d2f9b40fa6a834c9beaa8ee9dc490c1
Author: Dr. Stephen Henson <steve@openssl.org>
Date:   2005-12-02 13:46:39 +0000
   New functions to support opaque EVP_CIPHER_CTX handling.

Greetings,

Andres Freund

On 12/01/2016 02:48 AM, Andres Freund wrote:
> It appears openssl has removed the public definition of EVP_CIPHER_CTX
> leading to pgcrypto failing with:

Yes, I believe this is one of the changes in OpenSSL 1.1. I guess you 
might be the first one to try to compile with 1.1 since 
5ff4a67f63fd6d3eb01ff9707d4674ed54a89f3b was pushed.

If we do not already have it I think we should get a build farm animal 
with OpenSSL 1.1.

Andreas

On Thu, Dec 1, 2016 at 11:17 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 12/01/2016 02:48 AM, Andres Freund wrote:
>>
>> It appears openssl has removed the public definition of EVP_CIPHER_CTX
>> leading to pgcrypto failing with:

That's not much surprising, most distributions are still on 1.0.2 as
1.1.0 has created many breakages so a bunch of projects need to patch
first. This burden may take a couple of years to sort out.

> Yes, I believe this is one of the changes in OpenSSL 1.1. I guess you might
> be the first one to try to compile with 1.1 since
> 5ff4a67f63fd6d3eb01ff9707d4674ed54a89f3b was pushed.

Yes, I can see the failure as well using 1.1.0 on my OSX laptop with
homebrew packages.

> If we do not already have it I think we should get a build farm animal with
> OpenSSL 1.1.

I would really like to do it, but ArchLinux ARM is still on 1.0.2, as
is ArchLinux :(

Finally, attached is a patch to address the failure. make check is
passing here for 1.1.0 and 1.0.2. The problem is that OpenSSL 1.1
relies on an opaque structure here so we need to have the pgcrypto
code rely on a pointer and not a direct declaration of the structure.
EVP_CIPHER_CTX_free() and EVP_CIPHER_CTX_new() have been introduced in
0.9.8 which is the oldest version supported by HEAD, and 5ff4a67f is
HEAD-only, so there is no need to back-patch here.

I am adding that to the next CF so as we don't forget about it. I'll
just switch my laptop to OpenSSL 1.1.0 by default once the issue is
fixed, homebrew has packages for 1.0.2 and 1.1.0, that's easy enough
to switch.
--
Michael

On 12/05/2016 05:19 AM, Michael Paquier wrote:
> On Thu, Dec 1, 2016 at 11:17 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> On 12/01/2016 02:48 AM, Andres Freund wrote:
>> Yes, I believe this is one of the changes in OpenSSL 1.1. I guess you might
>> be the first one to try to compile with 1.1 since
>> 5ff4a67f63fd6d3eb01ff9707d4674ed54a89f3b was pushed.
>
> Yes, I can see the failure as well using 1.1.0 on my OSX laptop with
> homebrew packages.

Sorry about that! Given that I just dealt with this same issue with 
EVP_MD_CTX_init, I should've noticed.

> Finally, attached is a patch to address the failure. make check is
> passing here for 1.1.0 and 1.0.2. The problem is that OpenSSL 1.1
> relies on an opaque structure here so we need to have the pgcrypto
> code rely on a pointer and not a direct declaration of the structure.
> EVP_CIPHER_CTX_free() and EVP_CIPHER_CTX_new() have been introduced in
> 0.9.8 which is the oldest version supported by HEAD, and 5ff4a67f is
> HEAD-only, so there is no need to back-patch here.

I'm afraid if we just start using EVP_CIPHER_CTX_new(), we'll leak the 
context on any error. We had exactly the same problem with 
EVP_MD_CTX_init being removed, in the patch that added OpenSSL 1.1.0 
support. We'll have to use a resource owner to track it, just like we 
did with EVP_MD_CTX in commit 593d4e47. Want to do that, or should I?

- Heikki

On Mon, Dec 5, 2016 at 5:11 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> I'm afraid if we just start using EVP_CIPHER_CTX_new(), we'll leak the
> context on any error. We had exactly the same problem with EVP_MD_CTX_init
> being removed, in the patch that added OpenSSL 1.1.0 support. We'll have to
> use a resource owner to track it, just like we did with EVP_MD_CTX in commit
> 593d4e47. Want to do that, or should I?

I'll send a patch within 24 hours.
-- 
Michael

On Mon, Dec 5, 2016 at 6:09 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Dec 5, 2016 at 5:11 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> I'm afraid if we just start using EVP_CIPHER_CTX_new(), we'll leak the
>> context on any error. We had exactly the same problem with EVP_MD_CTX_init
>> being removed, in the patch that added OpenSSL 1.1.0 support. We'll have to
>> use a resource owner to track it, just like we did with EVP_MD_CTX in commit
>> 593d4e47. Want to do that, or should I?
>
> I'll send a patch within 24 hours.

And within the delay, attached is the promised patch.

While testing with a linked list, I have found out that the linked
list could leak with cases like that, where decryption and encryption
are done in a single transaction;
select pgp_sym_decrypt(pgp_sym_encrypt(repeat('x',65530),'1'),'1') =
repeat('x',65530);

What has taken me a bit of time was to figure out that this bit is
needed to free correctly elements in the open list:
@@ -219,6 +220,8 @@ encrypt_free(void *priv)
 {
    struct EncStat *st = priv;

+   if (st->ciph)
+       pgp_cfb_free(st->ciph);
    px_memset(st, 0, sizeof(*st));
    px_free(st);
 }
This does not matter on back-branches as things get cleaned up once
the transaction memory context gets freed.
--
Michael

On Tue, Dec 6, 2016 at 9:31 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thank you for v2 patch, I would like to comment on it. It seems that you
> have used function EVP_CIPHER_CTX_reset in the patch that was introduced in
> OpenSSL 1.1.0, older library version might not work now, is it intentional
> change ?.

I thought I tested that... But yes, that would not compile when linked
with 1.0.2 or older. Using EVP_CIPHER_CTX_cleanup() is safe instead as
that's available down to 0.9.8.
--
Michael

On Tue, Dec 6, 2016 at 11:42 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thanks for updated patch. Although EVP_CIPHER_CTX_cleanup() seems deprecated
> in OpenSSL >= 1.1.0 i.e.
>
>> # if OPENSSL_API_COMPAT < 0x10100000L
>> #  define EVP_CIPHER_CTX_init(c)      EVP_CIPHER_CTX_reset(c)
>> #  define EVP_CIPHER_CTX_cleanup(c)   EVP_CIPHER_CTX_reset(c)
>> # endif
>
>
> I guess use of deprecated function is fine, until OpenSSL library support
> it.

We could use some ifdef block with the OpenSSL version number, but I
am not sure if that's worth complicating the code at this stage.
-- 
Michael

On Fri, Dec 9, 2016 at 1:11 AM, Asif Naeem <anaeem.it@gmail.com> wrote:
> It make sense. I would like to share more comments as following i.e.
>
>> static int
>> bf_check_supported_key_len(void)
>> {
>> ...
>>      /* encrypt with 448bits key and verify output */
>>      evp_ctx = EVP_CIPHER_CTX_new();
>>      if (!evp_ctx)
>>           return 1;
>>      if (!EVP_EncryptInit_ex(evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL))
>>           goto leave;
>>      if (!EVP_CIPHER_CTX_set_key_length(evp_ctx, 56))
>>           goto leave;
>>      if (!EVP_EncryptInit_ex(evp_ctx, NULL, NULL, key, NULL))
>>           goto leave;
>>      if (!EVP_EncryptUpdate(evp_ctx, out, &outlen, data, 8))
>>           goto leave;
>>      if (memcmp(out, res, 8) != 0)
>>           goto leave;                    /* Output does not match ->
>> strong cipher is
>>                                          * not supported */
>>      status = 1;
>> leave:
>>      EVP_CIPHER_CTX_free(evp_ctx);
>>      return status;
>> }
>
>
> It seems that it need to return 0 instead of 1 in case of failure i.e.

Yep that's wrong. Thanks for pointing that out.

>>      /* encrypt with 448bits key and verify output */
>>      evp_ctx = EVP_CIPHER_CTX_new();
>>      if (!evp_ctx)
>>           return 0;
>
> We can avoid multiple if conditions and goto statement something like i.e.
>
>>      if (EVP_EncryptInit_ex(evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL) &&
>>          EVP_CIPHER_CTX_set_key_length(evp_ctx, 56) &&
>>          EVP_EncryptInit_ex(evp_ctx, NULL, NULL, key, NULL) &&
>>          EVP_EncryptUpdate(evp_ctx, out, &outlen, data, 8) &&
>>          memcmp(out, res, 8) == 0 )) /* Output does not match -> strong
>> cipher is not supported */
>>      status = 1;
>>      EVP_CIPHER_CTX_free(evp_ctx);
>>      return status;
>> }

I thought about doing that as well to be honest :) One way or the
other is fine, still I recall seeing more the style of the current
patch in PG code though, and that sticks better with current HEAD. But
my impressions may be wrong.

> What is your opinion ?. I am hopeful I will be able to share all my findings
> tomorrow. Thanks.

Thanks for looking at the patch. Looking forward to hearing more!
-- 
Michael

On Fri, Dec 9, 2016 at 10:22 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Thanks for looking at the patch. Looking forward to hearing more!

Here is an updated patch based on which reviews should be done. I have
fixed the issue you have reported, and upon additional lookup I have
noticed that returning -1 when failing on EVP_CIPHER_CTX_new() in
px_find_cipher() is dead wrong. The error code should be
PXE_CIPHER_INIT.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 12/12/2016 07:18 AM, Michael Paquier wrote:
> On Fri, Dec 9, 2016 at 10:22 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Thanks for looking at the patch. Looking forward to hearing more!
>
> Here is an updated patch based on which reviews should be done. I have
> fixed the issue you have reported, and upon additional lookup I have
> noticed that returning -1 when failing on EVP_CIPHER_CTX_new() in
> px_find_cipher() is dead wrong. The error code should be
> PXE_CIPHER_INIT.

> @@ -307,17 +360,18 @@ gen_ossl_decrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
>
>         if (!od->init)
>         {
> -               EVP_CIPHER_CTX_init(&od->evp_ctx);
> -               if (!EVP_DecryptInit_ex(&od->evp_ctx, od->evp_ciph, NULL, NULL, NULL))
> +               if (!EVP_CIPHER_CTX_cleanup(od->evp_ctx))
> +                       return PXE_CIPHER_INIT;
> +               if (!EVP_DecryptInit_ex(od->evp_ctx, od->evp_ciph, NULL, NULL, NULL))
>                         return PXE_CIPHER_INIT;
> -               if (!EVP_CIPHER_CTX_set_key_length(&od->evp_ctx, od->klen))
> +               if (!EVP_CIPHER_CTX_set_key_length(od->evp_ctx, od->klen))
>                         return PXE_CIPHER_INIT;
> -               if (!EVP_DecryptInit_ex(&od->evp_ctx, NULL, NULL, od->key, od->iv))
> +               if (!EVP_DecryptInit_ex(od->evp_ctx, NULL, NULL, od->key, od->iv))
>                         return PXE_CIPHER_INIT;
>                 od->init = true;
>         }

The EVP_CIPHER_CTX_cleanup() call seems superfluous. We know that the 
context isn't initialized yet, so no need to clean it up.

Removed that, did some further cosmetic changes, and pushed. I renamed a 
bunch variables and structs, so that they are more consistent with the 
similar digest stuff.

Thanks!

- Heikki

On Mon, Dec 12, 2016 at 6:17 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Removed that, did some further cosmetic changes, and pushed. I renamed a
> bunch variables and structs, so that they are more consistent with the
> similar digest stuff.

That definitely makes sense this way, thanks for the commit.
-- 
Michael