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 @@ -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) { - 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 " @@ -483,14 +487,38 @@ 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); - startBarrier(); + 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. 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 @@ -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; } 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 @@ -148,7 +148,10 @@ ServerApp::help() << "Options:\n" << " -a, --address
listen for clients on the given address.\n" << " -c, --config 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: [][:]. 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( 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 @@ -30,4 +30,5 @@ 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 @@ -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 { 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 @@ -21,6 +21,7 @@ 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 @@ -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()));