Fix LibreSSL DH opaque struct access and X448 curve parsing - #5461
#5461Fix LibreSSL DH opaque struct access and X448 curve parsing#5461blackandwhitebg wants to merge 3 commits into
Conversation
|
@blackandwhitebg please rebase PR on latest main. |
matejk
left a comment
There was a problem hiding this comment.
Thanks for chasing this down -- I reproduced both problems against LibreSSL 4.3.2 on Ubuntu 26.04 and macOS, and both are real:
mainfails to compile with 8incomplete type 'DH'errors, because#if !defined(LIBRESSL_VERSION_NUMBER)sends every LibreSSL version down thedh->ppath even thoughDHhas been opaque since LibreSSL 2.7.0.SSL_CTX_set1_curves_list()returns 0 for any list containingX448on LibreSSL, so everyContextthrows. Confirmed with a direct probe.
The DH_free() calls on the NotImplementedException paths and checking the DH_set0_pqg() return value are good catches -- main leaks p and g today because it ignores that return value entirely.
Two structural requests before this goes in.
1. A LibreSSL version macro instead of raw hex. Crypto.h already has POCO_OPENSSL_VERSION_PREREQ and asks callers not to compare OPENSSL_VERSION_NUMBER directly; the same should apply to LibreSSL. LIBRESSL_VERSION_NUMBER uses the same 0xMNNFFPPS encoding, so it is the same shape. Please add to Crypto/include/Poco/Crypto/Crypto.h, next to the existing macro:
#ifndef POCO_LIBRESSL_VERSION_PREREQ
/// Check for LibreSSL >= major.minor.fix using LIBRESSL_VERSION_NUMBER,
/// which uses the same 0xMNNFFPPS encoding as OPENSSL_VERSION_NUMBER.
///
/// Evaluates to 0 when not building against LibreSSL, so it needs no
/// #if defined(LIBRESSL_VERSION_NUMBER) guard around it.
#if defined(LIBRESSL_VERSION_NUMBER)
#define POCO_LIBRESSL_VERSION_PREREQ(maj, min, fix) \
(LIBRESSL_VERSION_NUMBER >= (((maj) << 28) | ((min) << 20) | ((fix) << 12)))
#else
#define POCO_LIBRESSL_VERSION_PREREQ(maj, min, fix) 0
#endif
#endif2. Drop pre-2.7.0 LibreSSL rather than keeping a legacy branch. LibreSSL 2.7.0 is from 2018, and Poco already requires OpenSSL >= 1.1.1. Setting a minimum the same way, right after the OpenSSL one:
#if defined(LIBRESSL_VERSION_NUMBER) && !POCO_LIBRESSL_VERSION_PREREQ(2, 7, 0)
#error "LibreSSL version too old. At least LibreSSL 2.7.0 is required."
#endifThis is needed as a separate check because LibreSSL hardcodes OPENSSL_VERSION_NUMBER to 0x20000000L for every release, so the existing OpenSSL 1.1.1 gate passes on all LibreSSL versions and can never say anything about which one is in use. Without the LibreSSL minimum, a 2.6 build sails past that gate and then fails deep in Context.cpp with the incomplete type 'DH' errors this PR is fixing.
It also lets the whole #if/#else split in initDH() go away -- the DH_set0_pqg() path becomes unconditional, the last direct struct access leaves the file, and anyone on ancient LibreSSL gets one clear message instead of eight compile errors.
Also, the branch still partly reverts #5460: the merge restored the FIPS block in initDH(), but initECDH() still has its FIPS branch removed, and the merge moved the OPENSSL_NO_DH guard in a way that changes behaviour. Details inline.
Note that #5464 needs to land first: it fixes a mis-nested #else in isFIPSEnabled() that leaves the function with no return on the LibreSSL path. With the above applied on top of #5464, LibreSSL 4.3.2 and OpenSSL 3 both build clean under -Werror=return-type and Crypto, JWT and 65/66 NetSSL tests pass. The one failure is TCPServerTest::testReuseSessionTLS13, a LibreSSL TLS 1.3 session resumption difference unrelated to this PR.
I have also opened #5465, a macOS LibreSSL CI job, so this cannot regress again. It will be merged after this one.
A rebase on main would make this easier to read than the merge commit, but that is up to you.
| { | ||
| #ifndef OPENSSL_NO_DH | ||
|
|
||
| #if POCO_OPENSSL_VERSION_PREREQ(3, 0, 0) |
There was a problem hiding this comment.
The merge moved #ifndef OPENSSL_NO_DH below this block, so on an OPENSSL_NO_DH build the FIPS path now runs and returns early, and the throw SSLContextException("Implementation does not support DH") further down is never reached -- a non-empty dhParamsFile is silently accepted and ignored. The guard needs to stay outermost.
| #if POCO_OPENSSL_VERSION_PREREQ(3, 0, 0) | |
| #ifndef OPENSSL_NO_DH | |
| #if POCO_OPENSSL_VERSION_PREREQ(3, 0, 0) |
| #ifndef OPENSSL_NO_DH | ||
|
|
There was a problem hiding this comment.
Paired with the above -- remove these two lines. Line 635 is also trailing whitespace (git diff --check flags it).
| #ifndef OPENSSL_NO_DH | |
| } | ||
|
|
||
| if (1 != EVP_PKEY_fromdata(pKeyCtx, &pKey, EVP_PKEY_KEY_PARAMETERS, params)) | ||
| if (1 != EVP_PKEY_fromdata(pKeyCtx, &pKey, EVP_PKEY_KEYPAIR, params)) |
There was a problem hiding this comment.
This reverts part of #5460 and is unrelated to LibreSSL -- the whole POCO_OPENSSL_VERSION_PREREQ(3, 0, 0) block is unreachable there, since LibreSSL pins OPENSSL_VERSION_NUMBER to 0x20000000L. Please keep EVP_PKEY_KEY_PARAMETERS; only parameters are being imported here.
| if (1 != EVP_PKEY_fromdata(pKeyCtx, &pKey, EVP_PKEY_KEYPAIR, params)) | |
| if (1 != EVP_PKEY_fromdata(pKeyCtx, &pKey, EVP_PKEY_KEY_PARAMETERS, params)) |
| } | ||
|
|
||
| #if !defined(LIBRESSL_VERSION_NUMBER) | ||
| #if !defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER >= 0x2070000fL |
There was a problem hiding this comment.
This is where the POCO_LIBRESSL_VERSION_PREREQ macro and the 2.7.0 minimum from the summary come in: with the minimum enforced in Crypto.h, this #if and the #else // Legacy LibreSSL (< 2.7.0) branch below it can both go, leaving one unconditional path down to the #endif on line 943.
While rewriting the block, the comment on lines 904-905 duplicates itself ("Verify allocations AND verification that") and the second line restates the code. One line covers the non-obvious part:
// DH_set0_pqg() only takes ownership of p and g when it succeeds.| #if defined(LIBRESSL_VERSION_NUMBER) | ||
| // LibreSSL does not support X448 in its curve parser | ||
| const std::string groups(curve.empty() ? "X25519:P-256:P-384:P-521" : curve); | ||
| #else | ||
| const std::string groups(curve.empty() ? "X448:X25519:P-521:P-384:P-256" : curve); | ||
| #endif |
There was a problem hiding this comment.
This still drops the FIPS branch added in #5460, so FIPS builds go back to advertising X25519/X448. The LibreSSL guard only needs to change the non-FIPS default.
Also, the order is inverted relative to the OpenSSL branch. I checked that LibreSSL accepts the list in either order, so keep it strongest-first for consistency.
This one is a vendor check rather than a version check, so plain defined(LIBRESSL_VERSION_NUMBER) is right here -- no need for the new macro.
| #if defined(LIBRESSL_VERSION_NUMBER) | |
| // LibreSSL does not support X448 in its curve parser | |
| const std::string groups(curve.empty() ? "X25519:P-256:P-384:P-521" : curve); | |
| #else | |
| const std::string groups(curve.empty() ? "X448:X25519:P-521:P-384:P-256" : curve); | |
| #endif | |
| #if defined(LIBRESSL_VERSION_NUMBER) | |
| // LibreSSL's curve list parser does not know X448. | |
| const char* defaultGroups = "X25519:P-521:P-384:P-256"; | |
| #else | |
| const char* defaultGroups = "X448:X25519:P-521:P-384:P-256"; | |
| #endif | |
| const std::string groups(curve.empty() | |
| ? (SSLManager::isFIPSEnabled() | |
| ? "P-521:P-384:P-256" // FIPS 140-2 + 140-3 safe | |
| : defaultGroups) | |
| : curve); |
Summary
This PR resolves two LibreSSL compatibility issues in
Poco::Net::Context:dh->panddh->gstruct member access with accessor functions (DH_set0_pqg,DH_set_length) for modern LibreSSL releases (2.7.0+), resolving opaque struct compilation errors (C2027).X448from the default ECDH curve list duringinitECDH()when compiling against LibreSSL. LibreSSL does not supportX448in its curve parser, which previously causedSSL_CTX_set1_curves_list()to fail and throw anSSLContextException.Testing Environment
main