Support "SSL_CTX_set1_curves_list"

[libressl]

Support "SSL_CTX_set1_curves_list"

[4a6f656c]

LibreSSL now has SSL{,_CTX}_set1_groups{,_list}() functions (and defines for SSL{,_CTX}_set1_curves{,_list}.

[busterb]

LibreSSL is currently API compatible with OpenSSL 1.0.1 (which it was forked from) with some new extensions and backported features.

SSL_CTRL_SET_CURVES was added in 2012, but only first appeared in OpenSSL 1.0.2 in 2015. We can certainly look at porting in this API, but does this mean that QT 5.5.0-beta also intends to be incompatible with OpenSSL <= 1.0.1 ?

[heirecka]

Their configure test claims >=0.9.7 is required. After looking at the code it appears they forgot to update that because some ifdefs for 0.9.7 and openssl 0.9.8f were removed with 5.5.0-beta. Nevertheless 1.0.1 should work but LibreSSL reports 0x20000000L whereas the failing bit is behing

#if OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(OPENSSL_NO_EC)

After looking at the history of opensslv.h it appears to me that using the version isn't considered a sane way to detect the presence of a feature. If you could provide a hint how to detect this instead, I would be grateful and provide qt with a patch.

[busterb]

For just the code around SSL_CTRL_SET_CURVES, I might suggest replacing with #if defined(SSL_CTRL_SET_CURVES). However, there are more complications when I looked at the rest of the QT ssl code.

LibreSSL is also missing EC_curve_nist2nid, but QT dynamically loads this function itself at runtime using its own internal definition. Thus, it does not fail at compile time, even though it is not defined in the LibreSSL headers. However, code like the following would then fail at runtime:

#if OPENSSL_VERSION_NUMBER >= 0x10002000L
    if (nid == 0 && q_SSLeay() >= 0x10002000L)
        nid = q_EC_curve_nist2nid(curveNameLatin1.data());
#endif // OPENSSL_VERSION_NUMBER >= 0x10002000L

If this were an autoconf project, I would recommend using AC_CHECK_FUNC/AC_SEARCH_LIBS to check for the function instead, but I'm not sure if you can do that with qmake's configure. It's possible something like this would work, where the function is only used if it actually loads at runtime:

if (q_EC_curve_nist2nid) {
  if (nid == 0) {
    nid = q_EC_curve_nist2nid(curveNameLatin1.data());
}

EC_curve_nist2nid doesn't look like a lot of trouble to backport at any rate. I am concerned that if we pick and choose some APIs from OpenSSL 1.0.2, we may create silent issues in upstream software, where the presence of one feature but not another was never tested.

[hasufell]

afais EC_curve_nist2nid is part of libressl now according to https://marc.info/?l=openbsd-announce&m=143635991232240

so the only problem seems the if-conditional which can easily be fixed with #if defined(SSL_CTRL_SET_CURVES)

[jeremyhu]

Should this be closed as behaving correctly?

[grayed]

I'm building Qt 5.5.1 on OpenBSD right now, with "defined(SSL_CTRL_SET_CURVES)" tweak added. I'll report if something will break at run-time when it'll be possible to test, so, please, don't close this issue for now.

[leonklingele]

@4a6f656c OpenSSL also defines SSL_CTRL_SET_CURVES_LIST which is checked for by e.g. nginx: https://github.com/nginx/nginx/blob/master/src/event/ngx_event_openssl.c#L1075
That is needed as well.

[4a6f656c]

@leonklingele it would be far better for things to check on the availability of the function/feature, or at least condition on the thing that the code calls rather than the CTRL define. That said, we've just added these, even though they are unused due to the implementation.

[zavorka]

@4a6f656c agree, bbf43a1 broke qtnetwork-5.7.1 which invokes those macros when they are defined

[v4hn]

@zavorka is right, I just hit this problem too. removing the doesnt_exist defines fixes the build.

[4a6f656c]

@zavorka @v4hn - thanks, the OpenBSD ports developers discovered the same issue with Qt. The problem here is that they do not use the macros, rather than decided to call SSL_ctrl() directly. Removing the doesnt_exist does fix the compilation issue, but it still will not use the functionality.

I've just committed a change that replaces the defines with actual values that are wired into the control interface. We'd hoped to remove this entirely, however it seems like we're stuck supporting it for now.