Update to options= after audit

diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc
index a32826e857975d5be5444903a96846689ae7145b..844aa16f8d2838127c087a3bf0bd862d3bce43ba 100644 (file)
--- a/src/security/PeerOptions.cc
+++ b/src/security/PeerOptions.cc
@@ -35,8 +35,7 @@ Security::PeerOptions::PeerOptions(const Security::PeerOptions &p) :
     sslVersion(p.sslVersion),
     encryptTransport(p.encryptTransport)
 {
-    if (!sslOptions.isEmpty())
-        parseOptions(parsedOptions); // re-parse after sslOptions copied.
+    parseOptions(); // re-parse after sslOptions copied.
     memcpy(&flags, &p.flags, sizeof(flags));
 }
 
@@ -44,8 +43,7 @@ Security::PeerOptions &
 Security::PeerOptions::operator =(const Security::PeerOptions &p)
 {
     sslOptions = p.sslOptions;
-    if (!sslOptions.isEmpty())
-        parseOptions(parsedOptions); // re-parse after sslOptions copied.
+    parseOptions(); // re-parse after sslOptions copied.
     caDir = p.caDir;
     crlFile = p.crlFile;
     sslCipher = p.sslCipher;
@@ -93,7 +91,7 @@ Security::PeerOptions::parse(const char *token)
         tlsMinVersion = SBuf(token + 12);
     } else if (strncmp(token, "options=", 8) == 0) {
         sslOptions = SBuf(token + 8);
-        parseOptions(parsedOptions);
+        parseOptions();
     } else if (strncmp(token, "cipher=", 7) == 0) {
         sslCipher = SBuf(token + 7);
     } else if (strncmp(token, "cafile=", 7) == 0) {
@@ -192,15 +190,15 @@ Security::PeerOptions::updateTlsVersionLimits()
 #if USE_OPENSSL
 #if SSL_OP_NO_TLSv1
             if (v > 0)
-                *parsedOptions |= SSL_OP_NO_TLSv1;
+                parsedOptions |= SSL_OP_NO_TLSv1;
 #endif
 #if SSL_OP_NO_TLSv1_1
             if (v > 1)
-                *parsedOptions |= SSL_OP_NO_TLSv1_1;
+                parsedOptions |= SSL_OP_NO_TLSv1_1;
 #endif
 #if SSL_OP_NO_TLSv1_2
             if (v > 2)
-                *parsedOptions |= SSL_OP_NO_TLSv1_2;
+                parsedOptions |= SSL_OP_NO_TLSv1_2;
 #endif
 
 #elif USE_GNUTLS
@@ -230,32 +228,32 @@ Security::PeerOptions::updateTlsVersionLimits()
         // only use if tls-min-version=N.N is not present
         // values 0-2 for auto and SSLv2 are not supported any longer.
         // Do it this way so we DO cause changes to options= in cachemgr config report
-        const char *add = NULL;
+        const char *add = nullptr;
         switch (sslVersion) {
         case 3:
 #if USE_OPENSSL
-            add = ",NO_TLSv1,NO_TLSv1_1,NO_TLSv1_2";
+            parsedOptions |= (SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_1|SSL_OP_NO_TLSv1_2);
 #elif USE_GNUTLS
             add = ":-VERS-TLS1.0:-VERS-TLS1.1:-VERS-TLS1.2";
 #endif
             break;
         case 4:
 #if USE_OPENSSL
-            add = ",NO_SSLv3,NO_TLSv1_1,NO_TLSv1_2";
+            parsedOptions |= (SSL_OP_NO_SSLv3|SSL_OP_NO_TLSv1_1|SSL_OP_NO_TLSv1_2);
 #elif USE_GNUTLS
             add = ":+VERS-TLS1.0:-VERS-TLS1.1:-VERS-TLS1.2";
 #endif
             break;
         case 5:
 #if USE_OPENSSL
-            add = ",NO_SSLv3,NO_TLSv1,NO_TLSv1_2";
+            parsedOptions |= (SSL_OP_NO_SSLv3|SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_2);
 #elif USE_GNUTLS
             add = ":-VERS-TLS1.0:+VERS-TLS1.1:-VERS-TLS1.2";
 #endif
             break;
         case 6:
 #if USE_OPENSSL
-            add = ",NO_SSLv3,NO_TLSv1,NO_TLSv1_1";
+            parsedOptions |= (SSL_OP_NO_SSLv3|SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_1);
 #elif USE_GNUTLS
             add = ":-VERS-TLS1.0:-VERS-TLS1.1";
 #endif
@@ -264,10 +262,12 @@ Security::PeerOptions::updateTlsVersionLimits()
             break;
         }
         if (add) {
+#if USE_GNUTLS // dont bother otherwise
             if (sslOptions.isEmpty())
                 sslOptions.append(add+1, strlen(add+1));
             else
                 sslOptions.append(add, strlen(add));
+#endif
         }
         sslVersion = 0; // prevent sslOptions being repeatedly appended
     }
