Merge current state of releaseprep/3.10 to master

* origin/releaseprep/3.10:
  Do not reject control message with trailing newlines
  aws: account for RandomAPI change
  Allow disabling TLS 1.3 in certcheck to more easily debug problems
  Implement changes to allow test dpc certcheck to be tested
  Allow setting a maximum TLS version
  Change cxa1 protocol tag to dpc1
  Fix spelling errors raised by Debian linter
  mac agent: reinstall host route during restart
  Preparing QA cycle for OpenVPN 3 Core library release v3.10

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>

client/ovpncli.cpp

@@ -1339,7 +1339,8 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::send_app_control_channel_msg(const std
 
 static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string &client_cert,
                                                               const std::string &extra_certs,
-                                                              const std::optional<const std::string> &ca)
+                                                              const std::optional<const std::string> &ca,
+                                                              bool disabletls13)
 {
     SSLLib::SSLAPI::Config::Ptr config = new SSLLib::SSLAPI::Config;
     config->set_frame(new Frame(Frame::Context(128, 4096, 4096 - 128, 0, 16, 0)));
@@ -1352,6 +1353,9 @@ static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string
     else
         flags |= SSLConfigAPI::LF_ALLOW_CLIENT_CERT_NOT_REQUIRED;
 
+    if (disabletls13)
+        config->set_tls_version_max(TLSVersion::Type::V1_2);
+
     config->set_flags(flags);
 
     return config;
@@ -1362,6 +1366,7 @@ static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string
   @param client_cert String containing the properly encoded client certificate
   @param clientkey String containing the properly encoded private key for \p client_cert
   @param ca String containing the properly encoded authority
+  @param disableTLS13 disable TLS 1.3 support
 
       Creates, initializes,and installs an SSLLib::SSLAPI::Config object into the TLS
       handshake object we use for the certcheck function. Then begins the handshake
@@ -1369,14 +1374,15 @@ static SSLLib::SSLAPI::Config::Ptr setup_certcheck_ssl_config(const std::string
 */
 OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check(const std::string &client_cert,
                                                            const std::string &clientkey,
-                                                           const std::optional<const std::string> &ca)
+                                                           const std::optional<const std::string> &ca,
+                                                           bool disableTLS13)
 {
     if (state->is_foreign_thread_access())
     {
         ClientConnect *session = state->session.get();
         if (session)
         {
-            SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(client_cert, "", ca);
+            SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(client_cert, "", ca, disableTLS13);
             config->load_private_key(clientkey);
 
             session->start_acc_certcheck(config);
@@ -1384,7 +1390,7 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check(const std::string &cl
     }
 }
 
-OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check_epki(const std::string &alias, const std::optional<const std::string> &ca)
+OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check_epki(const std::string &alias, const std::optional<const std::string> &ca, bool disableTLS13)
 {
     if (state->is_foreign_thread_access())
     {
@@ -1401,7 +1407,7 @@ OPENVPN_CLIENT_EXPORT void OpenVPNClient::start_cert_check_epki(const std::strin
                 return;
             }
 
-            SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(req.cert, req.supportingChain, ca);
+            SSLLib::SSLAPI::Config::Ptr config = setup_certcheck_ssl_config(req.cert, req.supportingChain, ca, disableTLS13);
 
             config->set_external_pki_callback(this, alias);
 

client/ovpncli.hpp

@@ -705,7 +705,8 @@ class OpenVPNClient : public TunBuilderBase,             // expose tun builder v
     */
     void start_cert_check(const std::string &client_cert,
                           const std::string &clientkey,
-                          const std::optional<const std::string> &ca = std::nullopt);
+                          const std::optional<const std::string> &ca = std::nullopt,
+                          bool disableTLS13 = false);
 
     /**
       @brief Start up the cert check handshake using the given epki_alias string
@@ -716,7 +717,7 @@ class OpenVPNClient : public TunBuilderBase,             // expose tun builder v
       session ACC certcheck TLS handshake object. Every time this function is called the state of
       the handshake object will be reset and the handshake will be restarted.
     */
-    void start_cert_check_epki(const std::string &alias, const std::optional<const std::string> &ca);
+    void start_cert_check_epki(const std::string &alias, const std::optional<const std::string> &ca, bool disableTLS13 = false);
 
     // Callback for delivering events during connect() call.
     // Will be called from the thread executing connect().

openvpn/aws/awsroute.hpp

@@ -73,7 +73,7 @@ class Route
       public:
         Context(PCQuery::Info instance_info_arg,
                 Creds creds_arg,
-                RandomAPI::Ptr rng,
+                StrongRandomAPI::Ptr rng,
                 Stop *async_stop_arg,
                 const int debug_level)
             : instance_info(std::move(instance_info_arg)),

openvpn/client/cliconnect.hpp

@@ -446,7 +446,7 @@ class ClientConnect : ClientProto::NotifyCallback,
             auto timer_left = std::chrono::duration_cast<std::chrono::seconds>(conn_timer.expiry() - AsioTimer::clock_type::now()).count();
             if (timer_left < timeout)
             {
-                OPENVPN_LOG("Extending connection timeout from " << timer_left << " to " << timeout << " for pending authentification");
+                OPENVPN_LOG("Extending connection timeout from " << timer_left << " to " << timeout << " for pending authentication");
                 conn_timer.cancel();
                 conn_timer_pending = false;
                 conn_timer_start(timeout);

openvpn/mbedtls/ssl/sslctx.hpp

@@ -412,6 +412,10 @@ class MbedTLSContext : public SSLFactoryAPI
             tls_version_min = tvm;
         }
 
+        virtual void set_tls_version_max(const TLSVersion::Type tvm)
+        {
+        }
+
         virtual void set_tls_version_min_override(const std::string &override)
         {
             TLSVersion::apply_override(tls_version_min, override);

openvpn/openssl/ssl/sslctx.hpp

@@ -322,6 +322,11 @@ class OpenSSLContext : public SSLFactoryAPI
             tls_version_min = tvm;
         }
 
+        void set_tls_version_max(const TLSVersion::Type tvm) override
+        {
+            tls_version_max = tvm;
+        }
+
         void set_tls_version_min_override(const std::string &override) override
         {
             TLSVersion::apply_override(tls_version_min, override);
@@ -631,7 +636,7 @@ class OpenSSLContext : public SSLFactoryAPI
 
                 default_provider.reset(OSSL_PROVIDER_load(lib_ctx.get(), "default"));
                 if (!default_provider)
-                    throw OpenSSLException("OpenSSLContext: laoding default provider failed");
+                    throw OpenSSLException("OpenSSLContext: loading default provider failed");
             }
 #endif
         }
@@ -678,9 +683,10 @@ class OpenSSLContext : public SSLFactoryAPI
         std::vector<unsigned int> ku; // if defined, peer cert X509 key usage must match one of these values
         std::string eku;              // if defined, peer cert X509 extended key usage must match this OID/string
         std::string tls_remote;
-        VerifyX509Name verify_x509_name;                          // --verify-x509-name feature
-        PeerFingerprints peer_fingerprints;                       // --peer-fingerprint
-        TLSVersion::Type tls_version_min{TLSVersion::Type::V1_2}; // minimum TLS version that we will negotiate
+        VerifyX509Name verify_x509_name;                           // --verify-x509-name feature
+        PeerFingerprints peer_fingerprints;                        // --peer-fingerprint
+        TLSVersion::Type tls_version_min{TLSVersion::Type::V1_2};  // minimum TLS version that we will negotiate
+        TLSVersion::Type tls_version_max{TLSVersion::Type::UNDEF}; // maximum TLS version that we will negotiate, use for testing only (NOT exposed via tls-version-max)
         TLSCertProfile::Type tls_cert_profile{TLSCertProfile::UNDEF};
         std::string tls_cipher_list;
         std::string tls_ciphersuite_list;
@@ -1260,6 +1266,27 @@ class OpenSSLContext : public SSLFactoryAPI
         if (config->tls_version_min > TLSVersion::Type::V1_3)
             sslopt |= SSL_OP_NO_TLSv1_3;
 #endif
+#endif
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L
+        if (config->tls_version_max != TLSVersion::Type::UNDEF)
+        {
+            SSL_CTX_set_max_proto_version(ctx.get(), TLSVersion::toTLSVersion(config->tls_version_max));
+        }
+#else
+        if (config->tls_version_max < TLSVersion::Type::V1_0)
+            sslopt |= SSL_OP_NO_TLSv1;
+#ifdef SSL_OP_NO_TLSv1_1
+        if (config->tls_version_max < TLSVersion::Type::V1_1)
+            sslopt |= SSL_OP_NO_TLSv1_1;
+#endif
+#ifdef SSL_OP_NO_TLSv1_2
+        if (config->tls_version_max < TLSVersion::Type::V1_2)
+            sslopt |= SSL_OP_NO_TLSv1_2;
+#endif
+#ifdef SSL_OP_NO_TLSv1_3
+        if (config->tls_version_max < TLSVersion::Type::V1_3)
+            sslopt |= SSL_OP_NO_TLSv1_3;
+#endif
 #endif
         SSL_CTX_set_options(ctx.get(), sslopt);
 

openvpn/ovpnagent/mac/ovpnagent.cpp

@@ -290,6 +290,7 @@ class MyListener : public WS::Server::Listener
 
         remove_cmds_bypass_hosts.execute(os);
         remove_cmds_bypass_hosts.clear();
+        bypass_host.clear();
     }
 
     void set_watchdog(pid_t pid)

openvpn/ssl/proto.hpp

@@ -1529,12 +1529,19 @@ class ProtoContext : public logging::LoggingMixin<OPENVPN_DEBUG_PROTO,
         size_t size = buf.size();
         if (size)
         {
-            if (buf[size - 1] == 0)
+            /* Trim any trailing \n or \r or 0x00 characters. Scripts plugin sometimes accidentally include a \n or \r\n in AUTH_FAILED
+             * or similar messages */
+            while (size > 0 && (buf[size - 1] == 0 || buf[size - 1] == '\r' || buf[size - 1] == '\n'))
+            {
                 --size;
+            }
+
             if (size)
-                return S((const char *)buf.c_data(), size);
+            {
+                return S{reinterpret_cast<const char *>(buf.c_data()), size};
+            }
         }
-        return S();
+        return {};
     }
 
     template <typename S>

openvpn/ssl/sslapi.hpp

@@ -177,6 +177,7 @@ class SSLConfigAPI : public RC<thread_unsafe_refcount>
     virtual void set_remote_cert_tls(const KUParse::TLSWebType wt) = 0;
     virtual void set_tls_remote(const std::string &tls_remote_arg) = 0;
     virtual void set_tls_version_min(const TLSVersion::Type tvm) = 0;
+    virtual void set_tls_version_max(const TLSVersion::Type tvm) = 0;
     virtual void set_tls_version_min_override(const std::string &override) = 0;
     virtual void set_tls_cert_profile(const TLSCertProfile::Type type) = 0;
     virtual void set_tls_cert_profile_override(const std::string &override) = 0;

test/ovpncli/cli.cpp

@@ -216,7 +216,7 @@ class ClientBase : public ClientAPI::OpenVPNClient
 class Client : public ClientBase
 {
     // This is the protocol tag that appcontrol expects to trigger the certcheck start.
-    static constexpr char certcheck_init_verb[] = "cxa1"; // TODO: std::string in C++ '20
+    static constexpr char certcheck_init_verb[] = "dpc1"; // TODO: std::string in C++ '20
 
   public:
     enum ClockTickAction
@@ -343,6 +343,33 @@ class Client : public ClientBase
         }
     }
 
