Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0591829
Update license disclaimer
yadij Jul 23, 2020
4dd37f3
Declaration of CRYPTO_EX_dup changed again in 3.0
yadij Jul 23, 2020
5564eb4
SSL_OP_* macro definitions changed in 3.0
yadij Oct 9, 2021
b787e70
Refactor Ssl::createSslPrivateKey()
yadij Jul 23, 2020
e290d6c
Switch to BN_rand()
yadij Oct 6, 2021
b98cd0f
TODO Upgrade API calls verifying loaded DH params file
yadij Jul 23, 2020
34d6c44
Initial DH conversion to EVP_PKEY
yadij Oct 6, 2021
be2c0e8
Update ECDH key settings
yadij Oct 10, 2021
743fa75
Detect and default-enable OpenSSL 3+
yadij Oct 14, 2021
a9e367d
Remove stale TODO and comment
yadij Oct 14, 2021
df2d57e
Update release notes for UI changes related to OpenSSL 3.0
yadij Oct 14, 2021
cc0ab60
Revert auto-enable for now
yadij May 21, 2022
fe02c2b
Fix DH/EC parameter loading
yadij May 21, 2022
6834979
Update src/cf.data.pre
yadij May 28, 2022
add1846
Reviewer requested performance regression
yadij Jun 4, 2022
55d159c
Reviewer requested type polish
yadij Jun 7, 2022
b993bce
Update src/security/ServerOptions.cc
yadij Jun 8, 2022
27bba66
log fopen() errors
yadij Jun 9, 2022
929cfba
Reviewer requested updates
yadij Jun 10, 2022
fd264d0
fixup: The branch code was asserting when decoding tls-dh
rousskov Jun 12, 2022
434a238
fixup: Do not reset errno before syscalls
rousskov Jun 12, 2022
9e9547d
fixup: Avoid rawCtx pointer by adjusting dctx declaration
rousskov Jun 12, 2022
5d2e616
Restored key parameters validation parity with OpenSSL v1 builds
rousskov Jun 12, 2022
ee3948c
fixup: Restored tls-dh=curve:... support parity with OpenSSL v1
rousskov Jun 12, 2022
dee21be
fixup: Minor polishing and diff reduction for recently changed code
rousskov Jun 12, 2022
ed96b56
Try to support ENGINE's
yadij Jun 16, 2022
743ea80
Restore TODO that has not been resolved yet
yadij Jun 17, 2022
955b86f
Revert ParsedOptions type in default case
yadij Jun 22, 2022
aef550c
Distinguish unknown vs unsupported OpenSSL options
yadij Jun 22, 2022
53e3404
Revert a2f0e1b
yadij Jun 22, 2022
754c2d1
Update src/ssl/gadgets.cc
yadij Jul 4, 2022
599d44a
Update src/ssl/gadgets.cc
yadij Jul 4, 2022
6781d41
Update release-6.sgml
yadij Jul 4, 2022
cb99ce7
fixup: Avoid returning raw pointers from object creators
rousskov Jul 4, 2022
7badc4c
Warn admins using ssl_engine about that feature deprecation
rousskov Jul 5, 2022
940efa1
Update doc/release-notes/release-6.sgml
yadij Jul 16, 2022
d4a42e7
Switch touched typedef to using syntax
yadij Jul 16, 2022
ae08044
Merge branch 'master' into openssl-3.0
yadij Jul 16, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -1283,6 +1283,7 @@ if test "x$with_openssl" = "xyes"; then
openssl/bio.h \
openssl/bn.h \
openssl/crypto.h \
openssl/decoder.h \
openssl/dh.h \
openssl/err.h \
openssl/evp.h \
Expand Down
4 changes: 3 additions & 1 deletion doc/release-notes/release-6.sgml
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ This section gives an account of those changes in three categories:
<sect1>Changes to existing directives<label id="modifieddirectives">
<p>
<descrip>
<p>There have been no directives changed.
<tag>ssl_engine</tag>
<p>OpenSSL 3.0.0 deprecates the Engine feature. This directive is
only supported when Squid is built for older OpenSSL versions.

