From 7dea132a87d532b18dbd0b4ecba547084f74cde4 Mon Sep 17 00:00:00 2001 From: Martchus Date: Sun, 29 Mar 2020 20:38:05 +0200 Subject: [PATCH] Check whether self-signed certificate actually matches the expected one This concerned only the built-in web view using Qt WebEngine. This change has only effect when using Qt >= 5.14 because the API did not expose the certificate chain before. --- README.md | 21 ++++++++++++-------- widgets/webview/webpage.cpp | 39 ++++++++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 7c0da70..2a29bf4 100644 --- a/README.md +++ b/README.md @@ -196,7 +196,9 @@ can be passed to CMake to influence the build. ### Further dependencies The following Qt 5 modules are requried (version 5.6 or newer): core network dbus gui widgets svg webenginewidgets/webkitwidgets -The built-in web view is optional (see section "Select Qt module for WebView"). +It is recommended to use at least Qt 5.14 to avoid limitations in previous versions (see *Known bugs* section). + +The built-in web view and therefore the modules webenginewidgets/webkitwidgets are optional (see section *Select Qt module for WebView*). To build the plugin for Dolphin integration KIO is also requried. Additionally, the Dolphin plugin requires Qt 5.8 or newer. To skip building the plugin, add `-DNO_FILE_ITEM_ACTION_PLUGIN:BOOL=ON` to the CMake arguments. @@ -351,13 +353,16 @@ on GitHub. Note that the Plasmoid is not affected by this limitation. * While the tray menu is shown its entry is shown in the taskbar. Not sure whether there is a way to avoid this. * Qt bugs - * Any self-signed certificate is accepted when using Qt WebEngine due to Qt bug https://bugreports.qt.io/browse/QTBUG-51176. - * Pausing/resuming folders and devices doesn't work when using scan-intervalls with a lot of zeros because of Syncthing bug - https://github.com/syncthing/syncthing/issues/4001. This has already been fixed on the Qt-side with - https://codereview.qt-project.org/#/c/187069/. However, the fix is only available in Qt 5.9 and above. - * The tray disconnects from the local instance when the network connection goes down. The network connection must be restored or - the tray restarted to be able to connect to local Syncthing again. This is caused by Qt bug - https://bugreports.qt.io/browse/QTBUG-60949. + * Qt < 5.14 + * Any self-signed certificate is accepted when using Qt WebEngine due to https://bugreports.qt.io/browse/QTBUG-51176. + * Qt < 5.9: + * Pausing/resuming folders and devices doesn't work when using scan-intervalls with a lot of zeros because of + Syncthing bug https://github.com/syncthing/syncthing/issues/4001. This has already been fixed on the Qt-side with + https://codereview.qt-project.org/#/c/187069/. However, the fix is only available in Qt 5.9 and above. + * any Qt version: + * The tray disconnects from the local instance when the network connection goes down. The network connection must be restored + or the tray restarted to be able to connect to local Syncthing again. This is caused by Qt bug + https://bugreports.qt.io/browse/QTBUG-60949. ## Attribution for 3rd party content * Some icons are taken from the Syncthing project. diff --git a/widgets/webview/webpage.cpp b/widgets/webview/webpage.cpp index 1c5229c..62fb69a 100644 --- a/widgets/webview/webpage.cpp +++ b/widgets/webview/webpage.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #elif defined(SYNCTHINGWIDGETS_USE_WEBKIT) #include #include @@ -101,18 +102,50 @@ SYNCTHINGWIDGETS_WEB_PAGE *WebPage::createWindow(SYNCTHINGWIDGETS_WEB_PAGE::WebW #ifdef SYNCTHINGWIDGETS_USE_WEBENGINE /*! - * \brief Accepts self-signed certificate. - * \todo Only ignore the error if the used certificate matches the certificate known to be used by the Syncthing GUI. + * \brief Accepts self-signed certificates used by the Syncthing GUI as configured. + * \remarks Before Qt 5.14 any self-signed certificates are accepted. */ bool WebPage::certificateError(const QWebEngineCertificateError &certificateError) { + // never ignore errors other than CertificateCommonNameInvalid and CertificateAuthorityInvalid switch (certificateError.error()) { case QWebEngineCertificateError::CertificateCommonNameInvalid: case QWebEngineCertificateError::CertificateAuthorityInvalid: - return true; + break; default: return false; } + + // don't ignore the error if there are no expected self-signed SSL certificates configured + if (!m_dlg || m_dlg->connectionSettings().expectedSslErrors.isEmpty()) { + return false; + } + + // ignore only certificate errors matching the expected URL of the Syncthing instance + const auto urlWithError = certificateError.url(); + const auto expectedUrl = m_view->url(); + if (urlWithError.scheme() != expectedUrl.scheme() || urlWithError.host() != expectedUrl.host()) { + return false; + } + +#if (QTWEBENGINEWIDGETS_VERSION >= QT_VERSION_CHECK(5, 14, 0)) + // don't ignore the error if no certificate is provided at all (possible?) + const auto certificateChain = certificateError.certificateChain(); + if (certificateChain.isEmpty()) { + return false; + } + + // don't ignore the error if the first certificate in the chain (the peer's immediate certificate) does + // not match the expected SSL certificate + // note: All the SSL errors in the settings refer to the same certificate so it is sufficient to just pick + // the first one. + if (certificateChain.first() != m_dlg->connectionSettings().expectedSslErrors.first().certificate()) { + return false; + } +#endif + + // accept the self-signed certificate + return true; } /*!