From c0ce893711eb5e504e3bf6b376c84b4616d4241e Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:05 +0200 Subject: [PATCH 01/13] lib/net: Load client SSL certificates when connecting --- src/lib/net/SecureSocket.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 6a658db1..093dcb98 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -464,6 +464,8 @@ SecureSocket::secureConnect(int socket) { createSSL(); + load_certificates(barrier::DataDirectories::ssl_certificate_path()); + // attach the socket descriptor SSL_set_fd(m_ssl->m_ssl, socket); From 92ba6f61e69dcc88ef7cea4ed74bc42883f263ed Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:06 +0200 Subject: [PATCH 02/13] gui: Move SSL fingerprint labels out of server frame SSL fingerprints will be used to auth both server and client. --- src/gui/src/MainWindowBase.ui | 202 +++++++++++++++++----------------- 1 file changed, 101 insertions(+), 101 deletions(-) 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 + + + + + + From 4d73ed9fddedba63b91935799c6f7ae22d9ea989 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:07 +0200 Subject: [PATCH 03/13] lib/net: Present client certificate when connecting to server --- doc/newsfragments/client-send-certificate.feature | 1 + src/lib/net/SecureSocket.cpp | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 doc/newsfragments/client-send-certificate.feature 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/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 093dcb98..1004bb39 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -462,9 +462,13 @@ SecureSocket::secureAccept(int socket) int SecureSocket::secureConnect(int socket) { - createSSL(); + 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 + } - load_certificates(barrier::DataDirectories::ssl_certificate_path()); + createSSL(); // attach the socket descriptor SSL_set_fd(m_ssl->m_ssl, socket); From ed32e2e326fa6a28129a597b6cdc7d0b01122562 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:08 +0200 Subject: [PATCH 04/13] gui: Expand checkboxes in settings dialog through both grid columns --- src/gui/src/SettingsDialogBase.ui | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/gui/src/SettingsDialogBase.ui b/src/gui/src/SettingsDialogBase.ui index 719a84bc..144daf3c 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,7 +188,7 @@ - + Enable &SSL From 8bc280e0dd6dd95952cecb4b810b9f034de53927 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:09 +0200 Subject: [PATCH 05/13] gui: Add configuration for requiring client certificates --- src/gui/src/AppConfig.cpp | 7 ++++ src/gui/src/AppConfig.h | 4 +++ src/gui/src/SettingsDialog.cpp | 2 ++ src/gui/src/SettingsDialogBase.ui | 55 +++++++++++++++++-------------- 4 files changed, 44 insertions(+), 24 deletions(-) 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/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 144daf3c..44ed98cb 100644 --- a/src/gui/src/SettingsDialogBase.ui +++ b/src/gui/src/SettingsDialogBase.ui @@ -195,6 +195,13 @@ + + + + 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... - - - From 133e447fb67d558d52309da80bfa1b0d10e6173b Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:10 +0200 Subject: [PATCH 06/13] lib/net: Don't hardcode fingerprint DB path in verify_cert_fingerprint() --- src/lib/net/SecureSocket.cpp | 7 ++----- src/lib/net/SecureSocket.h | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 1004bb39..640740d1 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -497,7 +497,7 @@ 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()) { disconnect(); @@ -655,8 +655,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; @@ -676,8 +675,6 @@ SecureSocket::verifyCertFingerprint() 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..2e11097b 100644 --- a/src/lib/net/SecureSocket.h +++ b/src/lib/net/SecureSocket.h @@ -69,7 +69,7 @@ private: 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); From 82b8fa905eb31b0291bfe2ca4adffaa764292c4e Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:11 +0200 Subject: [PATCH 07/13] lib/net: Improve name of showCertificate() to reflect what it does --- src/lib/net/SecureSocket.cpp | 8 ++++---- src/lib/net/SecureSocket.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/net/SecureSocket.cpp b/src/lib/net/SecureSocket.cpp index 640740d1..2bb117ab 100644 --- a/src/lib/net/SecureSocket.cpp +++ b/src/lib/net/SecureSocket.cpp @@ -499,7 +499,7 @@ SecureSocket::secureConnect(int socket) m_secureReady = true; 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 } @@ -518,7 +518,7 @@ SecureSocket::secureConnect(int socket) } bool -SecureSocket::showCertificate() +SecureSocket::ensure_peer_certificate() { X509* cert; char* line; @@ -527,12 +527,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; } diff --git a/src/lib/net/SecureSocket.h b/src/lib/net/SecureSocket.h index 2e11097b..3c35b8e0 100644 --- a/src/lib/net/SecureSocket.h +++ b/src/lib/net/SecureSocket.h @@ -64,7 +64,7 @@ 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(); From 5c7d7194d5cb8589b2d12a89fa0c678887da51d4 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:12 +0200 Subject: [PATCH 08/13] lib/net: Use enum for connection security level instead of boolean --- src/lib/barrier/ServerApp.cpp | 8 ++++++-- src/lib/client/Client.cpp | 10 ++++++--- src/lib/net/ConnectionSecurityLevel.h | 26 ++++++++++++++++++++++++ src/lib/net/ISocketFactory.h | 11 +++++----- src/lib/net/TCPSocketFactory.cpp | 12 +++++------ src/lib/net/TCPSocketFactory.h | 11 +++++----- src/lib/server/ClientListener.cpp | 11 +++++----- src/lib/server/ClientListener.h | 9 ++++---- src/test/integtests/net/NetworkTests.cpp | 12 +++++++---- 9 files changed, 72 insertions(+), 38 deletions(-) create mode 100644 src/lib/net/ConnectionSecurityLevel.h diff --git a/src/lib/barrier/ServerApp.cpp b/src/lib/barrier/ServerApp.cpp index bbb35dd5..8870e0de 100644 --- a/src/lib/barrier/ServerApp.cpp +++ b/src/lib/barrier/ServerApp.cpp @@ -655,11 +655,15 @@ ServerApp::handleResume(const Event&, void*) ClientListener* ServerApp::openClientListener(const NetworkAddress& address) { + auto security_level = ConnectionSecurityLevel::PLAINTEXT; + if (args().m_enableCrypto) { + security_level = ConnectionSecurityLevel::ENCRYPTED; + } + 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/client/Client.cpp b/src/lib/client/Client.cpp index b0dbbc37..b000575c 100644 --- a/src/lib/client/Client.cpp +++ b/src/lib/client/Client.cpp @@ -127,6 +127,11 @@ Client::connect() return; } + auto security_level = ConnectionSecurityLevel::PLAINTEXT; + if (m_useSecureNetwork) { + security_level = ConnectionSecurityLevel::ENCRYPTED; + } + 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 +150,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..d597bd67 --- /dev/null +++ b/src/lib/net/ConnectionSecurityLevel.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_NET_CONNECTION_SECURITY_LEVEL_H +#define BARRIER_LIB_NET_CONNECTION_SECURITY_LEVEL_H + +enum class ConnectionSecurityLevel { + PLAINTEXT, + ENCRYPTED, +}; + +#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/TCPSocketFactory.cpp b/src/lib/net/TCPSocketFactory.cpp index fe24e97f..6f5f40be 100644 --- a/src/lib/net/TCPSocketFactory.cpp +++ b/src/lib/net/TCPSocketFactory.cpp @@ -40,10 +40,10 @@ 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) { + if (security_level != ConnectionSecurityLevel::PLAINTEXT) { SecureSocket* secureSocket = new SecureSocket(m_events, m_socketMultiplexer, family); secureSocket->initSsl (false); return secureSocket; @@ -53,11 +53,11 @@ 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) { + if (security_level != ConnectionSecurityLevel::PLAINTEXT) { socket = new SecureListenSocket(m_events, m_socketMultiplexer, family); } else { 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; From 57769cffdae571298671f828ee681c153f098d4d Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:13 +0200 Subject: [PATCH 09/13] lib/net: Pass connection security level to within socket classes --- src/lib/net/SecureListenSocket.cpp | 16 +++++++--------- src/lib/net/SecureListenSocket.h | 9 ++++++--- src/lib/net/SecureSocket.cpp | 19 +++++++++---------- src/lib/net/SecureSocket.h | 10 ++++++---- src/lib/net/TCPSocketFactory.cpp | 5 +++-- 5 files changed, 31 insertions(+), 28 deletions(-) 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 2bb117ab..41d6a7bb 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} { } diff --git a/src/lib/net/SecureSocket.h b/src/lib/net/SecureSocket.h index 3c35b8e0..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 @@ -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 6f5f40be..30e930e3 100644 --- a/src/lib/net/TCPSocketFactory.cpp +++ b/src/lib/net/TCPSocketFactory.cpp @@ -44,7 +44,8 @@ IDataSocket* TCPSocketFactory::create(IArchNetwork::EAddressFamily family, ConnectionSecurityLevel security_level) const { if (security_level != ConnectionSecurityLevel::PLAINTEXT) { - SecureSocket* secureSocket = new SecureSocket(m_events, m_socketMultiplexer, family); + SecureSocket* secureSocket = new SecureSocket(m_events, m_socketMultiplexer, family, + security_level); secureSocket->initSsl (false); return secureSocket; } @@ -58,7 +59,7 @@ IListenSocket* TCPSocketFactory::createListen(IArchNetwork::EAddressFamily famil { IListenSocket* socket = NULL; if (security_level != ConnectionSecurityLevel::PLAINTEXT) { - socket = new SecureListenSocket(m_events, m_socketMultiplexer, family); + socket = new SecureListenSocket(m_events, m_socketMultiplexer, family, security_level); } else { socket = new TCPListenSocket(m_events, m_socketMultiplexer, family); From e79bdf333c47952b9f18972f782bc3b4b75ad204 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:14 +0200 Subject: [PATCH 10/13] gui: Fix fingerprint database being not populated due to missing dirs --- src/gui/src/MainWindow.cpp | 5 +++++ src/gui/src/SslCertificate.cpp | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/src/gui/src/MainWindow.cpp b/src/gui/src/MainWindow.cpp index 87f31eb2..7d4a9a5e 100644 --- a/src/gui/src/MainWindow.cpp +++ b/src/gui/src/MainWindow.cpp @@ -444,6 +444,11 @@ void MainWindow::checkFingerprint(const QString& line) auto db_path = barrier::DataDirectories::trusted_servers_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 // to SHA1 vulnerabilities is when the user is reconnecting again. 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)); From 229abab99f39f11624e5651f819e7f1f8eddedcc Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:15 +0200 Subject: [PATCH 11/13] 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 . --- .../client-certificate-checking.bugfix | 7 +++ src/gui/src/MainWindow.cpp | 54 +++++++++++++++---- src/lib/barrier/ArgParser.cpp | 4 +- src/lib/barrier/ServerApp.cpp | 8 ++- src/lib/barrier/ServerArgs.h | 1 + src/lib/client/Client.cpp | 3 +- src/lib/net/ConnectionSecurityLevel.h | 1 + src/lib/net/SecureSocket.cpp | 33 +++++++++++- 8 files changed, 96 insertions(+), 15 deletions(-) create mode 100644 doc/newsfragments/client-certificate-checking.bugfix 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())); From 165100a0d2855fd7844e2e5d9f94267115ed0590 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:16 +0200 Subject: [PATCH 12/13] gui: Extract barrier type to separate enum --- src/gui/src/MainWindow.cpp | 15 ++++++++++----- src/gui/src/MainWindow.h | 10 +++------- src/gui/src/ZeroconfService.cpp | 2 +- src/lib/barrier/BarrierType.h | 26 ++++++++++++++++++++++++++ 4 files changed, 40 insertions(+), 13 deletions(-) create mode 100644 src/lib/barrier/BarrierType.h diff --git a/src/gui/src/MainWindow.cpp b/src/gui/src/MainWindow.cpp index 2172507b..85fa830e 100644 --- a/src/gui/src/MainWindow.cpp +++ b/src/gui/src/MainWindow.cpp @@ -442,7 +442,7 @@ void MainWindow::checkFingerprint(const QString& line) barrier::string::from_hex(fingerprintRegex.cap(2).toStdString()) }; - bool is_client = barrierType() == barrierClient; + bool is_client = barrier_type() == BarrierType::Client; auto db_path = is_client ? barrier::DataDirectories::trusted_servers_ssl_fingerprints_path() @@ -600,8 +600,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; @@ -616,7 +616,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; @@ -726,6 +726,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(); @@ -1020,7 +1025,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/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/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 From 7cacbd148941de596d8266385192489ec2764046 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Mon, 1 Nov 2021 04:50:17 +0200 Subject: [PATCH 13/13] gui: Improve formatting of the fingerprint acceptance dialog --- src/gui/CMakeLists.txt | 2 + src/gui/src/FingerprintAcceptDialog.cpp | 65 +++++++++ src/gui/src/FingerprintAcceptDialog.h | 45 ++++++ src/gui/src/FingerprintAcceptDialog.ui | 174 ++++++++++++++++++++++++ src/gui/src/MainWindow.cpp | 46 +------ 5 files changed, 289 insertions(+), 43 deletions(-) create mode 100644 src/gui/src/FingerprintAcceptDialog.cpp create mode 100644 src/gui/src/FingerprintAcceptDialog.h create mode 100644 src/gui/src/FingerprintAcceptDialog.ui 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/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 85fa830e..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" @@ -469,50 +470,9 @@ void MainWindow::checkFingerprint(const QString& line) stopBarrier(); } - 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 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" - "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))); - } 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) { + 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);