CVE-2021-42073: Session Fixation (CWE-384)
value: An issue was discovered in Barrier before 2.4.0. An attacker can enter an active session state with the barriers component (aka the server-side implementation of Barrier) simply by supplying a client label that identifies a valid client configuration. This label is "Unnamed" by default but could instead be guessed from hostnames or other publicly available information. In the active session state, an attacker can capture input device events from the server, and also modify the clipboard content on the server.
CWE description: Authenticating a user, or otherwise establishing a new user session, without invalidating any existing session identifier gives an attacker the opportunity to steal authenticated sessions.
CVSS v3 base score: 8.2
Published: 2021-11-08T04:15Z
Repository: https://github.com/debauchee/barrier
Language: C++
Files changed: doc/newsfragments/client-certificate-checking.bugfix, src/gui/src/MainWindow.cpp, src/lib/barrier/ArgParser.cpp, src/lib/barrier/ServerApp.cpp, src/lib/barrier/ServerArgs.h, src/lib/client/Client.cpp, src/lib/net/ConnectionSecurityLevel.h, src/lib/net/SecureSocket.cpp
Commit date: 2021-11-01 04:50:15 +0200
Commit message: 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>.

Diff: diff --git a/doc/newsfragments/client-certificate-checking.bugfix b/doc/newsfragments/client-certificate-checking.bugfix
new file mode 100644
index 00000000..01d4d03d
--- /dev/null
+++ b/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.
diff --git a/src/gui/src/MainWindow.cpp b/src/gui/src/MainWindow.cpp
index 7d4a9a5e..2172507b 100644
--- a/src/gui/src/MainWindow.cpp
+++ b/src/gui/src/MainWindow.cpp
@@ -425,11 +425,11 @@ 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;
     }
 
     barrier::FingerprintData fingerprint_sha1 = {
@@ -440,11 +440,15 @@ void MainWindow::checkFingerprint(const QString& line)
     barrier::FingerprintData fingerprint_sha256 = {
         barrier::fingerprint_type_to_string(barrier::FingerprintType::SHA256),
         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)) {
         barrier::fs::create_directories(db_dir);
     }
@@ -459,21 +463,21 @@ void MainWindow::checkFingerprint(const QString& line)
     }
 
     static bool messageBoxAlreadyShown = false;
 
     if (!messageBoxAlreadyShown) {
-        stopBarrier();
+        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 "
                "two don't match exactly, then it's probably not the server "
                "you're expecting (it could be a malicious user).\n\n"
@@ -481,18 +485,42 @@ void MainWindow::checkFingerprint(const QString& line)
                "connections, click Yes. To reject this fingerprint and "
                "disconnect from the server, click No.")
             .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);
-            startBarrier();
+            if (is_client) {
+                startBarrier();
+            }
         }
 
         messageBoxAlreadyShown = false;
     }
 }
@@ -732,10 +760,14 @@ bool MainWindow::serverArgs(QStringList& args, QString& app)
         appConfig().persistLogDir();
 
         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.
     configFilename = QString("\"%1\"").arg(configFilename);
 #endif
diff --git a/src/lib/barrier/ArgParser.cpp b/src/lib/barrier/ArgParser.cpp
index 44e29491..99cd803b 100644
--- a/src/lib/barrier/ArgParser.cpp
+++ b/src/lib/barrier/ArgParser.cpp
@@ -63,11 +63,13 @@ ArgParser::parseServerArgs(ServerArgs& args, int argc, const char* const* argv)
         }
         else if (isArg(i, argc, argv, NULL, "--screen-change-script", 1)) {
             // 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;
         }
     }
 
diff --git a/src/lib/barrier/ServerApp.cpp b/src/lib/barrier/ServerApp.cpp
index 8870e0de..4810b8a5 100644
--- a/src/lib/barrier/ServerApp.cpp
+++ b/src/lib/barrier/ServerApp.cpp
@@ -146,11 +146,14 @@ ServerApp::help()
            << WINAPI_ARGS << HELP_SYS_ARGS << HELP_COMMON_ARGS << "\n"
            << "\n"
            << "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"
            << "hostname must be the address or hostname of an interface on the system.\n"
            << "Placing brackets around an IPv6 address is required when also specifying \n"
@@ -656,10 +659,13 @@ ClientListener*
 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(
         address,
         new TCPSocketFactory(m_events, getSocketMultiplexer()),
diff --git a/src/lib/barrier/ServerArgs.h b/src/lib/barrier/ServerArgs.h
index 6d912331..6323705d 100644
--- a/src/lib/barrier/ServerArgs.h
+++ b/src/lib/barrier/ServerArgs.h
@@ -28,6 +28,7 @@ public:
 
 public:
     String                m_configFile;
     Config*                m_config;
     String                m_screenChangeScript;
+    bool check_client_certificates = true;
 };
