pgcrypto: Remove internal padding implementation

On Mon, 2022-02-14 at 10:42 +0100, Peter Eisentraut wrote:

This is a rebase of the patch from [0]. It removes the internal padding
implementation in pgcrypto and lets OpenSSL do it. The internal
implementation was once applicable to the non-OpenSSL code paths, but
those have since been removed.

These removed parts looked interesting to me:

- else if (bpos % bs)
- {
- /* ERROR? */
- pad = bs - (bpos % bs);
- for (i = 0; i < pad; i++)
- bbuf[bpos++] = 0;
- }

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

After this patch, bad padding is no longer ignored during decryption,
and encryption without padding now requires the input size to be a
multiple of the block size. To see the difference you can try the
following queries with and without the patch:

select encrypt_iv('foo', '0123456', 'abcd', 'aes/pad:none');
select encode(decrypt_iv('\xa21a9c15231465964e3396d32095e67eb52bab05f556a581621dee1b85385789', '0123456', 'abcd', 'aes'), 'escape');

Both changes seem correct to me. I can imagine some system out there
being somehow dependent on the prior decryption behavior to avoid a
padding oracle -- but if that's a concern, hopefully you're not using
unauthenticated encryption in the first place? It might be worth a note
in the documentation.

--Jacob

On 15.02.22 00:07, Jacob Champion wrote:

After this patch, bad padding is no longer ignored during decryption,
and encryption without padding now requires the input size to be a
multiple of the block size. To see the difference you can try the
following queries with and without the patch:

select encrypt_iv('foo', '0123456', 'abcd', 'aes/pad:none');
select encode(decrypt_iv('\xa21a9c15231465964e3396d32095e67eb52bab05f556a581621dee1b85385789', '0123456', 'abcd', 'aes'), 'escape');

Interesting. I have added test cases about this. Could you describe
how you arrived at the second test case?

Both changes seem correct to me. I can imagine some system out there
being somehow dependent on the prior decryption behavior to avoid a
padding oracle -- but if that's a concern, hopefully you're not using
unauthenticated encryption in the first place? It might be worth a note
in the documentation.

Hmm. I started reading up on "padding oracle attack". I don't
understand it well enough yet to know whether this is a concern here.

Is there any reasonable meaning of the previous behaviors? Does bad
padding even give correct answers on decryption? What does encryption
without padding even do on incorrect input sizes?

On Mon, 2022-02-21 at 11:42 +0100, Peter Eisentraut wrote:

On 15.02.22 00:07, Jacob Champion wrote:

After this patch, bad padding is no longer ignored during decryption,
and encryption without padding now requires the input size to be a
multiple of the block size. To see the difference you can try the
following queries with and without the patch:

select encrypt_iv('foo', '0123456', 'abcd', 'aes/pad:none');
select encode(decrypt_iv('\xa21a9c15231465964e3396d32095e67eb52bab05f556a581621dee1b85385789', '0123456', 'abcd', 'aes'), 'escape');

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.

Both changes seem correct to me. I can imagine some system out there
being somehow dependent on the prior decryption behavior to avoid a
padding oracle -- but if that's a concern, hopefully you're not using
unauthenticated encryption in the first place? It might be worth a note
in the documentation.

Hmm. I started reading up on "padding oracle attack". I don't
understand it well enough yet to know whether this is a concern here.

I *think* the only reasonable way to prevent that attack is by
authenticating with a MAC or an authenticated cipher, to avoid running
decryption on corrupted ciphertext in the first place. But I'm out of
my expertise here.

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

--Jacob

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.

On Wed, Mar 16, 2022 at 11:12:06AM +0100, Peter Eisentraut wrote:

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.

I took a look at this patch and found nothing of concern.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 22.03.22 00:51, Nathan Bossart wrote:

On Wed, Mar 16, 2022 at 11:12:06AM +0100, Peter Eisentraut wrote:

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.

I took a look at this patch and found nothing of concern.

committed