+    void handle_dpc1_protocol(const ClientAPI::AppCustomControlMessageEvent &acev)
+    {
+        if (string::starts_with(acev.payload, "{\"dpc_request\"") && acev.payload.find("certificate") != std::string::npos)
+        {
+            std::cout << "ACC CERTCHECK challenge initiated" << std::endl;
+            handle_certcheck_request();
+        }
+        else if (string::starts_with(acev.payload, "{\"dpc_request\"") && acev.payload.find("client_info") != std::string::npos)
+        {
+            std::string fakeResponse{R"("dpc_response\": {
+                    "client_info" : {
+                            "os" :  {"type" : "FakeOS", "version" : "1.2.3.4" }
+                        }
+                    })"};
+
+            send_app_control_channel_msg("dpc1", fakeResponse);
+        }
+        else if (string::starts_with(acev.payload, "{\"dpc_request\""))
+        {
+            std::cout << "Cannot parse dpc request message:" << acev.payload << std::endl;
+        }
+        else
+        {
+            std::cout << "Cannot parse device posture message:" << acev.payload << std::endl;
+        }
+    }
+
     /**
       @brief Handles ACC messages
       @param acev The current ACC event
@@ -355,21 +382,17 @@ class Client : public ClientBase
         }
         else if (acev.protocol == certcheck_init_verb)
         {
-            if (string::starts_with(acev.payload, "{\"dpc_certcheck_cert_req\""))
-            {
-                std::cout << "ACC CERTCHECK challenge initiated\n";
-                handle_certcheck_request();
-            }
+            handle_dpc1_protocol(acev);
         }
         else
         {
-            std::cout << "received app custom control message for protocol " << acev.protocol
+            std::cout << "received unhandled app custom control message for protocol " << acev.protocol
                       << " msg payload: " << acev.payload << std::endl;
         }
     }
 
     /**
-      @brief Begin a cck1 (certcheck) handshake in response to a cxa1 server request.
+      @brief Begin a cck1 (certcheck) handshake in response to a dpc1 server request.
     */
     void handle_certcheck_request()
     {

test/unittests/test_proto.cpp

@@ -1327,3 +1327,44 @@ TEST(proto, iv_ciphers_legacy)
 
     EXPECT_EQ(ivciphers, expectedstr);
 }
