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.
