Implement client identity verification

This commit fixes two security vulnerabilities: CVE-2021-42072 and
CVE-2021-42073.

The issues have been reported by Matthias Gerstner <mgerstner@suse.de>.

doc/newsfragments/client-certificate-checking.bugfix

@@ -0,0 +1,7 @@
+SECURITY ISSUE
+
+Barrier now supports client identity verification (fixes CVE-2021-42072, CVE-2021-42073).
+
+To support seamless upgrades from older versions of Barrier this is currently disabled by default.
+The feature can be enabled in the settings dialog. If enabled, older clients of Barrier will be
+rejected.

src/gui/src/MainWindow.cpp

@@ -427,7 +427,7 @@ void MainWindow::checkConnected(const QString& line)
 
 void MainWindow::checkFingerprint(const QString& line)
 {
-    QRegExp fingerprintRegex(".*server fingerprint \\(SHA1\\): ([A-F0-9:]+) \\(SHA256\\): ([A-F0-9:]+)");
+    QRegExp fingerprintRegex(".*peer fingerprint \\(SHA1\\): ([A-F0-9:]+) \\(SHA256\\): ([A-F0-9:]+)");
     if (!fingerprintRegex.exactMatch(line)) {
         return;
     }
@@ -442,7 +442,11 @@ void MainWindow::checkFingerprint(const QString& line)
         barrier::string::from_hex(fingerprintRegex.cap(2).toStdString())
     };
 
