pgcrypto compilation error due to stack-allocated EVP_CIPHER_CTX

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

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

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

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

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

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

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

Hi Michael,

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

Regards,
Muhammad Asif Naeem

On Tue, Dec 6, 2016 at 12:15 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

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

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.

On Tue, Dec 6, 2016 at 6:15 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

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

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

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.

/* 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;
}

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

On Wed, Dec 7, 2016 at 2:23 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

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

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

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

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

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

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

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