</descrip>

Expand Down
2 changes: 2 additions & 0 deletions src/cf.data.pre
Original file line number Diff line number Diff line change
Expand Up @@ -3061,6 +3061,8 @@ DEFAULT: none
DOC_START
The OpenSSL engine to use. You will need to set this if you
would like to use hardware SSL acceleration for example.

Not supported in builds with OpenSSL v3 or newer.
DOC_END

NAME: sslproxy_session_ttl
Expand Down
2 changes: 2 additions & 0 deletions src/main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,9 @@ mainHandleCommandLineOption(const int optId, const char *optValue)
printf("%s\n",SQUID_BUILD_INFO);
#if USE_OPENSSL
printf("\nThis binary uses %s. ", OpenSSL_version(OPENSSL_VERSION));
#if OPENSSL_VERSION_MAJOR < 3
printf("For legal restrictions on distribution see https://www.openssl.org/source/license.html\n\n");
#endif
#endif
printf( "configure options: %s\n", SQUID_CONFIGURE_OPTIONS);

Expand Down
66 changes: 35 additions & 31 deletions src/security/PeerOptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -293,134 +293,134 @@ Security::PeerOptions::createClientContext(bool setOptions)
/// set of options we can parse and what they map to
static struct ssl_option {
const char *name;
long value;
Security::ParsedOptions value;

} ssl_options[] = {

#if SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG
#if defined(SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG)
{
"NETSCAPE_REUSE_CIPHER_CHANGE_BUG", SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG
},
#endif
#if SSL_OP_SSLREF2_REUSE_CERT_TYPE_BUG
#if defined(SSL_OP_SSLREF2_REUSE_CERT_TYPE_BUG)
{
"SSLREF2_REUSE_CERT_TYPE_BUG", SSL_OP_SSLREF2_REUSE_CERT_TYPE_BUG
},
#endif
#if SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER
#if defined(SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER)
{
"MICROSOFT_BIG_SSLV3_BUFFER", SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER
},
#endif
#if SSL_OP_SSLEAY_080_CLIENT_DH_BUG
#if defined(SSL_OP_SSLEAY_080_CLIENT_DH_BUG)
{
"SSLEAY_080_CLIENT_DH_BUG", SSL_OP_SSLEAY_080_CLIENT_DH_BUG
},
#endif
#if SSL_OP_TLS_D5_BUG
#if defined(SSL_OP_TLS_D5_BUG)
{
"TLS_D5_BUG", SSL_OP_TLS_D5_BUG
},
#endif
#if SSL_OP_TLS_BLOCK_PADDING_BUG
#if defined(SSL_OP_TLS_BLOCK_PADDING_BUG)
{
"TLS_BLOCK_PADDING_BUG", SSL_OP_TLS_BLOCK_PADDING_BUG
},
#endif
#if SSL_OP_TLS_ROLLBACK_BUG
#if defined(SSL_OP_TLS_ROLLBACK_BUG)
{
"TLS_ROLLBACK_BUG", SSL_OP_TLS_ROLLBACK_BUG
},
#endif
#if SSL_OP_ALL
#if defined(SSL_OP_ALL)
{
"ALL", (long)SSL_OP_ALL
},
#endif
#if SSL_OP_SINGLE_DH_USE
#if defined(SSL_OP_SINGLE_DH_USE)
{
"SINGLE_DH_USE", SSL_OP_SINGLE_DH_USE
},
#endif
#if SSL_OP_EPHEMERAL_RSA
#if defined(SSL_OP_EPHEMERAL_RSA)
{
"EPHEMERAL_RSA", SSL_OP_EPHEMERAL_RSA
},
#endif
#if SSL_OP_PKCS1_CHECK_1
#if defined(SSL_OP_PKCS1_CHECK_1)
{
"PKCS1_CHECK_1", SSL_OP_PKCS1_CHECK_1
},
#endif
#if SSL_OP_PKCS1_CHECK_2
#if defined(SSL_OP_PKCS1_CHECK_2)
{
"PKCS1_CHECK_2", SSL_OP_PKCS1_CHECK_2
},
#endif
#if SSL_OP_NETSCAPE_CA_DN_BUG
#if defined(SSL_OP_NETSCAPE_CA_DN_BUG)
{
"NETSCAPE_CA_DN_BUG", SSL_OP_NETSCAPE_CA_DN_BUG
},
#endif
#if SSL_OP_NON_EXPORT_FIRST
#if defined(SSL_OP_NON_EXPORT_FIRST)
{
"NON_EXPORT_FIRST", SSL_OP_NON_EXPORT_FIRST
},
#endif
#if SSL_OP_CIPHER_SERVER_PREFERENCE
#if defined(SSL_OP_CIPHER_SERVER_PREFERENCE)
{
"CIPHER_SERVER_PREFERENCE", SSL_OP_CIPHER_SERVER_PREFERENCE
},
#endif
#if SSL_OP_NETSCAPE_DEMO_CIPHER_CHANGE_BUG
#if defined(SSL_OP_NETSCAPE_DEMO_CIPHER_CHANGE_BUG)
{
"NETSCAPE_DEMO_CIPHER_CHANGE_BUG", SSL_OP_NETSCAPE_DEMO_CIPHER_CHANGE_BUG
},
#endif
#if SSL_OP_NO_SSLv3
#if defined(SSL_OP_NO_SSLv3)
{
"NO_SSLv3", SSL_OP_NO_SSLv3
},
#endif
#if SSL_OP_NO_TLSv1
#if defined(SSL_OP_NO_TLSv1)
{
"NO_TLSv1", SSL_OP_NO_TLSv1
},
#else
{ "NO_TLSv1", 0 },
#endif
#if SSL_OP_NO_TLSv1_1
#if defined(SSL_OP_NO_TLSv1_1)
{
"NO_TLSv1_1", SSL_OP_NO_TLSv1_1
},
#else
{ "NO_TLSv1_1", 0 },
#endif
#if SSL_OP_NO_TLSv1_2
#if defined(SSL_OP_NO_TLSv1_2)
{
"NO_TLSv1_2", SSL_OP_NO_TLSv1_2
},
#else
{ "NO_TLSv1_2", 0 },
#endif
#if SSL_OP_NO_TLSv1_3
#if defined(SSL_OP_NO_TLSv1_3)
{
"NO_TLSv1_3", SSL_OP_NO_TLSv1_3
},
#else
{ "NO_TLSv1_3", 0 },
#endif
#if SSL_OP_NO_COMPRESSION
#if defined(SSL_OP_NO_COMPRESSION)
{
"No_Compression", SSL_OP_NO_COMPRESSION
},
#endif
#if SSL_OP_NO_TICKET
#if defined(SSL_OP_NO_TICKET)
{
"NO_TICKET", SSL_OP_NO_TICKET
},
#endif
#if SSL_OP_SINGLE_ECDH_USE
#if defined(SSL_OP_SINGLE_ECDH_USE)
{
"SINGLE_ECDH_USE", SSL_OP_SINGLE_ECDH_USE
},
Expand Down Expand Up @@ -455,7 +455,7 @@ Security::PeerOptions::parseOptions()

#if USE_OPENSSL
::Parser::Tokenizer tok(str);
long op = 0;
ParsedOptions op = 0;

while (!tok.atEnd()) {
enum {
Expand All @@ -472,7 +472,8 @@ Security::PeerOptions::parseOptions()
static const CharacterSet optChars = CharacterSet("TLS-option", "_") + CharacterSet::ALPHA + CharacterSet::DIGIT;
int64_t hex = 0;
SBuf option;
long value = 0;
ParsedOptions value = 0;
bool found = false;

// Bug 4429: identify the full option name before determining text or numeric
if (tok.prefix(option, optChars)) {
Expand All @@ -481,14 +482,16 @@ Security::PeerOptions::parseOptions()
for (struct ssl_option *opttmp = ssl_options; opttmp->name; ++opttmp) {
if (option.cmp(opttmp->name) == 0) {
value = opttmp->value;
found = true;
break;
}
}

// Special case.. hex specification
::Parser::Tokenizer tmp(option);
if (!value && tmp.int64(hex, 16, false) && tmp.atEnd()) {
if (!found && tmp.int64(hex, 16, false) && tmp.atEnd()) {
value = hex;
found = true;
}
}

Comment on lines 495 to 497
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be using Optional instead of a (value, found) pair. Optional improves code quality in use cases like this. Internally, Optional is essentially a (value, found) pair itself.

I cannot attach a change request to the line immediately below this one, but it is abusing non-zero value for "found". BTW, using Optional (as suggested in an earlier change request IIRC) would likely eliminate that problem (or highlight it).

In the interest of merging this long-neglected PR sooner, I do not insist on this change, but there is no good reason for avoid it AFAICT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I am not against the idea of making this array better and will probably waive through good quality PRs doing it. I really just think it is complexity, not necessary for this PR scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change request is not about the array. It is about the (value, found) pair (which is essentially added by this PR). Addressing this change request will not make the code more complex.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be happy to address this change request, demonstrating that the improved code will remain simple. Just let me know if you want me to do that. I still do not insist on this change.

Expand All @@ -502,7 +505,7 @@ Security::PeerOptions::parseOptions()
break;
}
} else {
debugs(83, DBG_PARSE_NOTE(1), "ERROR: Unknown TLS option " << option);
debugs(83, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: " << (found?"Unsupported":"Unknown") << " TLS option " << option);
}

static const CharacterSet delims("TLS-option-delim",":,");
Expand All @@ -512,9 +515,10 @@ Security::PeerOptions::parseOptions()

}

#if SSL_OP_NO_SSLv2
#if defined(SSL_OP_NO_SSLv2)
// compliance with RFC 6176: Prohibiting Secure Sockets Layer (SSL) Version 2.0
op = op | SSL_OP_NO_SSLv2;
if (SSL_OP_NO_SSLv2)
op |= SSL_OP_NO_SSLv2;
#endif
parsedOptions = op;

Expand Down
82 changes: 82 additions & 0 deletions src/security/ServerOptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "anyp/PortCfg.h"
#include "base/Packable.h"
#include "cache_cf.h"
#include "error/SysErrorDetail.h"
#include "fatal.h"
#include "globals.h"
#include "security/ServerOptions.h"
Expand All @@ -19,6 +20,9 @@
#include "compat/openssl.h"
#include "ssl/support.h"

#if HAVE_OPENSSL_DECODER_H
#include <openssl/decoder.h>
#endif
#if HAVE_OPENSSL_ERR_H
#include <openssl/err.h>
#endif
Expand Down Expand Up @@ -352,11 +356,20 @@ Security::ServerOptions::loadDhParams()
if (dhParamsFile.isEmpty())
return;

// TODO: After loading and validating parameters, also validate that "the
// public and private components have the correct mathematical
// relationship". See EVP_PKEY_check().

#if USE_OPENSSL
#if OPENSSL_VERSION_MAJOR < 3
DH *dhp = nullptr;
if (FILE *in = fopen(dhParamsFile.c_str(), "r")) {
dhp = PEM_read_DHparams(in, nullptr, nullptr, nullptr);
fclose(in);
} else {
const auto xerrno = errno;
debugs(83, DBG_IMPORTANT, "WARNING: Failed to open '" << dhParamsFile << "'" << ReportSysError(xerrno));
return;
}

if (!dhp) {
Expand All @@ -374,7 +387,65 @@ Security::ServerOptions::loadDhParams()
}

parsedDhParams.resetWithoutLocking(dhp);

#else // OpenSSL 3.0+
const auto type = eecdhCurve.isEmpty() ? "DH" : "EC";

Ssl::ForgetErrors();
EVP_PKEY *rawPkey = nullptr;
using DecoderContext = std::unique_ptr<OSSL_DECODER_CTX, HardFun<void, OSSL_DECODER_CTX*, &OSSL_DECODER_CTX_free> >;
if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, 0, nullptr, nullptr)}) {

// OpenSSL documentation is vague on this, but OpenSSL code and our
// tests suggest that rawPkey remains nil here while rawCtx keeps
// rawPkey _address_ for use by the decoder (see OSSL_DECODER_from_fp()
// below). Thus, we must not move *rawPkey into a smart pointer until
// decoding is over. For cleanup code simplicity, we assert nil rawPkey.
assert(!rawPkey);

if (OSSL_DECODER_CTX_get_num_decoders(dctx.get()) == 0) {
debugs(83, DBG_IMPORTANT, "WARNING: No suitable decoders found for " << type << " parameters" << Ssl::ReportAndForgetErrors);
return;
}

if (const auto in = fopen(dhParamsFile.c_str(), "r")) {
if (OSSL_DECODER_from_fp(dctx.get(), in)) {
assert(rawPkey);
const Security::DhePointer pkey(rawPkey);
// TODO: verify that the loaded parameters match the curve named in eecdhCurve

if (const Ssl::EVP_PKEY_CTX_Pointer pkeyCtx{EVP_PKEY_CTX_new_from_pkey(nullptr, pkey.get(), nullptr)}) {
switch (EVP_PKEY_param_check(pkeyCtx.get())) {
case 1: // success
parsedDhParams = pkey;
break;
case -2:
debugs(83, DBG_PARSE_NOTE(2), "WARNING: OpenSSL does not support " << type << " parameters check: " << dhParamsFile << Ssl::ReportAndForgetErrors);
break;
default:
debugs(83, DBG_IMPORTANT, "ERROR: Failed to verify " << type << " parameters in " << dhParamsFile << Ssl::ReportAndForgetErrors);
break;
}
} else {
// TODO: Reduce error reporting code duplication.
debugs(83, DBG_IMPORTANT, "ERROR: Cannot check " << type << " parameters in " << dhParamsFile << Ssl::ReportAndForgetErrors);
}
} else {
debugs(83, DBG_IMPORTANT, "WARNING: Failed to decode " << type << " parameters '" << dhParamsFile << "'" << Ssl::ReportAndForgetErrors);
EVP_PKEY_free(rawPkey); // probably still nil, but just in case
}
fclose(in);
} else {
const auto xerrno = errno;
debugs(83, DBG_IMPORTANT, "WARNING: Failed to open '" << dhParamsFile << "'" << ReportSysError(xerrno));
}

} else {
debugs(83, DBG_IMPORTANT, "WARNING: Unable to create decode context for " << type << " parameters" << Ssl::ReportAndForgetErrors);
return;
}
#endif
#endif // USE_OPENSSL
}