+
+TEST(proto, controlmessage_invalidchar)
+{
+    std::string valid_auth_fail{"AUTH_FAILED: go away"};
+    std::string valid_auth_fail_newline_end{"AUTH_FAILED: go away\n"};
+    std::string invalid_auth_fail{"AUTH_FAILED: go\n away\n"};
+    std::string lot_of_whitespace{"AUTH_FAILED: a lot of white space\n\n\r\n\r\n\r\n"};
+    std::string only_whitespace{"\n\n\r\n\r\n\r\n"};
+    std::string empty{""};
+
+    BufferAllocated valid_auth_fail_buf{reinterpret_cast<const unsigned char *>(valid_auth_fail.c_str()), valid_auth_fail.size(), BufferAllocated::GROW};
+    BufferAllocated valid_auth_fail_newline_end_buf{reinterpret_cast<const unsigned char *>(valid_auth_fail_newline_end.c_str()), valid_auth_fail_newline_end.size(), BufferAllocated::GROW};
+    BufferAllocated invalid_auth_fail_buf{reinterpret_cast<const unsigned char *>(invalid_auth_fail.c_str()), invalid_auth_fail.size(), BufferAllocated::GROW};
+    BufferAllocated lot_of_whitespace_buf{reinterpret_cast<const unsigned char *>(lot_of_whitespace.c_str()), lot_of_whitespace.size(), BufferAllocated::GROW};
+    BufferAllocated only_whitespace_buf{reinterpret_cast<const unsigned char *>(only_whitespace.c_str()), only_whitespace.size(), BufferAllocated::GROW};
+    BufferAllocated empty_buf{reinterpret_cast<const unsigned char *>(empty.c_str()), empty.size(), BufferAllocated::GROW};
+
+    auto msg = ProtoContext::read_control_string<std::string>(valid_auth_fail_buf);
+    EXPECT_EQ(msg, valid_auth_fail);
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg, Unicode::UTF8_NO_CTRL));
+
+    auto msg2 = ProtoContext::read_control_string<std::string>(valid_auth_fail_newline_end_buf);
+    EXPECT_EQ(msg2, valid_auth_fail);
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg2, Unicode::UTF8_NO_CTRL));
+
+    auto msg3 = ProtoContext::read_control_string<std::string>(invalid_auth_fail_buf);
+    EXPECT_EQ(msg3, "AUTH_FAILED: go\n away");
+    EXPECT_FALSE(Unicode::is_valid_utf8(msg3, Unicode::UTF8_NO_CTRL));
+
+    auto msg4 = ProtoContext::read_control_string<std::string>(lot_of_whitespace_buf);
+    EXPECT_EQ(msg4, "AUTH_FAILED: a lot of white space");
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg4, Unicode::UTF8_NO_CTRL));
+
+    auto msg5 = ProtoContext::read_control_string<std::string>(only_whitespace_buf);
+    EXPECT_EQ(msg5, "");
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg5, Unicode::UTF8_NO_CTRL));
+
+    auto msg6 = ProtoContext::read_control_string<std::string>(empty_buf);
+    EXPECT_EQ(msg6, "");
+    EXPECT_TRUE(Unicode::is_valid_utf8(msg5, Unicode::UTF8_NO_CTRL));
+}