Thread: pgcrypto: Remove internal padding implementation
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. [0]: https://www.postgresql.org/message-id/b1a62889-bb45-e5e0-d138-7a370a0a334f@enterprisedb.com
Attachment
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?
Attachment
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.
Attachment
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