bool
Expand Down Expand Up @@ -454,12 +525,16 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx)
debugs(83, 9, "Setting Ephemeral ECDH curve to " << eecdhCurve << ".");

#if USE_OPENSSL && OPENSSL_VERSION_NUMBER >= 0x0090800fL && !defined(OPENSSL_NO_ECDH)

Ssl::ForgetErrors();

int nid = OBJ_sn2nid(eecdhCurve.c_str());
if (!nid) {
debugs(83, DBG_CRITICAL, "ERROR: Unknown EECDH curve '" << eecdhCurve << "'");
return;
}

#if OPENSSL_VERSION_MAJOR < 3
auto ecdh = EC_KEY_new_by_curve_name(nid);
if (!ecdh) {
const auto x = ERR_get_error();
Expand All @@ -473,6 +548,13 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx)
}
EC_KEY_free(ecdh);

#else
// TODO: Support multiple group names via SSL_CTX_set1_groups_list().
if (!SSL_CTX_set1_groups(ctx.get(), &nid, 1)) {
debugs(83, DBG_CRITICAL, "ERROR: Unable to set Ephemeral ECDH: " << Ssl::ReportAndForgetErrors);
return;
}
#endif
#else
debugs(83, DBG_CRITICAL, "ERROR: EECDH is not available in this build." <<
" Please link against OpenSSL>=0.9.8 and ensure OPENSSL_NO_ECDH is not set.");
Expand Down
Loading