On Mon, Dec 5, 2016 at 6:09 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Mon, Dec 5, 2016 at 5:11 PM, Heikki Linnakangas <hlinn...@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
diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index 1d3e58d925..b0487081f9 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -248,17 +248,72 @@ struct ossl_cipher
int max_key_size;
};
-typedef struct
+/*
+ * To make sure we don't leak OpenSSL handles on abort, we keep ossldata
+ * objects in a linked list, allocated in TopMemoryContext. We use the
+ * ResourceOwner mechanism to free them on abort.
+ */
+typedef struct ossldata
{
- EVP_CIPHER_CTX evp_ctx;
+ EVP_CIPHER_CTX *evp_ctx;
const EVP_CIPHER *evp_ciph;
uint8 key[MAX_KEY];
uint8 iv[MAX_IV];
unsigned klen;
unsigned init;
const struct ossl_cipher *ciph;
+
+ ResourceOwner owner;
+ struct ossldata *next;
+ struct ossldata *prev;
} ossldata;
+static ossldata *open_ossls = NULL;
+static bool ossl_callback_registered = false;
+
+static void
+free_ossldata(ossldata *od)
+{
+ EVP_CIPHER_CTX_free(od->evp_ctx);
+ if (od->prev)
+ od->prev->next = od->next;
+ else
+ open_ossls = od->next;
+ if (od->next)
+ od->next->prev = od->prev;
+ pfree(od);
+}
+
+/*
+ * Close any open OpenSSL handles on abort.
+ */
+static void
+ossldata_free_callback(ResourceReleasePhase phase,
+ bool isCommit,
+ bool isTopLevel,
+ void *arg)
+{
+ ossldata *curr;
+ ossldata *next;
+
+ if (phase != RESOURCE_RELEASE_AFTER_LOCKS)
+ return;
+
+ next = open_ossls;
+ while (next)
+ {
+ curr = next;
+ next = curr->next;
+
+ if (curr->owner == CurrentResourceOwner)
+ {
+ if (isCommit)
+ elog(WARNING, "pgcrypto ossldata reference leak: ossldata %p still referenced", curr);
+ free_ossldata(curr);
+ }
+ }
+}
+
/* Common routines for all algorithms */
static unsigned
@@ -292,9 +347,7 @@ gen_ossl_free(PX_Cipher *c)
{
ossldata *od = (ossldata *) c->ptr;
- EVP_CIPHER_CTX_cleanup(&od->evp_ctx);
- px_memset(od, 0, sizeof(*od));
- px_free(od);
+ free_ossldata(od);
px_free(c);
}
@@ -307,17 +360,17 @@ 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))
+ EVP_CIPHER_CTX_reset(od->evp_ctx);
+ 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;
}
- if (!EVP_DecryptUpdate(&od->evp_ctx, res, &outlen, data, dlen))
+ if (!EVP_DecryptUpdate(od->evp_ctx, res, &outlen, data, dlen))
return PXE_DECRYPT_FAILED;
return 0;
@@ -332,17 +385,17 @@ gen_ossl_encrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
if (!od->init)
{
- EVP_CIPHER_CTX_init(&od->evp_ctx);
- if (!EVP_EncryptInit_ex(&od->evp_ctx, od->evp_ciph, NULL, NULL, NULL))
+ EVP_CIPHER_CTX_reset(od->evp_ctx);
+ if (!EVP_EncryptInit_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_EncryptInit_ex(&od->evp_ctx, NULL, NULL, od->key, od->iv))
+ if (!EVP_EncryptInit_ex(od->evp_ctx, NULL, NULL, od->key, od->iv))
return PXE_CIPHER_INIT;
od->init = true;
}
- if (!EVP_EncryptUpdate(&od->evp_ctx, res, &outlen, data, dlen))
+ if (!EVP_EncryptUpdate(od->evp_ctx, res, &outlen, data, dlen))
return PXE_ERR_GENERIC;
return 0;
@@ -370,25 +423,32 @@ bf_check_supported_key_len(void)
static const uint8 data[8] = {0xfe, 0xdc, 0xba, 0x98, 0x76, 0x54, 0x32, 0x10};
static const uint8 res[8] = {0xc0, 0x45, 0x04, 0x01, 0x2e, 0x4e, 0x1f, 0x53};
uint8 out[8];
- EVP_CIPHER_CTX evp_ctx;
+ EVP_CIPHER_CTX *evp_ctx;
int outlen;
+ int status = 0;
/* encrypt with 448bits key and verify output */
- EVP_CIPHER_CTX_init(&evp_ctx);
- if (!EVP_EncryptInit_ex(&evp_ctx, EVP_bf_ecb(), NULL, NULL, NULL))
- return 0;
- if (!EVP_CIPHER_CTX_set_key_length(&evp_ctx, 56))
- return 0;
- if (!EVP_EncryptInit_ex(&evp_ctx, NULL, NULL, key, NULL))
- return 0;
-
- if (!EVP_EncryptUpdate(&evp_ctx, out, &outlen, data, 8))
- return 0;
+ 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)
- return 0; /* Output does not match -> strong cipher is
+ goto leave; /* Output does not match -> strong cipher is
* not supported */
- return 1;
+ status = 1;
+
+leave:
+ EVP_CIPHER_CTX_free(evp_ctx);
+ return status;
}
static int
@@ -682,8 +742,9 @@ int
px_find_cipher(const char *name, PX_Cipher **res)
{
const struct ossl_cipher_lookup *i;
- PX_Cipher *c = NULL;
- ossldata *od;
+ PX_Cipher *c = NULL;
+ EVP_CIPHER_CTX *ctx;
+ ossldata *od;
name = px_resolve_alias(ossl_aliases, name);
for (i = ossl_cipher_types; i->name; i++)
@@ -692,13 +753,38 @@ px_find_cipher(const char *name, PX_Cipher **res)
if (i->name == NULL)
return PXE_NO_CIPHER;
- od = px_alloc(sizeof(*od));
- memset(od, 0, sizeof(*od));
+ if (!ossl_callback_registered)
+ {
+ RegisterResourceReleaseCallback(ossldata_free_callback, NULL);
+ ossl_callback_registered = true;
+ }
+
+ /*
+ * Create an ossldata object, a EVP_CIPHER_CTX object and a PX_Cipher.
+ * The order is crucial, to make sure we don't leak anything on
+ * out-of-memory or other error.
+ */
+ od = MemoryContextAllocZero(TopMemoryContext, sizeof(*od));
od->ciph = i->ciph;
+ /* Allocate a EVP_CIPHER_CTX object. */
+ ctx = EVP_CIPHER_CTX_new();
+ if (!ctx)
+ {
+ pfree(od);
+ return -1;
+ }
+
+ od->evp_ctx = ctx;
+ od->owner = CurrentResourceOwner;
+ od->next = open_ossls;
+ od->prev = NULL;
+ open_ossls = od;
+
if (i->ciph->cipher_func)
od->evp_ciph = i->ciph->cipher_func();
+ /* The PX_Cipher is allocated in current memory context */
c = px_alloc(sizeof(*c));
c->block_size = gen_ossl_block_size;
c->key_size = gen_ossl_key_size;
diff --git a/contrib/pgcrypto/pgp-encrypt.c b/contrib/pgcrypto/pgp-encrypt.c
index be933bf86c..d510729e5b 100644
--- a/contrib/pgcrypto/pgp-encrypt.c
+++ b/contrib/pgcrypto/pgp-encrypt.c
@@ -219,6 +219,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);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
