On 12.03.21 08:51, Peter Eisentraut wrote:
On 11.03.21 11:41, Daniel Gustafsson wrote:
.. and apply the padding changes as proposed in a patch upthread like
this (these
work for all OpenSSL versions I've tested, and I'm rather more puzzled
as to
why we got away with not having them in the past):
Yes, before proceeding with this, we should probably understand why
these changes are effective and why they haven't been required in the past.
I took another look at this with openssl-3.0.0-beta1. The issue with
the garbled padding output is still there. What I found is that
pgcrypto has been using the encryption and decryption API slightly
incorrectly. You are supposed to call EVP_DecryptUpdate() followed by
EVP_DecryptFinal_ex() (and similarly for encryption), but pgcrypto
doesn't do the second one. (To be fair, this API was added to OpenSSL
after pgcrypto first appeared.) The "final" functions take care of the
padding. We have been getting away with it like this because we do the
padding manually elsewhere. But apparently, something has changed in
OpenSSL 3.0.0 in that if padding is enabled in OpenSSL,
EVP_DecryptUpdate() doesn't flush the last normal block until the
"final" function is called.
Your proposed fix was to explicitly disable padding, and then this
problem goes away. You can still call the "final" functions, but they
won't do anything, except check that there is no more data left, but we
already check that elsewhere.
Another option is to throw out our own padding code and let OpenSSL
handle it. See attached demo patch. But that breaks the non-OpenSSL
code in internal.c, so we'd have to re-add the padding code there. So
this isn't quite as straightforward an option. (At least, with the
patch we can confirm that the OpenSSL padding works consistently with
our own implementation.)
So I think your proposed patch is sound and a good short-term and
low-risk solution.
From be5882c7e7b80a6d213eee88a8960f6c9773f958 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sat, 3 Jul 2021 15:39:30 +0200
Subject: [PATCH] Use EVP_EncryptFinal_ex() and EVP_DecryptFinal_ex()
---
contrib/pgcrypto/openssl.c | 22 +++++--
contrib/pgcrypto/pgp-cfb.c | 4 +-
contrib/pgcrypto/px.c | 119 +------------------------------------
contrib/pgcrypto/px.h | 12 ++--
4 files changed, 27 insertions(+), 130 deletions(-)
diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index ed8e74a2b9..68fd61b716 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -369,16 +369,18 @@ gen_ossl_free(PX_Cipher *c)
}
static int
-gen_ossl_decrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
- uint8 *res)
+gen_ossl_decrypt(PX_Cipher *c, int padding, const uint8 *data, unsigned dlen,
+ uint8 *res, unsigned *rlen)
{
OSSLCipher *od = c->ptr;
- int outlen;
+ int outlen, outlen2;
if (!od->init)
{
if (!EVP_DecryptInit_ex(od->evp_ctx, od->evp_ciph, NULL, NULL,
NULL))
return PXE_CIPHER_INIT;
+ if (!EVP_CIPHER_CTX_set_padding(od->evp_ctx, padding))
+ return PXE_CIPHER_INIT;
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))
@@ -388,21 +390,26 @@ gen_ossl_decrypt(PX_Cipher *c, const uint8 *data,
unsigned dlen,
if (!EVP_DecryptUpdate(od->evp_ctx, res, &outlen, data, dlen))
return PXE_DECRYPT_FAILED;
+ if (!EVP_DecryptFinal_ex(od->evp_ctx, res + outlen, &outlen2))
+ return PXE_DECRYPT_FAILED;
+ *rlen = outlen + outlen2;
return 0;
}
static int
-gen_ossl_encrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
- uint8 *res)
+gen_ossl_encrypt(PX_Cipher *c, int padding, const uint8 *data, unsigned dlen,
+ uint8 *res, unsigned *rlen)
{
OSSLCipher *od = c->ptr;
- int outlen;
+ int outlen, outlen2;
if (!od->init)
{
if (!EVP_EncryptInit_ex(od->evp_ctx, od->evp_ciph, NULL, NULL,
NULL))
return PXE_CIPHER_INIT;
+ if (!EVP_CIPHER_CTX_set_padding(od->evp_ctx, padding))
+ return PXE_CIPHER_INIT;
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))
@@ -412,6 +419,9 @@ gen_ossl_encrypt(PX_Cipher *c, const uint8 *data, unsigned
dlen,
if (!EVP_EncryptUpdate(od->evp_ctx, res, &outlen, data, dlen))
return PXE_ENCRYPT_FAILED;
+ if (!EVP_EncryptFinal_ex(od->evp_ctx, res + outlen, &outlen2))
+ return PXE_ENCRYPT_FAILED;
+ *rlen = outlen + outlen2;
return 0;
}
diff --git a/contrib/pgcrypto/pgp-cfb.c b/contrib/pgcrypto/pgp-cfb.c
index dafa562daa..de41e825b0 100644
--- a/contrib/pgcrypto/pgp-cfb.c
+++ b/contrib/pgcrypto/pgp-cfb.c
@@ -220,7 +220,9 @@ cfb_process(PGP_CFB *ctx, const uint8 *data, int len, uint8
*dst,
while (len > 0)
{
- px_cipher_encrypt(ctx->ciph, ctx->fr, ctx->block_size,
ctx->fre);
+ unsigned rlen;
+
+ px_cipher_encrypt(ctx->ciph, 0, ctx->fr, ctx->block_size,
ctx->fre, &rlen);
if (ctx->block_no < 5)
ctx->block_no++;
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 4205e9c3ef..5ccab44122 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -223,129 +223,14 @@ static int
combo_encrypt(PX_Combo *cx, const uint8 *data, unsigned dlen,
uint8 *res, unsigned *rlen)
{
- int err = 0;
- uint8 *bbuf;
- unsigned bs,
- bpos,
- i,
- pad;
-
- PX_Cipher *c = cx->cipher;
-
- bbuf = NULL;
- bs = px_cipher_block_size(c);
-
- /* encrypt */
- if (bs > 1)
- {
- bbuf = palloc(bs * 4);
- bpos = dlen % bs;
- *rlen = dlen - bpos;
- memcpy(bbuf, data + *rlen, bpos);
-
- /* encrypt full-block data */
- if (*rlen)
- {
- err = px_cipher_encrypt(c, data, *rlen, res);
- if (err)
- goto out;
- }
-
- /* bbuf has now bpos bytes of stuff */
- if (cx->padding)
- {
- pad = bs - (bpos % bs);
- for (i = 0; i < pad; i++)
- bbuf[bpos++] = pad;
- }
- else if (bpos % bs)
- {
- /* ERROR? */
- pad = bs - (bpos % bs);
- for (i = 0; i < pad; i++)
- bbuf[bpos++] = 0;
- }
-
- /* encrypt the rest - pad */
- if (bpos)
- {
- err = px_cipher_encrypt(c, bbuf, bpos, res + *rlen);
- *rlen += bpos;
- }
- }
- else
- {
- /* stream cipher/mode - no pad needed */
- err = px_cipher_encrypt(c, data, dlen, res);
- if (err)
- goto out;
- *rlen = dlen;
- }
-out:
- if (bbuf)
- pfree(bbuf);
-
- return err;
+ return px_cipher_encrypt(cx->cipher, cx->padding, data, dlen, res,
rlen);
}
static int
combo_decrypt(PX_Combo *cx, const uint8 *data, unsigned dlen,
uint8 *res, unsigned *rlen)
{
- int err = 0;
- unsigned bs,
- i,
- pad;
- unsigned pad_ok;
-
- PX_Cipher *c = cx->cipher;
-
- /* decide whether zero-length input is allowed */
- if (dlen == 0)
- {
- /* with padding, empty ciphertext is not allowed */
- if (cx->padding)
- return PXE_DECRYPT_FAILED;
-
- /* without padding, report empty result */
- *rlen = 0;
- return 0;
- }
-
- bs = px_cipher_block_size(c);
- if (bs > 1 && (dlen % bs) != 0)
- goto block_error;
-
- /* decrypt */
- *rlen = dlen;
- err = px_cipher_decrypt(c, data, dlen, res);
- if (err)
- return err;
-
- /* unpad */
- if (bs > 1 && cx->padding)
- {
- pad = res[*rlen - 1];
- pad_ok = 0;
- if (pad > 0 && pad <= bs && pad <= *rlen)
- {
- pad_ok = 1;
- for (i = *rlen - pad; i < *rlen; i++)
- if (res[i] != pad)
- {
- pad_ok = 0;
- break;
- }
- }
-
- if (pad_ok)
- *rlen -= pad;
- }
-
- return 0;
-
-block_error:
- return PXE_NOTBLOCKSIZE;
+ return px_cipher_decrypt(cx->cipher, cx->padding, data, dlen, res,
rlen);
}
static void
diff --git a/contrib/pgcrypto/px.h b/contrib/pgcrypto/px.h
index 17d6f22498..9348d6c997 100644
--- a/contrib/pgcrypto/px.h
+++ b/contrib/pgcrypto/px.h
@@ -143,8 +143,8 @@ struct px_cipher
unsigned (*iv_size) (PX_Cipher *c);
int (*init) (PX_Cipher *c, const uint8 *key,
unsigned klen, const uint8 *iv);
- int (*encrypt) (PX_Cipher *c, const uint8 *data,
unsigned dlen, uint8 *res);
- int (*decrypt) (PX_Cipher *c, const uint8 *data,
unsigned dlen, uint8 *res);
+ int (*encrypt) (PX_Cipher *c, int padding, const
uint8 *data, unsigned dlen, uint8 *res, unsigned *rlen);
+ int (*decrypt) (PX_Cipher *c, int padding, const
uint8 *data, unsigned dlen, uint8 *res, unsigned *rlen);
void (*free) (PX_Cipher *c);
/* private */
void *ptr;
@@ -207,10 +207,10 @@ void px_debug(const char *fmt,...)
pg_attribute_printf(1, 2);
#define px_cipher_block_size(c) (c)->block_size(c)
#define px_cipher_iv_size(c) (c)->iv_size(c)
#define px_cipher_init(c, k, klen, iv) (c)->init(c, k, klen, iv)
-#define px_cipher_encrypt(c, data, dlen, res) \
- (c)->encrypt(c, data, dlen, res)
-#define px_cipher_decrypt(c, data, dlen, res) \
- (c)->decrypt(c, data, dlen, res)
+#define px_cipher_encrypt(c, padding, data, dlen, res, rlen) \
+ (c)->encrypt(c, padding, data, dlen,
res, rlen)
+#define px_cipher_decrypt(c, padding, data, dlen, res, rlen) \
+ (c)->decrypt(c, padding, data, dlen,
res, rlen)
#define px_cipher_free(c) (c)->free(c)
--
2.32.0
