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/doc/newsfragments/client-send-certificate.feature b/doc/newsfragments/client-send-certificate.feature new file mode 100644 index 00000000..b5584290 --- /dev/null +++ b/doc/newsfragments/client-send-certificate.feature @@ -0,0 +1 @@ +Barrier client now sends certificate that the server can verify. diff --git a/src/gui/CMakeLists.txt b/src/gui/CMakeLists.txt index fb7678f2..570e8424 100644 --- a/src/gui/CMakeLists.txt +++ b/src/gui/CMakeLists.txt @@ -29,6 +29,7 @@ set(GUI_SOURCE_FILES src/CommandProcess.cpp src/DataDownloader.cpp src/DisplayIsValid.cpp + src/FingerprintAcceptDialog.cpp src/HotkeyDialog.cpp src/IpcClient.cpp src/Ipc.cpp @@ -104,6 +105,7 @@ set(GUI_UI_FILES src/AboutDialogBase.ui src/ActionDialogBase.ui src/AddClientDialogBase.ui + src/FingerprintAcceptDialog.ui src/HotkeyDialogBase.ui src/LogWindowBase.ui src/MainWindowBase.ui diff --git a/src/gui/src/AppConfig.cpp b/src/gui/src/AppConfig.cpp index 63943267..894ce49a 100644 --- a/src/gui/src/AppConfig.cpp +++ b/src/gui/src/AppConfig.cpp @@ -158,6 +158,8 @@ void AppConfig::loadSettings() m_ElevateMode = static_cast(elevateMode.toInt()); m_AutoConfigPrompted = settings().value("autoConfigPrompted", false).toBool(); m_CryptoEnabled = settings().value("cryptoEnabled", true).toBool(); + // TODO: set default value of requireClientCertificate to true on Barrier 2.5.0 + m_RequireClientCertificate = settings().value("requireClientCertificate", false).toBool(); m_AutoHide = settings().value("autoHide", false).toBool(); m_AutoStart = settings().value("autoStart", false).toBool(); m_MinimizeToTray = settings().value("minimizeToTray", false).toBool(); @@ -181,6 +183,7 @@ void AppConfig::saveSettings() settings().setValue("elevateModeEnum", static_cast(m_ElevateMode)); settings().setValue("autoConfigPrompted", m_AutoConfigPrompted); settings().setValue("cryptoEnabled", m_CryptoEnabled); + settings().setValue("requireClientCertificate", m_RequireClientCertificate); settings().setValue("autoHide", m_AutoHide); settings().setValue("autoStart", m_AutoStart); settings().setValue("minimizeToTray", m_MinimizeToTray); @@ -225,6 +228,10 @@ void AppConfig::setCryptoEnabled(bool e) { m_CryptoEnabled = e; } bool AppConfig::getCryptoEnabled() const { return m_CryptoEnabled; } +void AppConfig::setRequireClientCertificate(bool e) { m_RequireClientCertificate = e; } + +bool AppConfig::getRequireClientCertificate() const { return m_RequireClientCertificate; } + void AppConfig::setAutoHide(bool b) { m_AutoHide = b; } bool AppConfig::getAutoHide() { return m_AutoHide; } diff --git a/src/gui/src/AppConfig.h b/src/gui/src/AppConfig.h index 124ee85f..0dabb182 100644 --- a/src/gui/src/AppConfig.h +++ b/src/gui/src/AppConfig.h @@ -91,6 +91,9 @@ class AppConfig: public QObject void setCryptoEnabled(bool e); bool getCryptoEnabled() const; + void setRequireClientCertificate(bool e); + bool getRequireClientCertificate() const; + void setAutoHide(bool b); bool getAutoHide(); @@ -132,6 +135,7 @@ protected: ElevateMode m_ElevateMode; bool m_AutoConfigPrompted; bool m_CryptoEnabled; + bool m_RequireClientCertificate = false; bool m_AutoHide; bool m_AutoStart; bool m_MinimizeToTray; diff --git a/src/gui/src/FingerprintAcceptDialog.cpp b/src/gui/src/FingerprintAcceptDialog.cpp new file mode 100644 index 00000000..e0dc7e60 --- /dev/null +++ b/src/gui/src/FingerprintAcceptDialog.cpp @@ -0,0 +1,65 @@ +/* + barrier -- mouse and keyboard sharing utility + Copyright (C) Barrier contributors + + This package is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + found in the file LICENSE that should have accompanied this file. + + This package is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . +*/ + +#include "FingerprintAcceptDialog.h" +#include "ui_FingerprintAcceptDialog.h" +#include "net/SecureUtils.h" + +FingerprintAcceptDialog::FingerprintAcceptDialog(QWidget *parent, + BarrierType type, + const barrier::FingerprintData& fingerprint_sha1, + const barrier::FingerprintData& fingerprint_sha256) : + QDialog(parent), + ui_{std::make_unique()} +{ + ui_->setupUi(this); + + if (type == BarrierType::Server) { + ui_->label_sha1->hide(); + ui_->label_sha1_fingerprint_full->hide(); + } else { + ui_->label_sha1_fingerprint_full->setText( + QString::fromStdString(barrier::format_ssl_fingerprint(fingerprint_sha1.data))); + } + + ui_->label_sha256_fingerprint_full->setText( + QString::fromStdString(barrier::format_ssl_fingerprint_columns(fingerprint_sha256.data))); + ui_->label_sha256_fingerprint_randomart->setText( + QString::fromStdString(barrier::create_fingerprint_randomart(fingerprint_sha256.data))); + + QString explanation; + if (type == BarrierType::Server) { + explanation = tr("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."); + } else { + explanation = tr("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" + "To automatically trust this fingerprint for future " + "connections, click Yes. To reject this fingerprint and " + "disconnect from the server, click No."); + } + ui_->label_explanation->setText(explanation); +} + +FingerprintAcceptDialog::~FingerprintAcceptDialog() = default; diff --git a/src/gui/src/FingerprintAcceptDialog.h b/src/gui/src/FingerprintAcceptDialog.h new file mode 100644 index 00000000..da8884c9 --- /dev/null +++ b/src/gui/src/FingerprintAcceptDialog.h @@ -0,0 +1,45 @@ +/* + barrier -- mouse and keyboard sharing utility + Copyright (C) Barrier contributors + + This package is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + found in the file LICENSE that should have accompanied this file. + + This package is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . +*/ + +#ifndef BARRIER_GUI_FINGERPRINT_ACCEPT_DIALOG_H +#define BARRIER_GUI_FINGERPRINT_ACCEPT_DIALOG_H + +#include "net/FingerprintData.h" +#include "barrier/BarrierType.h" +#include +#include + +namespace Ui { +class FingerprintAcceptDialog; +} + +class FingerprintAcceptDialog : public QDialog +{ + Q_OBJECT + +public: + explicit FingerprintAcceptDialog(QWidget* parent, + BarrierType type, + const barrier::FingerprintData& fingerprint_sha1, + const barrier::FingerprintData& fingerprint_sha256); + ~FingerprintAcceptDialog() override; + +private: + std::unique_ptr ui_; +}; + +#endif // BARRIER_GUI_FINGERPRINT_ACCEPT_DIALOG_H diff --git a/src/gui/src/FingerprintAcceptDialog.ui b/src/gui/src/FingerprintAcceptDialog.ui new file mode 100644 index 00000000..9c181ec3 --- /dev/null +++ b/src/gui/src/FingerprintAcceptDialog.ui @@ -0,0 +1,174 @@ + + + FingerprintAcceptDialog + + + + 0 + 0 + 600 + 400 + + + + + 0 + 0 + + + + Security question + + + + QLayout::SetFixedSize + + + + + Qt::Horizontal + + + QDialogButtonBox::No|QDialogButtonBox::Yes + + + + + + + + 0 + 0 + + + + SHA1 (deprecated, compare to old servers only) + + + + + + + + 0 + 0 + + + + + + + true + + + 10 + + + + + + + + 0 + 0 + + + + + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse + + + + + + + Do you trust this fingerprint? + + + + + + + + 0 + 0 + + + + + Courier + 75 + true + + + + + + + Qt::AlignCenter + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse + + + + + + + + + + Qt::AlignCenter + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse + + + + + + + SHA256: + + + + + + + + + buttonBox + accepted() + FingerprintAcceptDialog + accept() + + + 248 + 254 + + + 157 + 274 + + + + + buttonBox + rejected() + FingerprintAcceptDialog + reject() + + + 316 + 260 + + + 286 + 274 + + + + + diff --git a/src/gui/src/MainWindow.cpp b/src/gui/src/MainWindow.cpp index 87f31eb2..d17548a4 100644 --- a/src/gui/src/MainWindow.cpp +++ b/src/gui/src/MainWindow.cpp @@ -26,6 +26,7 @@ #include "ZeroconfService.h" #include "DataDownloader.h" #include "CommandProcess.h" +#include "FingerprintAcceptDialog.h" #include "QUtility.h" #include "ProcessorArch.h" #include "SslCertificate.h" @@ -427,7 +428,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 +443,16 @@ 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 = barrier_type() == BarrierType::Client; + + 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); + } // We compare only SHA256 fingerprints, but show both SHA1 and SHA256 so that the users can // still verify fingerprints on old Barrier servers. This way the only time when we are exposed @@ -456,36 +466,19 @@ 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" - "SHA256:\n" - "%1\n" - "%2\n\n" - "SHA1 (obsolete, when using old Barrier server):\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" - "To automatically trust this fingerprint for future " - "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))), - QMessageBox::Yes | QMessageBox::No); - - if (fingerprintReply == QMessageBox::Yes) { + FingerprintAcceptDialog dialog{this, barrier_type(), fingerprint_sha1, fingerprint_sha256}; + if (dialog.exec() == QDialog::Accepted) { // restart core process after trusting fingerprint. db.add_trusted(fingerprint_sha256); db.write(db_path); - startBarrier(); + if (is_client) { + startBarrier(); + } } messageBoxAlreadyShown = false; @@ -567,8 +560,8 @@ void MainWindow::startBarrier() args << "--profile-dir" << QString::fromStdString("\"" + barrier::DataDirectories::profile().u8string() + "\""); #endif - if ((barrierType() == barrierClient && !clientArgs(args, app)) - || (barrierType() == barrierServer && !serverArgs(args, app))) + if ((barrier_type() == BarrierType::Client && !clientArgs(args, app)) + || (barrier_type() == BarrierType::Server && !serverArgs(args, app))) { stopBarrier(); return; @@ -583,7 +576,7 @@ void MainWindow::startBarrier() m_pLogWindow->startNewInstance(); - appendLogInfo("starting " + QString(barrierType() == barrierServer ? "server" : "client")); + appendLogInfo("starting " + QString(barrier_type() == BarrierType::Server ? "server" : "client")); qDebug() << args; @@ -693,6 +686,11 @@ QString MainWindow::configFilename() return filename; } +BarrierType MainWindow::barrier_type() const +{ + return m_pGroupClient->isChecked() ? BarrierType::Client : BarrierType::Server; +} + QString MainWindow::address() { QString address = appConfig().networkInterface(); @@ -729,6 +727,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. @@ -983,7 +985,7 @@ void MainWindow::updateZeroconfService() m_pZeroconfService = NULL; } - if (m_AppConfig->autoConfig() || barrierType() == barrierServer) { + if (m_AppConfig->autoConfig() || barrier_type() == BarrierType::Server) { m_pZeroconfService = new ZeroconfService(this); } } diff --git a/src/gui/src/MainWindow.h b/src/gui/src/MainWindow.h index 59a0e0db..0c582c9f 100644 --- a/src/gui/src/MainWindow.h +++ b/src/gui/src/MainWindow.h @@ -20,6 +20,8 @@ #define MAINWINDOW__H +#include "barrier/BarrierType.h" + #include #include #include @@ -76,12 +78,6 @@ class MainWindow : public QMainWindow, public Ui::MainWindowBase barrierTransfering }; - enum qBarrierType - { - barrierClient, - barrierServer - }; - enum qLevel { Error, Info @@ -98,7 +94,7 @@ class MainWindow : public QMainWindow, public Ui::MainWindowBase public: void setVisible(bool visible); - int barrierType() const { return m_pGroupClient->isChecked() ? barrierClient : barrierServer; } + BarrierType barrier_type() const; int barrierState() const { return m_BarrierState; } QString hostname() const { return m_pLineEditHostname->text(); } QString configFilename(); diff --git a/src/gui/src/MainWindowBase.ui b/src/gui/src/MainWindowBase.ui index 9f8d4896..88994cf9 100644 --- a/src/gui/src/MainWindowBase.ui +++ b/src/gui/src/MainWindowBase.ui @@ -55,107 +55,6 @@ - - - - - - - 0 - 0 - - - - SSL Fingerprint: - - - - - - - - - - Qt::PlainText - - - - - - - ... - - - Qt::DownArrow - - - - - - - - - QFrame::StyledPanel - - - QFrame::Raised - - - - QLayout::SetMinimumSize - - - - - - Courier - - - - - - - Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse - - - - - - - SHA1 (deprecated, compare to old clients only): - - - - - - - - - - Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse - - - - - - - SHA256: - - - - - - - - - - Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse - - - - - - @@ -305,6 +204,107 @@ + + + + + + + 0 + 0 + + + + SSL Fingerprint: + + + + + + + + + + Qt::PlainText + + + + + + + ... + + + Qt::DownArrow + + + + + + + + + QFrame::StyledPanel + + + QFrame::Raised + + + + QLayout::SetMinimumSize + + + + + + Courier + + + + + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse + + + + + + + SHA1 (deprecated, compare to old clients and servers only): + + + + + + + + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse + + + + + + + SHA256: + + + + + + + + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByMouse + + + + + + diff --git a/src/gui/src/SettingsDialog.cpp b/src/gui/src/SettingsDialog.cpp index 1caeae5d..8d01c777 100644 --- a/src/gui/src/SettingsDialog.cpp +++ b/src/gui/src/SettingsDialog.cpp @@ -51,6 +51,7 @@ SettingsDialog::SettingsDialog(QWidget* parent, AppConfig& config) : m_pCheckBoxAutoStart->setChecked(appConfig().getAutoStart()); m_pCheckBoxMinimizeToTray->setChecked(appConfig().getMinimizeToTray()); m_pCheckBoxEnableCrypto->setChecked(m_appConfig.getCryptoEnabled()); + checkbox_require_client_certificate->setChecked(m_appConfig.getRequireClientCertificate()); #if defined(Q_OS_WIN) m_pComboElevate->setCurrentIndex(static_cast(appConfig().elevateMode())); @@ -67,6 +68,7 @@ void SettingsDialog::accept() m_appConfig.setPort(m_pSpinBoxPort->value()); m_appConfig.setNetworkInterface(m_pLineEditInterface->text()); m_appConfig.setCryptoEnabled(m_pCheckBoxEnableCrypto->isChecked()); + m_appConfig.setRequireClientCertificate(checkbox_require_client_certificate->isChecked()); m_appConfig.setLogLevel(m_pComboLogLevel->currentIndex()); m_appConfig.setLogToFile(m_pCheckBoxLogToFile->isChecked()); m_appConfig.setLogFilename(m_pLineEditLogFilename->text()); diff --git a/src/gui/src/SettingsDialogBase.ui b/src/gui/src/SettingsDialogBase.ui index 719a84bc..44ed98cb 100644 --- a/src/gui/src/SettingsDialogBase.ui +++ b/src/gui/src/SettingsDialogBase.ui @@ -142,6 +142,16 @@ Networking + + + + &Address: + + + m_pLineEditInterface + + + @@ -171,16 +181,6 @@ - - - - &Address: - - - m_pLineEditInterface - - - @@ -188,13 +188,20 @@ - + Enable &SSL + + + + Require client certificate + + + @@ -216,19 +223,20 @@ false - - - - - 75 - 0 - + + + + Log to file: + + + + + + + false - &Logging level: - - - m_pComboLogLevel + Browse... @@ -271,10 +279,19 @@ - - + + + + + 75 + 0 + + - Log to file: + &Logging level: + + + m_pComboLogLevel @@ -285,16 +302,6 @@ - - - - false - - - Browse... - - - diff --git a/src/gui/src/SslCertificate.cpp b/src/gui/src/SslCertificate.cpp index 26f3b3ee..65ac08a6 100644 --- a/src/gui/src/SslCertificate.cpp +++ b/src/gui/src/SslCertificate.cpp @@ -65,6 +65,11 @@ void SslCertificate::generate_fingerprint(const barrier::fs::path& cert_path) { try { auto local_path = barrier::DataDirectories::local_ssl_fingerprints_path(); + auto local_dir = local_path.parent_path(); + if (!barrier::fs::exists(local_dir)) { + barrier::fs::create_directories(local_dir); + } + barrier::FingerprintDatabase db; db.add_trusted(barrier::get_pem_file_cert_fingerprint(cert_path.u8string(), barrier::FingerprintType::SHA1)); diff --git a/src/gui/src/ZeroconfService.cpp b/src/gui/src/ZeroconfService.cpp index f7b10007..fbb0ea10 100644 --- a/src/gui/src/ZeroconfService.cpp +++ b/src/gui/src/ZeroconfService.cpp @@ -66,7 +66,7 @@ ZeroconfService::ZeroconfService(MainWindow* mainWindow) : m_ServiceRegistered(false) { silence_avahi_warning(); - if (m_pMainWindow->barrierType() == MainWindow::barrierServer) { + if (m_pMainWindow->barrier_type() == BarrierType::Server) { if (registerService(true)) { m_pZeroconfBrowser = new ZeroconfBrowser(this); connect(m_pZeroconfBrowser, SIGNAL( 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/BarrierType.h b/src/lib/barrier/BarrierType.h new file mode 100644 index 00000000..0a184773 --- /dev/null +++ b/src/lib/barrier/BarrierType.h @@ -0,0 +1,26 @@ +/* + barrier -- mouse and keyboard sharing utility + Copyright (C) Barrier contributors + + This package is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + found in the file LICENSE that should have accompanied this file. + + This package is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . +*/ + +#ifndef BARRIER_LIB_BARRIER_BARRIER_TYPE_H +#define BARRIER_LIB_BARRIER_BARRIER_TYPE_H + +enum class BarrierType { + Server, + Client +}; + +#endif // BARRIER_LIB_BARRIER_BARRIER_TYPE_H diff --git a/src/lib/barrier/ServerApp.cpp b/src/lib/barrier/ServerApp.cpp index bbb35dd5..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" @@ -655,11 +658,18 @@ ServerApp::handleResume(const Event&, void*) 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()), - m_events, - args().m_enableCrypto); + m_events, security_level); m_events->adoptHandler( m_events->forClientListener().connected(), listen, 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 b0dbbc37..4f1fe73a 100644 --- a/src/lib/client/Client.cpp +++ b/src/lib/client/Client.cpp @@ -127,6 +127,12 @@ Client::connect() return; } + auto security_level = ConnectionSecurityLevel::PLAINTEXT; + if (m_useSecureNetwork) { + // 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 @@ -145,9 +151,8 @@ Client::connect() } // create the socket - IDataSocket* socket = m_socketFactory->create( - ARCH->getAddrFamily(m_serverAddress.getAddress()), - m_useSecureNetwork); + IDataSocket* socket = m_socketFactory->create(ARCH->getAddrFamily(m_serverAddress.getAddress()), + security_level); m_socket = dynamic_cast(socket); // filter socket messages, including a packetizing filter diff --git a/src/lib/net/ConnectionSecurityLevel.h b/src/lib/net/ConnectionSecurityLevel.h new file mode 100644 index 00000000..913c2fd7 --- /dev/null +++ b/src/lib/net/ConnectionSecurityLevel.h @@ -0,0 +1,27 @@ +/* + barrier -- mouse and keyboard sharing utility + Copyright (C) Barrier contributors + + This package is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + found in the file LICENSE that should have accompanied this file. + + This package is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . +*/ + +#ifndef BARRIER_LIB_NET_CONNECTION_SECURITY_LEVEL_H +#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/ISocketFactory.h b/src/lib/net/ISocketFactory.h index a98ddd4f..edfc8c90 100644 --- a/src/lib/net/ISocketFactory.h +++ b/src/lib/net/ISocketFactory.h @@ -20,6 +20,7 @@ #include "common/IInterface.h" #include "arch/IArchNetwork.h" +#include "net/ConnectionSecurityLevel.h" class IDataSocket; class IListenSocket; @@ -35,14 +36,12 @@ public: //@{ //! Create data socket - virtual IDataSocket* create( - IArchNetwork::EAddressFamily family, - bool secure) const = 0; + virtual IDataSocket* create(IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) const = 0; //! Create listen socket - virtual IListenSocket* createListen( - IArchNetwork::EAddressFamily family, - bool secure) const = 0; + virtual IListenSocket* createListen(IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) const = 0; //@} }; diff --git a/src/lib/net/SecureListenSocket.cpp b/src/lib/net/SecureListenSocket.cpp index 71e09ce5..27b84635 100644 --- a/src/lib/net/SecureListenSocket.cpp +++ b/src/lib/net/SecureListenSocket.cpp @@ -25,11 +25,11 @@ #include "common/DataDirectories.h" #include "base/String.h" -SecureListenSocket::SecureListenSocket( - IEventQueue* events, - SocketMultiplexer* socketMultiplexer, - IArchNetwork::EAddressFamily family) : - TCPListenSocket(events, socketMultiplexer, family) +SecureListenSocket::SecureListenSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, + IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) : + TCPListenSocket(events, socketMultiplexer, family), + security_level_{security_level} { } @@ -38,10 +38,8 @@ SecureListenSocket::accept() { SecureSocket* socket = NULL; try { - socket = new SecureSocket( - m_events, - m_socketMultiplexer, - ARCH->acceptSocket(m_socket, NULL)); + socket = new SecureSocket(m_events, m_socketMultiplexer, + ARCH->acceptSocket(m_socket, NULL), security_level_); socket->initSsl(true); if (socket != NULL) { diff --git a/src/lib/net/SecureListenSocket.h b/src/lib/net/SecureListenSocket.h index fab92bf5..a0e792ac 100644 --- a/src/lib/net/SecureListenSocket.h +++ b/src/lib/net/SecureListenSocket.h @@ -19,6 +19,7 @@ #include "net/TCPListenSocket.h" #include "common/stdset.h" +#include "ConnectionSecurityLevel.h" class IEventQueue; class SocketMultiplexer; @@ -26,11 +27,13 @@ class IDataSocket; class SecureListenSocket : public TCPListenSocket { public: - SecureListenSocket(IEventQueue* events, - SocketMultiplexer* socketMultiplexer, - IArchNetwork::EAddressFamily family); + SecureListenSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, + IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level); // IListenSocket overrides virtual IDataSocket* accept(); +private: + ConnectionSecurityLevel security_level_; }; diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 6a658db1..f096729d 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -54,25 +54,24 @@ struct Ssl { SSL* m_ssl; }; -SecureSocket::SecureSocket( - IEventQueue* events, - SocketMultiplexer* socketMultiplexer, - IArchNetwork::EAddressFamily family) : +SecureSocket::SecureSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, + IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) : TCPSocket(events, socketMultiplexer, family), m_ssl(nullptr), m_secureReady(false), - m_fatal(false) + m_fatal(false), + security_level_{security_level} { } -SecureSocket::SecureSocket( - IEventQueue* events, - SocketMultiplexer* socketMultiplexer, - ArchSocket socket) : +SecureSocket::SecureSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, + ArchSocket socket, ConnectionSecurityLevel security_level) : TCPSocket(events, socketMultiplexer, socket), m_ssl(nullptr), m_secureReady(false), - m_fatal(false) + m_fatal(false), + security_level_{security_level} { } @@ -362,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) { @@ -397,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 @@ -437,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) { @@ -462,6 +492,12 @@ SecureSocket::secureAccept(int socket) int SecureSocket::secureConnect(int socket) { + if (!load_certificates(barrier::DataDirectories::ssl_certificate_path())) { + LOG((CLOG_ERR "could not load client certificates")); + // FIXME: this is fatal error, but we current don't disconnect because whole logic in this + // function needs to be cleaned up + } + createSSL(); // attach the socket descriptor @@ -491,9 +527,9 @@ SecureSocket::secureConnect(int socket) retry = 0; // No error, set ready, process and return ok m_secureReady = true; - if (verifyCertFingerprint()) { + if (verify_cert_fingerprint(barrier::DataDirectories::trusted_servers_ssl_fingerprints_path())) { LOG((CLOG_INFO "connected to secure socket")); - if (!showCertificate()) { + if (!ensure_peer_certificate()) { disconnect(); return -1;// Cert fail, error } @@ -512,7 +548,7 @@ SecureSocket::secureConnect(int socket) } bool -SecureSocket::showCertificate() +SecureSocket::ensure_peer_certificate() { X509* cert; char* line; @@ -521,12 +557,12 @@ SecureSocket::showCertificate() cert = SSL_get_peer_certificate(m_ssl->m_ssl); if (cert != NULL) { line = X509_NAME_oneline(X509_get_subject_name(cert), 0, 0); - LOG((CLOG_INFO "server ssl certificate info: %s", line)); + LOG((CLOG_INFO "peer ssl certificate info: %s", line)); OPENSSL_free(line); X509_free(cert); } else { - showError("server has no ssl certificate"); + showError("peer has no ssl certificate"); return false; } @@ -649,8 +685,7 @@ SecureSocket::disconnect() sendEvent(getEvents()->forIStream().inputShutdown()); } -bool -SecureSocket::verifyCertFingerprint() +bool SecureSocket::verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path) { // calculate received certificate fingerprint barrier::FingerprintData fingerprint_sha1, fingerprint_sha256; @@ -666,12 +701,10 @@ SecureSocket::verifyCertFingerprint() } // 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())); - auto fingerprint_db_path = barrier::DataDirectories::trusted_servers_ssl_fingerprints_path(); - // 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())); diff --git a/src/lib/net/SecureSocket.h b/src/lib/net/SecureSocket.h index 6e355008..496a3656 100644 --- a/src/lib/net/SecureSocket.h +++ b/src/lib/net/SecureSocket.h @@ -17,6 +17,7 @@ #pragma once +#include "ConnectionSecurityLevel.h" #include "net/TCPSocket.h" #include "net/XSocket.h" #include "io/filesystem.h" @@ -33,10 +34,10 @@ A secure socket using SSL. */ class SecureSocket : public TCPSocket { public: - SecureSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, IArchNetwork::EAddressFamily family); - SecureSocket(IEventQueue* events, - SocketMultiplexer* socketMultiplexer, - ArchSocket socket); + SecureSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, + IArchNetwork::EAddressFamily family, ConnectionSecurityLevel security_level); + SecureSocket(IEventQueue* events, SocketMultiplexer* socketMultiplexer, + ArchSocket socket, ConnectionSecurityLevel security_level); ~SecureSocket(); // ISocket overrides @@ -64,12 +65,12 @@ private: void createSSL(); int secureAccept(int s); int secureConnect(int s); - bool showCertificate(); + bool ensure_peer_certificate(); void checkResult(int n, int& retry); void showError(const std::string& reason); std::string getError(); void disconnect(); - bool verifyCertFingerprint(); + bool verify_cert_fingerprint(const barrier::fs::path& fingerprint_db_path); MultiplexerJobStatus serviceConnect(ISocketMultiplexerJob*, bool, bool, bool); MultiplexerJobStatus serviceAccept(ISocketMultiplexerJob*, bool, bool, bool); @@ -86,4 +87,5 @@ private: Ssl* m_ssl; bool m_secureReady; bool m_fatal; + ConnectionSecurityLevel security_level_ = ConnectionSecurityLevel::ENCRYPTED; }; diff --git a/src/lib/net/TCPSocketFactory.cpp b/src/lib/net/TCPSocketFactory.cpp index fe24e97f..30e930e3 100644 --- a/src/lib/net/TCPSocketFactory.cpp +++ b/src/lib/net/TCPSocketFactory.cpp @@ -40,11 +40,12 @@ TCPSocketFactory::~TCPSocketFactory() // do nothing } -IDataSocket* -TCPSocketFactory::create(IArchNetwork::EAddressFamily family, bool secure) const +IDataSocket* TCPSocketFactory::create(IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) const { - if (secure) { - SecureSocket* secureSocket = new SecureSocket(m_events, m_socketMultiplexer, family); + if (security_level != ConnectionSecurityLevel::PLAINTEXT) { + SecureSocket* secureSocket = new SecureSocket(m_events, m_socketMultiplexer, family, + security_level); secureSocket->initSsl (false); return secureSocket; } @@ -53,12 +54,12 @@ TCPSocketFactory::create(IArchNetwork::EAddressFamily family, bool secure) const } } -IListenSocket* -TCPSocketFactory::createListen(IArchNetwork::EAddressFamily family, bool secure) const +IListenSocket* TCPSocketFactory::createListen(IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) const { IListenSocket* socket = NULL; - if (secure) { - socket = new SecureListenSocket(m_events, m_socketMultiplexer, family); + if (security_level != ConnectionSecurityLevel::PLAINTEXT) { + socket = new SecureListenSocket(m_events, m_socketMultiplexer, family, security_level); } else { socket = new TCPListenSocket(m_events, m_socketMultiplexer, family); diff --git a/src/lib/net/TCPSocketFactory.h b/src/lib/net/TCPSocketFactory.h index 202366e0..ac21cab0 100644 --- a/src/lib/net/TCPSocketFactory.h +++ b/src/lib/net/TCPSocketFactory.h @@ -31,12 +31,11 @@ public: virtual ~TCPSocketFactory(); // ISocketFactory overrides - virtual IDataSocket* create( - IArchNetwork::EAddressFamily family, - bool secure) const; - virtual IListenSocket* createListen( - IArchNetwork::EAddressFamily family, - bool secure) const; + virtual IDataSocket* create(IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) const; + + virtual IListenSocket* createListen(IArchNetwork::EAddressFamily family, + ConnectionSecurityLevel security_level) const; private: IEventQueue* m_events; diff --git a/src/lib/server/ClientListener.cpp b/src/lib/server/ClientListener.cpp index 04d8a596..a9b3a700 100644 --- a/src/lib/server/ClientListener.cpp +++ b/src/lib/server/ClientListener.cpp @@ -36,18 +36,17 @@ ClientListener::ClientListener(const NetworkAddress& address, ISocketFactory* socketFactory, IEventQueue* events, - bool enableCrypto) : + ConnectionSecurityLevel security_level) : m_socketFactory(socketFactory), m_server(NULL), m_events(events), - m_useSecureNetwork(enableCrypto) + security_level_{security_level} { assert(m_socketFactory != NULL); try { - m_listen = m_socketFactory->createListen( - ARCH->getAddrFamily(address.getAddress()), - m_useSecureNetwork); + m_listen = m_socketFactory->createListen(ARCH->getAddrFamily(address.getAddress()), + security_level); // setup event handler m_events->adoptHandler(m_events->forIListenSocket().connecting(), @@ -140,7 +139,7 @@ ClientListener::handleClientConnecting(const Event&, void*) // When using non SSL, server accepts clients immediately, while SSL // has to call secure accept which may require retry - if (!m_useSecureNetwork) { + if (security_level_ == ConnectionSecurityLevel::PLAINTEXT) { m_events->addEvent(Event(m_events->forClientListener().accepted(), socket->getEventTarget())); } diff --git a/src/lib/server/ClientListener.h b/src/lib/server/ClientListener.h index 86d962ef..1debc2b9 100644 --- a/src/lib/server/ClientListener.h +++ b/src/lib/server/ClientListener.h @@ -23,6 +23,7 @@ #include "base/Event.h" #include "common/stddeque.h" #include "common/stdset.h" +#include "net/ConnectionSecurityLevel.h" class ClientProxy; class ClientProxyUnknown; @@ -36,10 +37,8 @@ class IDataSocket; class ClientListener { public: // The factories are adopted. - ClientListener(const NetworkAddress&, - ISocketFactory*, - IEventQueue* events, - bool enableCrypto); + ClientListener(const NetworkAddress&, ISocketFactory*, IEventQueue* events, + ConnectionSecurityLevel security_level); ~ClientListener(); //! @name manipulators @@ -86,6 +85,6 @@ private: WaitingClients m_waitingClients; Server* m_server; IEventQueue* m_events; - bool m_useSecureNetwork; + ConnectionSecurityLevel security_level_; ClientSockets m_clientSockets; }; diff --git a/src/test/integtests/net/NetworkTests.cpp b/src/test/integtests/net/NetworkTests.cpp index 4bd19354..1e843867 100644 --- a/src/test/integtests/net/NetworkTests.cpp +++ b/src/test/integtests/net/NetworkTests.cpp @@ -115,7 +115,8 @@ TEST_F(NetworkTests, sendToClient_mockData) // server SocketMultiplexer serverSocketMultiplexer; TCPSocketFactory* serverSocketFactory = new TCPSocketFactory(&m_events, &serverSocketMultiplexer); - ClientListener listener(serverAddress, serverSocketFactory, &m_events, false); + ClientListener listener(serverAddress, serverSocketFactory, &m_events, + ConnectionSecurityLevel::PLAINTEXT); NiceMock serverScreen; NiceMock primaryClient; NiceMock serverConfig; @@ -173,7 +174,8 @@ TEST_F(NetworkTests, sendToClient_mockFile) // server SocketMultiplexer serverSocketMultiplexer; TCPSocketFactory* serverSocketFactory = new TCPSocketFactory(&m_events, &serverSocketMultiplexer); - ClientListener listener(serverAddress, serverSocketFactory, &m_events, false); + ClientListener listener(serverAddress, serverSocketFactory, &m_events, + ConnectionSecurityLevel::PLAINTEXT); NiceMock serverScreen; NiceMock primaryClient; NiceMock serverConfig; @@ -230,7 +232,8 @@ TEST_F(NetworkTests, sendToServer_mockData) // server SocketMultiplexer serverSocketMultiplexer; TCPSocketFactory* serverSocketFactory = new TCPSocketFactory(&m_events, &serverSocketMultiplexer); - ClientListener listener(serverAddress, serverSocketFactory, &m_events, false); + ClientListener listener(serverAddress, serverSocketFactory, &m_events, + ConnectionSecurityLevel::PLAINTEXT); NiceMock serverScreen; NiceMock primaryClient; NiceMock serverConfig; @@ -287,7 +290,8 @@ TEST_F(NetworkTests, sendToServer_mockFile) // server SocketMultiplexer serverSocketMultiplexer; TCPSocketFactory* serverSocketFactory = new TCPSocketFactory(&m_events, &serverSocketMultiplexer); - ClientListener listener(serverAddress, serverSocketFactory, &m_events, false); + ClientListener listener(serverAddress, serverSocketFactory, &m_events, + ConnectionSecurityLevel::PLAINTEXT); NiceMock serverScreen; NiceMock primaryClient; NiceMock serverConfig;