diff --git a/src/lib/client/Client.cpp b/src/lib/client/Client.cpp
index b000575c..4f1fe73a 100644
--- a/src/lib/client/Client.cpp
+++ b/src/lib/client/Client.cpp
@@ -127,11 +127,12 @@ Client::connect()
         return;
     }
 
     auto security_level = ConnectionSecurityLevel::PLAINTEXT;
     if (m_useSecureNetwork) {
-        security_level = ConnectionSecurityLevel::ENCRYPTED;
+        // client always authenticates server
+        security_level = ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED;
     }
 
     try {
         // resolve the server hostname.  do this every time we connect
         // in case we couldn't resolve the address earlier or the address
diff --git a/src/lib/net/ConnectionSecurityLevel.h b/src/lib/net/ConnectionSecurityLevel.h
index d597bd67..913c2fd7 100644
--- a/src/lib/net/ConnectionSecurityLevel.h
+++ b/src/lib/net/ConnectionSecurityLevel.h
@@ -19,8 +19,9 @@
 #define BARRIER_LIB_NET_CONNECTION_SECURITY_LEVEL_H
 
 enum class ConnectionSecurityLevel {
     PLAINTEXT,
     ENCRYPTED,
+    ENCRYPTED_AUTHENTICATED
 };
 
 #endif // BARRIER_LIB_NET_CONNECTION_SECURITY_LEVEL_H
diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp
index 41d6a7bb..f096729d 100644
--- a/src/lib/net/SecureSocket.cpp
+++ b/src/lib/net/SecureSocket.cpp
@@ -359,10 +359,15 @@ 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)
 {
     SSL_library_init();
 
@@ -394,10 +399,18 @@ SecureSocket::initContext(bool server)
     SSL_CTX_set_options(m_ssl->m_context, SSL_OP_NO_SSLv3);
 
     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
 SecureSocket::createSSL()
 {
@@ -434,10 +447,28 @@ SecureSocket::secureAccept(int socket)
         return -1; // Failed, error out
     }
 
     // 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) {
             showSecureCipherInfo();
         }
@@ -668,11 +699,11 @@ bool SecureSocket::verify_cert_fingerprint(const barrier::fs::path& fingerprint_
         LOG((CLOG_ERR "%s", e.what()));
         return false;
     }
 
     // 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()));
 
     // Provide debug hint as to what file is being used to verify fingerprint trust
     LOG((CLOG_NOTE "fingerprint_db_path: %s", fingerprint_db_path.u8string().c_str()));

Vulnerable code:     QRegExp fingerprintRegex(".*server fingerprint \\(SHA1\\): ([A-F0-9:]+) \\(SHA256\\): ([A-F0-9:]+)");
    auto db_path = barrier::DataDirectories::trusted_servers_ssl_fingerprints_path();
        stopBarrier();
        messageBoxAlreadyShown = true;
        QMessageBox::StandardButton fingerprintReply =
            QMessageBox::information(
            this, tr("Security question"),
            tr("Do you trust this fingerprint?\n\n"
               "SHA1 (obsolete, when using old Barrier server):\n"
            .arg(QString::fromStdString(barrier::format_ssl_fingerprint(fingerprint_sha1.data))),
            startBarrier();
        else {
           << HELP_COMMON_INFO_1 << WINAPI_INFO << HELP_SYS_INFO << HELP_COMMON_INFO_2 << "\n"
        security_level = ConnectionSecurityLevel::ENCRYPTED;
    LOG((CLOG_NOTE "server fingerprint (SHA1): %s (SHA256): %s",
Fixed code: 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.
    QRegExp fingerprintRegex(".*peer fingerprint \\(SHA1\\): ([A-F0-9:]+) \\(SHA256\\): ([A-F0-9:]+)");
    bool is_client = barrierType() == barrierClient;

    auto db_path = is_client
            ? barrier::DataDirectories::trusted_servers_ssl_fingerprints_path()
            : barrier::DataDirectories::trusted_clients_ssl_fingerprints_path();
        if (is_client) {
            stopBarrier();
        }
        QString message;
        if (is_client) {
            message = tr("Do you trust this fingerprint?\n\n"
               "SHA1 (obsolete, when using old Barrier client):\n"
            .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,
            if (is_client) {
                startBarrier();
            }
    if (!appConfig().getRequireClientCertificate()) {
        args << "--disable-client-cert-checking";
    }

        else if (isArg(i, argc, argv, nullptr, "--disable-client-cert-checking")) {
            args.check_client_certificates = false;
        } else {
           << 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"
        if (args().check_client_certificates) {
            security_level = ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED;
        }
    bool check_client_certificates = true;
        // client always authenticates server
        security_level = ConnectionSecurityLevel::ENCRYPTED_AUTHENTICATED;
    ENCRYPTED_AUTHENTICATED
static int cert_verify_ignore_callback(X509_STORE_CTX*, void*)
{
    return 1;
}


    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);
    }
        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
            }
        }

    LOG((CLOG_NOTE "peer fingerprint (SHA1): %s (SHA256): %s",