> On 19 Sep 2020, at 04:11, Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Sep 18, 2020 at 04:11:13PM +0200, Daniel Gustafsson wrote:
>> The other problem was that the cipher context
>> padding must be explicitly set, whereas in previous versions relying on the
>> default worked fine. EVP_CIPHER_CTX_set_padding always returns 1 so thats why
>> it isn't checking the returnvalue as the other nearby initialization calls.
>
> It seems to me that it would be a good idea to still check for the
> return value of EVP_CIPHER_CTX_set_padding() and just return with
> a PXE_CIPHER_INIT. By looking at the upstream code, it is true that
> it always returns true for <= 1.1.1, but that's not the case for
> 3.0.0. Some code paths of upstream also check after it.
Thats a good point, it's now provider dependent and can thus fail in case the
provider isn't supplying the functionality. We've already loaded a provider
where we know the call is supported, but it's of course better to check. Fixed
in the attached v2.
I was only reading the docs and not the code, and they haven't been updated to
reflect this. I'll open a PR to the OpenSSL devs to fix that.
> Also, what's the impact with disabling the padding for <= 1.1.1? This
> part of the upstream code is still a bit obscure to me..
I'm far from fluent in OpenSSL, but AFAICT it has to do with the new provider
API. The default value for padding is unchanged, but it needs to be propaged
into the provider to be set in the context where the old code picked it up
automatically. The relevant OpenSSL commit (83f68df32f0067ee7b0) which changes
the docs to say the function should be called doesn't contain enough
information to explain why.
>> To avoid problems with the by LibreSSL overloaded OPENSSL_VERSION_NUMBER macro
>> (which too is deprecated in 3.0.0), I used the new macro which is only set in
>> 3.0.0. Not sure if that's considered acceptable or if we should invent our own
>> version macro in autoconf.
>
> OSSL_PROVIDER_load() is new as of 3.0.0, so using a configure switch
> similarly as what we do for the other functions should be more
> consistent and enough, no?
Good point, fixed in the attached.
>> For the main SSL tests, the incorrect password test has a new errormessage
>> which is added in 0002.
>
> Hmm. I am linking to a build of alpha6 here, but I still see the
> error being reported as a bad decrypt for this test. Interesting.
Turns out it was coming from when I tested against OpenSSL git HEAD, so it may
come in alpha7 (or whatever the next version will be). Let's disregard this
for now until dust settles, I've dropped the patch from the series.
cheers ./daniel
