Interesting. I have added test cases about this. Could you describe
how you arrived at the second test case?
Sure -- that second ciphertext is the result of running
SELECT encrypt_iv('abcdefghijklmnopqrstuvwxyz', '0123456', 'abcd', 'aes');
and then incrementing the last byte of the first block (i.e. the 16th
byte) to corrupt the padding in the last block.
I have added this information to the test file.
Is there any reasonable meaning of the previous behaviors?
I definitely don't think the previous behavior was reasonable. It's
possible that someone built a strange system on top of that
unreasonable behavior, but I hope not.
Does bad padding even give correct answers on decryption?
No -- or rather, I'm not really sure how to define "correct answer" for
garbage input. I especially don't like that two different ciphertexts
can silently decrypt to the same plaintext.
What does encryption without padding even do on incorrect input sizes?
Looks like it's being silently zero-padded by the previous code, which
doesn't seem very helpful to me. The documentation says "data must be
multiple of cipher block size" for the pad:none case, though I suppose
it doesn't say what happens if you ignore the "must".
Right, the previous behaviors were clearly faulty. I have updated the
commit message to call out the behavior change more clearly.
This patch is now complete from my perspective.
From 633d4aa0f762bf976e3ddc077cb9c34b58eeb4d5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Wed, 16 Mar 2022 10:58:17 +0100
Subject: [PATCH v4] pgcrypto: Remove internal padding implementation
Use the padding provided by OpenSSL instead of doing it ourselves.
The internal implementation was once applicable to the non-OpenSSL
code paths, but those have since been removed. The padding algorithm
is still the same.
The OpenSSL padding implementation is stricter than the previous
internal one: Bad padding during decryption is now an error, and
encryption without padding now requires the input size to be a
multiple of the block size, otherwise it is also an error.
Previously, these cases silently proceeded, in spite of the
documentation saying otherwise.
Add some test cases about this, too. (The test cases are in
rijndael.sql, but they apply to all encryption algorithms.)
Discussion:
https://www.postgresql.org/message-id/flat/ba94c26b-0c58-c97e-7a44-f44e08b4cca2%40enterprisedb.com
---
contrib/pgcrypto/expected/rijndael.out | 14 +++
contrib/pgcrypto/openssl.c | 22 +++--
contrib/pgcrypto/pgp-cfb.c | 4 +-
contrib/pgcrypto/px.c | 120 +------------------------
contrib/pgcrypto/px.h | 14 +--
contrib/pgcrypto/sql/rijndael.sql | 12 +++
6 files changed, 52 insertions(+), 134 deletions(-)
diff --git a/contrib/pgcrypto/expected/rijndael.out
b/contrib/pgcrypto/expected/rijndael.out
index ad69cdba49..015ba4430d 100644
--- a/contrib/pgcrypto/expected/rijndael.out
+++ b/contrib/pgcrypto/expected/rijndael.out
@@ -39,6 +39,12 @@ SELECT encrypt(
\x8ea2b7ca516745bfeafc49904b496089
(1 row)
+-- without padding, input not multiple of block size
+SELECT encrypt(
+'\x00112233445566778899aabbccddeeff00',
+'\x000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f',
+'aes-cbc/pad:none');
+ERROR: encrypt error: Encryption failed
-- key padding
SELECT encrypt(
'\x0011223344',
@@ -95,6 +101,14 @@ select encode(decrypt(encrypt('foo', '0123456', 'aes'),
'0123456', 'aes'), 'esca
foo
(1 row)
+-- data not multiple of block size
+select encode(decrypt(encrypt('foo', '0123456', 'aes') || '\x00'::bytea,
'0123456', 'aes'), 'escape');
+ERROR: decrypt error: Decryption failed
+-- bad padding
+-- (The input value is the result of encrypt_iv('abcdefghijklmnopqrstuvwxyz',
'0123456', 'abcd', 'aes')
+-- with the 16th byte changed (s/db/eb/) to corrupt the padding of the last
block.)
+select
encode(decrypt_iv('\xa21a9c15231465964e3396d32095e67eb52bab05f556a581621dee1b85385789',
'0123456', 'abcd', 'aes'), 'escape');
+ERROR: decrypt_iv error: Decryption failed
-- iv
select encrypt_iv('foo', '0123456', 'abcd', 'aes');
encrypt_iv
diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index e236b0d79c..68fd61b716 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -369,17 +369,17 @@ 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, 0))
+ 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;
@@ -390,22 +390,25 @@ 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, 0))
+ 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;
@@ -416,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 0010addaf7..c139798f3b 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -44,7 +44,6 @@ static const struct error_desc px_err_list[] = {
{PXE_ERR_GENERIC, "Some PX error (not specified)"},
{PXE_NO_HASH, "No such hash algorithm"},
{PXE_NO_CIPHER, "No such cipher algorithm"},
- {PXE_NOTBLOCKSIZE, "Data not a multiple of block size"},
{PXE_BAD_OPTION, "Unknown option"},
{PXE_BAD_FORMAT, "Badly formatted type"},
{PXE_KEY_TOO_BIG, "Key was too big"},
@@ -221,129 +220,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 eef49a8b76..f175862f8e 100644
--- a/contrib/pgcrypto/px.h
+++ b/contrib/pgcrypto/px.h
@@ -47,7 +47,7 @@
#define PXE_ERR_GENERIC -1
#define PXE_NO_HASH -2
#define PXE_NO_CIPHER -3
-#define PXE_NOTBLOCKSIZE -4
+/* -4 is unused */
#define PXE_BAD_OPTION -5
#define PXE_BAD_FORMAT -6
#define PXE_KEY_TOO_BIG -7
@@ -144,8 +144,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;
@@ -208,10 +208,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)
diff --git a/contrib/pgcrypto/sql/rijndael.sql
b/contrib/pgcrypto/sql/rijndael.sql
index b162fc61f5..a276641998 100644
--- a/contrib/pgcrypto/sql/rijndael.sql
+++ b/contrib/pgcrypto/sql/rijndael.sql
@@ -24,6 +24,12 @@
'\x000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f',
'aes-cbc/pad:none');
+-- without padding, input not multiple of block size
+SELECT encrypt(
+'\x00112233445566778899aabbccddeeff00',
+'\x000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f',
+'aes-cbc/pad:none');
+
-- key padding
SELECT encrypt(
@@ -50,6 +56,12 @@
-- decrypt
select encode(decrypt(encrypt('foo', '0123456', 'aes'), '0123456', 'aes'),
'escape');
+-- data not multiple of block size
+select encode(decrypt(encrypt('foo', '0123456', 'aes') || '\x00'::bytea,
'0123456', 'aes'), 'escape');
+-- bad padding
+-- (The input value is the result of encrypt_iv('abcdefghijklmnopqrstuvwxyz',
'0123456', 'abcd', 'aes')
+-- with the 16th byte changed (s/db/eb/) to corrupt the padding of the last
block.)
+select
encode(decrypt_iv('\xa21a9c15231465964e3396d32095e67eb52bab05f556a581621dee1b85385789',
'0123456', 'abcd', 'aes'), 'escape');
-- iv
select encrypt_iv('foo', '0123456', 'abcd', 'aes');
--
2.35.1