-    auto db_path = barrier::DataDirectories::trusted_servers_ssl_fingerprints_path();
+    bool is_client = barrierType() == barrierClient;
+
+    auto db_path = is_client
+            ? barrier::DataDirectories::trusted_servers_ssl_fingerprints_path()
+            : barrier::DataDirectories::trusted_clients_ssl_fingerprints_path();
 
     auto db_dir = db_path.parent_path();
     if (!barrier::fs::exists(db_dir)) {
@@ -461,17 +465,17 @@ void MainWindow::checkFingerprint(const QString& line)
     static bool messageBoxAlreadyShown = false;
 
     if (!messageBoxAlreadyShown) {
+        if (is_client) {
             stopBarrier();
+        }
 
-        messageBoxAlreadyShown = true;
-        QMessageBox::StandardButton fingerprintReply =
-            QMessageBox::information(
-            this, tr("Security question"),
-            tr("Do you trust this fingerprint?\n\n"
+        QString message;
+        if (is_client) {
+            message = tr("Do you trust this fingerprint?\n\n"
                "SHA256:\n"
                "%1\n"
                "%2\n\n"
-               "SHA1 (obsolete, when using old Barrier server):\n"
+               "SHA1 (obsolete, when using old Barrier client):\n"
                "%3\n\n"
                "This is a server fingerprint. You should compare this "
                "fingerprint to the one on your server's screen. If the "
@@ -483,15 +487,39 @@ void MainWindow::checkFingerprint(const QString& line)
             .arg(QString::fromStdString(barrier::format_ssl_fingerprint(fingerprint_sha256.data)))
             .arg(QString::fromStdString(
                      barrier::create_fingerprint_randomart(fingerprint_sha256.data)))
-            .arg(QString::fromStdString(barrier::format_ssl_fingerprint(fingerprint_sha1.data))),
+            .arg(QString::fromStdString(barrier::format_ssl_fingerprint(fingerprint_sha1.data)));
+        } else {
+            message = tr("Do you trust this fingerprint?\n\n"
+               "SHA256:\n"
+               "%1\n"
+               "%2\n\n"
+               "This is a client fingerprint. You should compare this "
+               "fingerprint to the one on your client's screen. If the "
+               "two don't match exactly, then it's probably not the client "
+               "you're expecting (it could be a malicious user).\n\n"
+               "To automatically trust this fingerprint for future "
+               "connections, click Yes. To reject this fingerprint and "
+               "disconnect the client, click No.")
+            .arg(QString::fromStdString(barrier::format_ssl_fingerprint(fingerprint_sha256.data)))
+            .arg(QString::fromStdString(
+                     barrier::create_fingerprint_randomart(fingerprint_sha256.data)));
+        }
+
+        messageBoxAlreadyShown = true;
+        QMessageBox::StandardButton fingerprintReply =
+            QMessageBox::information(
+            this, tr("Security question"),
+            message,
             QMessageBox::Yes | QMessageBox::No);
 
         if (fingerprintReply == QMessageBox::Yes) {
             // restart core process after trusting fingerprint.
             db.add_trusted(fingerprint_sha256);
             db.write(db_path);
+            if (is_client) {
                 startBarrier();
             }
+        }
 
         messageBoxAlreadyShown = false;
     }
@@ -734,6 +762,10 @@ bool MainWindow::serverArgs(QStringList& args, QString& app)
         args << "--log" << appConfig().logFilenameCmd();
     }
 
+    if (!appConfig().getRequireClientCertificate()) {
+        args << "--disable-client-cert-checking";
+    }
+
     QString configFilename = this->configFilename();
 #if defined(Q_OS_WIN)
     // wrap in quotes in case username contains spaces.

src/lib/barrier/ArgParser.cpp

@@ -65,7 +65,9 @@ ArgParser::parseServerArgs(ServerArgs& args, int argc, const char* const* argv)
             // save screen change script path
             args.m_screenChangeScript = argv[++i];
         }
-        else {
+        else if (isArg(i, argc, argv, nullptr, "--disable-client-cert-checking")) {
+            args.check_client_certificates = false;
+        } else {
             LOG((CLOG_PRINT "%s: unrecognized option `%s'" BYE, args.m_exename.c_str(), argv[i], args.m_exename.c_str()));
             return false;
         }

src/lib/barrier/ServerApp.cpp

@@ -148,7 +148,10 @@ ServerApp::help()
            << "Options:\n"
            << "  -a, --address <address>  listen for clients on the given address.\n"
            << "  -c, --config <pathname>  use the named configuration file instead.\n"
-           << HELP_COMMON_INFO_1 << WINAPI_INFO << HELP_SYS_INFO << HELP_COMMON_INFO_2 << "\n"
+           << HELP_COMMON_INFO_1
+           << "      --disable-client-cert-checking disable client SSL certificate \n"
+              "                                     checking (deprecated)\n"
+           << WINAPI_INFO << HELP_SYS_INFO << HELP_COMMON_INFO_2 << "\n"
            << "Default options are marked with a *\n"
            << "\n"
            << "The argument for --address is of the form: [<hostname>][:<port>].  The\n"
@@ -658,6 +661,9 @@ ServerApp::openClientListener(const NetworkAddress& address)
     auto security_level = ConnectionSecurityLevel::PLAINTEXT;
     if (args().m_enableCrypto) {
         security_level = ConnectionSecurityLevel::ENCRYPTED;
+        if (args().check_client_certificates) {
+            security_level = ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED;
+        }
     }
 
     ClientListener* listen = new ClientListener(

src/lib/barrier/ServerArgs.h

@@ -30,4 +30,5 @@ public:
     String                m_configFile;
     Config*                m_config;
     String                m_screenChangeScript;
+    bool check_client_certificates = true;
 };

src/lib/client/Client.cpp

@@ -129,7 +129,8 @@ Client::connect()
 
     auto security_level = ConnectionSecurityLevel::PLAINTEXT;
     if (m_useSecureNetwork) {
-        security_level = ConnectionSecurityLevel::ENCRYPTED;
+        // client always authenticates server
+        security_level = ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED;
     }
 
     try {

src/lib/net/ConnectionSecurityLevel.h

@@ -21,6 +21,7 @@
 enum class ConnectionSecurityLevel {
     PLAINTEXT,
     ENCRYPTED,
+    ENCRYPTED_AUTHENTICATED
 };
 
 #endif // BARRIER_LIB_NET_CONNECTION_SECURITY_LEVEL_H

src/lib/net/SecureSocket.cpp

@@ -361,6 +361,11 @@ bool SecureSocket::load_certificates(const barrier::fs::path& path)
     return true;
 }
 
+static int cert_verify_ignore_callback(X509_STORE_CTX*, void*)
+{
+    return 1;
+}
+
 void
 SecureSocket::initContext(bool server)
 {
@@ -396,6 +401,14 @@ SecureSocket::initContext(bool server)
     if (m_ssl->m_context == NULL) {
         showError("");
     }
+
+    if (security_level_ == ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED) {
+        // We want to ask for peer certificate, but not verify it. If we don't ask for peer
+        // certificate, e.g. client won't send it.
+        SSL_CTX_set_verify(m_ssl->m_context, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT,
+                           nullptr);
+        SSL_CTX_set_cert_verify_callback(m_ssl->m_context, cert_verify_ignore_callback, nullptr);
+    }
 }
 
 void
@@ -436,6 +449,24 @@ SecureSocket::secureAccept(int socket)
 
     // If not fatal and no retry, state is good
     if (retry == 0) {
+        if (security_level_ == ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED) {
+            if (verify_cert_fingerprint(
+                        barrier::DataDirectories::trusted_clients_ssl_fingerprints_path())) {
+                LOG((CLOG_INFO "accepted secure socket"));
+                if (!ensure_peer_certificate()) {
+                    retry = 0;
+                    disconnect();
+                    return -1;// Cert fail, error
+                }
+            }
+            else {
+                LOG((CLOG_ERR "failed to verify server certificate fingerprint"));
+                retry = 0;
+                disconnect();
+                return -1; // Fingerprint failed, error
+            }
+        }
+
         m_secureReady = true;
         LOG((CLOG_INFO "accepted secure socket"));
         if (CLOG->getFilter() >= kDEBUG1) {
@@ -670,7 +701,7 @@ bool SecureSocket::verify_cert_fingerprint(const barrier::fs::path& fingerprint_
     }
 
     // note: the GUI parses the following two lines of logs, don't change unnecessarily
-    LOG((CLOG_NOTE "server fingerprint (SHA1): %s (SHA256): %s",
+    LOG((CLOG_NOTE "peer fingerprint (SHA1): %s (SHA256): %s",
          barrier::format_ssl_fingerprint(fingerprint_sha1.data).c_str(),
          barrier::format_ssl_fingerprint(fingerprint_sha256.data).c_str()));