Horton

Thread

Commits

  1. pgcrypto: Remove internal padding implementation

  1. pgcrypto: Remove internal padding implementation

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-02-14T09:42:45Z

    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

  2. Re: pgcrypto: Remove internal padding implementation

    Jacob Champion <pchampion@vmware.com> — 2022-02-14T23:07:38Z

    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

  3. Re: pgcrypto: Remove internal padding implementation

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-02-21T10:42:07Z

    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?

  4. Re: pgcrypto: Remove internal padding implementation

    Jacob Champion <pchampion@vmware.com> — 2022-02-23T00:53:53Z

    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

  5. Re: pgcrypto: Remove internal padding implementation

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-16T10:12:06Z

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

  6. Re: pgcrypto: Remove internal padding implementation

    Nathan Bossart <nathandbossart@gmail.com> — 2022-03-21T23:51:41Z

    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

  7. Re: pgcrypto: Remove internal padding implementation

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-22T08:01:09Z

    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