Re: OpenSSL 3.0.0 compatibility

> On 3 Jul 2021, at 17:00, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> 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.

That does make a lot of sense, following the code and API docs I concur with
your findings.

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

Skimming the code I wasn't able to find something off the cuff, but there has
been work done to postpone/move padding for constant time operations so maybe
it's related to that.

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

In earlier versions, _Final also closed the context to ensure nothing can leak
from there, but I'm not sure if 1.0.1 constitutes as earlier. Still calling it
from the finish function seems like a good idea though.

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

While the PX cleanup is nice, since we can't get rid of all the padding we
simply shift the complexity to another place where I'd be wary of introducing
bugs into quite stable code.

> (At least, with the patch we can confirm that the OpenSSL padding works consistently with our own implementation.)

+1

> So I think your proposed patch is sound and a good short-term and low-risk solution

The attached 0001 disables the padding. I've tested this with OpenSSL 1.0.1,
1.0.2, 1.1.1 and Git HEAD at e278127cbfa2709d.

Another aspect of OpenSSL 3 compatibility is that of legacy cipher support, and
as we concluded upthread it's best to leave that to the user to define in
openssl.cnf. The attached 0002 adds alternative output files for 3.0.0
installations without the legacy provider loaded, as well as adds a note in the
pgcrypto docs to enable it in case DES is needed. It does annoy me a bit that
we don't load the openssl.cnf file for 1.0.1 if we start mentioning it in the
docs for other versions, but it's probably not worth the effort to fix it given
the lack of complaints so far (it needs a call to OPENSSL_config(NULL); guarded
to HAVE_ macros for 1.0.1).

--
Daniel Gustafsson https://vmware.com/