From c96b5508019670869ab95fff9e27f051f1546038 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Tue, 24 Jan 2017 23:04:04 +1300 Subject: [PATCH] Polishing updates from audit --- src/security/PeerOptions.cc | 3 ++- src/security/Session.cc | 19 ++++++++++++++----- src/security/Session.h | 7 +++++++ src/security/forward.h | 11 ++++++++--- src/ssl/support.cc | 6 +----- src/tests/stub_libsecurity.cc | 5 ++++- 6 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc index 16f20845ed..cb0778f31b 100644 --- a/src/security/PeerOptions.cc +++ b/src/security/PeerOptions.cc @@ -731,8 +731,9 @@ Security::PeerOptions::updateContextCrl(Security::ContextPointer &ctx) void Security::PeerOptions::updateSessionOptions(Security::SessionPointer &s) { +#if USE_OPENSSL // 'options=' value being set to session is a GnuTLS specific thing. -#if !USE_OPENSSL && USE_GNUTLS +#elif USE_GNUTLS int x; SBuf errMsg; if (!parsedOptions) { diff --git a/src/security/Session.cc b/src/security/Session.cc index b5f2a6d905..f9083f6b92 100644 --- a/src/security/Session.cc +++ b/src/security/Session.cc @@ -87,6 +87,19 @@ tls_write_method(int fd, const char *buf, int len) } #endif +#if USE_OPENSSL +Security::SessionPointer +Security::NewSessionObject(const Security::ContextPointer &ctx) +{ + Security::SessionPointer session(SSL_new(ctx.get()), [](SSL *p) { + debugs(83, 5, "SSL_free session=" << (void*)p); + SSL_free(p); + }); + debugs(83, 5, "SSL_new session=" << (void*)session.get()); + return session; +} +#endif + static bool CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &conn, Security::Io::Type type, const char *squidCtx) { @@ -100,11 +113,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer const char *errAction = "with no TLS/SSL library"; int errCode = 0; #if USE_OPENSSL - Security::SessionPointer session(SSL_new(ctx.get()), [](SSL *p) { - debugs(83, 5, "SSL_free session=" << (void*)p); - SSL_free(p); - }); - debugs(83, 5, "SSL_new session=" << (void*)session.get()); + Security::SessionPointer session(Security::NewSessionObject(ctx)); if (!session) { errCode = ERR_get_error(); errAction = "failed to allocate handle"; diff --git a/src/security/Session.h b/src/security/Session.h index 6574743679..8c40efdb48 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -77,6 +77,13 @@ void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::Sessi /// Needs to be done before using the SessionPointer for a handshake. void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &); +#if USE_OPENSSL +/// \deprecated use the PeerOptions/ServerOptions API methods instead. +/// Wraps SessionPointer value creation to reduce risk of +/// a nasty hack in ssl/support.cc. +Security::SessionPointer NewSessionObject(const Security::ContextPointer &); +#endif + } // namespace Security #endif /* SQUID_SRC_SECURITY_SESSION_H */ diff --git a/src/security/forward.h b/src/security/forward.h index 844611a2d2..e9866f6e9c 100644 --- a/src/security/forward.h +++ b/src/security/forward.h @@ -111,7 +111,10 @@ typedef std::unordered_set Errors; namespace Io { enum Type { -#if USE_GNUTLS +#if USE_OPENSSL + BIO_TO_CLIENT = 6000, + BIO_TO_SERVER +#elif USE_GNUTLS // NP: this is odd looking but correct. // 'to-client' means we are a server, and vice versa. BIO_TO_CLIENT = GNUTLS_SERVER, @@ -126,10 +129,12 @@ namespace Io class KeyData; -#if !USE_OPENSSL && USE_GNUTLS +#if USE_OPENSSL +typedef long ParsedOptions; +#elif USE_GNUTLS typedef std::shared_ptr ParsedOptions; #else -typedef long ParsedOptions; +class ParsedOptions {}; // we never parse/use TLS options in this case #endif class PeerConnector; diff --git a/src/ssl/support.cc b/src/ssl/support.cc index d726c4f30e..dea8df5ab0 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -1020,11 +1020,7 @@ Ssl::verifySslCertificate(Security::ContextPointer &ctx, CertificateProperties c assert(0); #else // Temporary ssl for getting X509 certificate from SSL_CTX. - Security::SessionPointer ssl(SSL_new(ctx.get()), [](SSL *p) { - debugs(83, 5, "SSL_free session=" << (void*)p); - SSL_free(p); - }); - debugs(83, 5, "SSL_new session=" << (void*)ssl.get()); + Security::SessionPointer ssl(Security::NewSessionObject(ctx)); X509 * cert = SSL_get_certificate(ssl.get()); #endif if (!cert) diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index a9f8f4f1e2..28ee94ca83 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -69,7 +69,7 @@ void PeerConnector::recordNegotiationDetails() STUB #include "security/PeerOptions.h" Security::PeerOptions Security::ProxyOutgoingConfig; Security::PeerOptions::PeerOptions() { -#if !USE_GNUTLS +#if USE_OPENSSL parsedOptions = 0; #endif STUB_NOP @@ -103,5 +103,8 @@ void SessionSendGoodbye(const Security::SessionPointer &) STUB bool SessionIsResumed(const Security::SessionPointer &) STUB_RETVAL(false) void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &) STUB +#if USE_OPENSSL +Security::SessionPointer NewSessionObject(const Security::ContextPointer &) STUB_RETVAL(nullptr) +#endif } // namespace Security -- 2.47.2