@@ -316,7 +316,7 @@ Security::PeerOptions::createClientContext(bool setOptions)
     if (t) {
 #if USE_OPENSSL
         // NP: GnuTLS uses 'priorities' which are set per-session instead.
-        SSL_CTX_set_options(t.get(), (setOptions ? *parsedOptions : 0));
+        SSL_CTX_set_options(t.get(), (setOptions ? parsedOptions : 0));
 
         // XXX: temporary performance regression. c_str() data copies and prevents this being a const method
         Ssl::InitClientContext(t, *this, parsedFlags);
@@ -466,7 +466,7 @@ static struct ssl_option {
  * Options must not used in the case of peek or stare bump mode.
  */
 void
-Security::PeerOptions::parseOptions(Security::ParsedOptionsPointer &theOut)
+Security::PeerOptions::parseOptions()
 {
 #if USE_OPENSSL
     ::Parser::Tokenizer tok(sslOptions);
@@ -531,18 +531,21 @@ Security::PeerOptions::parseOptions(Security::ParsedOptionsPointer &theOut)
     // compliance with RFC 6176: Prohibiting Secure Sockets Layer (SSL) Version 2.0
     op = op | SSL_OP_NO_SSLv2;
 #endif
-    theOut.reset(new long(op));
+    parsedOptions = op;
 
 #elif USE_GNUTLS
+    if (sslOptions.isEmpty()) {
+        parsedOptions.reset();
+        return;
+    }
+
     const char *err = nullptr;
-    const char *priorities = (sslOptions.isEmpty() ? nullptr : sslOptions.c_str());
+    const char *priorities = sslOptions.c_str();
     gnutls_priority_t op;
-    int x = gnutls_priority_init(&op, priorities, &err);
-    if (x != GNUTLS_E_SUCCESS) {
+    if (gnutls_priority_init(&op, priorities, &err) != GNUTLS_E_SUCCESS) {
         fatalf("Unknown TLS option '%s'", err);
     }
-    theOut.reset(op);
-
+    parsedOptions.reset(op);
 #endif
 }
 
@@ -723,16 +726,20 @@ Security::PeerOptions::updateSessionOptions(Security::SessionPointer &s)
     // 'options=' value being set to session is a GnuTLS specific thing.
 #if !USE_OPENSSL && USE_GNUTLS
     int x;
+    SBuf errMsg;
     if (!parsedOptions) {
         debugs(83, 5, "set GnuTLS default priority/options for session=" << s);
         x = gnutls_set_default_priority(s.get());
+        static const SBuf defaults("default");
+        errMsg = defaults;
     } else {
         debugs(83, 5, "set GnuTLS options '" << sslOptions << "' for session=" << s);
         x = gnutls_priority_set(s.get(), parsedOptions.get());
+        errMsg = sslOptions;
     }
 
     if (x != GNUTLS_E_SUCCESS) {
-        debugs(83, 1, "Failed to set TLS options. error: " << Security::ErrorString(x));
+        debugs(83, DBG_IMPORTANT, "ERROR: Failed to set TLS options (" << errMsg << "). error: " << Security::ErrorString(x));
     }
 #endif
 }

diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h
index 7b32edd70940d4f119c2d66f1f295c8ab3fac7bc..5c6b89f354a43211a2867e3f55a1b8b9573315e2 100644 (file)
--- a/src/security/PeerOptions.h
+++ b/src/security/PeerOptions.h
@@ -58,7 +58,7 @@ public:
     virtual void dumpCfg(Packable *, const char *pfx) const;
 
 private:
-    void parseOptions(Security::ParsedOptionsPointer &); ///< parsed value of sslOptions
+    void parseOptions(); ///< parsed value of sslOptions
     long parseFlags();
     void loadCrlFile();
 
@@ -73,7 +73,7 @@ public:
 
     SBuf tlsMinVersion;  ///< version label for minimum TLS version to permit
 
-    Security::ParsedOptionsPointer parsedOptions; ///< parsed value of sslOptions
+    Security::ParsedOptions parsedOptions; ///< parsed value of sslOptions
     long parsedFlags = 0;   ///< parsed value of sslFlags
 
     std::list<Security::KeyData> certs; ///< details from the cert= and file= config parameters

diff --git a/src/security/forward.h b/src/security/forward.h
index 9845b522003b8946628bcad331bf85f955f650d5..858c9ccfb5627b89d96420d664438d3c54e2132d 100644 (file)
--- a/src/security/forward.h
+++ b/src/security/forward.h
@@ -127,9 +127,9 @@ namespace Io
 class KeyData;
 
 #if !USE_OPENSSL && USE_GNUTLS
-typedef std::unique_ptr<struct gnutls_priority_st, HardFun<void, gnutls_priority_t, &gnutls_priority_deinit>> ParsedOptionsPointer;
+typedef std::unique_ptr<struct gnutls_priority_st, HardFun<void, gnutls_priority_t, &gnutls_priority_deinit>> ParsedOptions;
 #else
-typedef std::unique_ptr<long> ParsedOptionsPointer;
+typedef long ParsedOptions;
 #endif
 
 class PeerConnector;

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index 5df56eea2ab434a65b8a009434dab93e461fe07a..d3e6249f956ecedc0315589657f0e40603ecb9e2 100644 (file)
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -185,7 +185,7 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
             }
         } else {
             // Set client SSL options
-            SSL_set_options(serverSession.get(), *::Security::ProxyOutgoingConfig.parsedOptions);
+            SSL_set_options(serverSession.get(), ::Security::ProxyOutgoingConfig.parsedOptions);
 
             // Use SNI TLS extension only when we connect directly
             // to the origin server and we know the server host name.

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index c91c57cbe7854c69fbae02cd0e814d847ddcc1ff..d726c4f30eb6c3af6c7a5eb90a391b8ef8c51de9 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -526,7 +526,7 @@ static bool
 configureSslContext(Security::ContextPointer &ctx, AnyP::PortCfg &port)
 {
     int ssl_error;
-    SSL_CTX_set_options(ctx.get(), *port.secure.parsedOptions);
+    SSL_CTX_set_options(ctx.get(), port.secure.parsedOptions);
 
     maybeDisableRenegotiate(ctx);