From dd974c684ab233e1dddeafa6d46ec6866f640756 Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Tue, 8 Sep 2026 16:18:57 -0600 Subject: [PATCH] RDP: enforce TLS certificate verification (was fully disabled) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IgnoreCertificate was hardcoded TRUE, meaning FreeRDP's entire certificate-verification pipeline was bypassed: every RDP server's TLS certificate was silently accepted, including certificates that had changed since a prior trusted connection to the same host. That is precisely the scenario TLS verification exists to catch — an active MITM presenting a different certificate was indistinguishable from a legitimate server. Switches to FreeRDP's own trust-on-first-use certificate store (AutoAcceptCertificate) so first-time connections still connect without a prompt, matching SSH's "accept-new" known-hosts policy. Certificate changes now correctly refuse the connection by default, via VerifyChangedCertificateEx, with a clear message (host, port, old/new SHA256 fingerprints) surfaced through the existing connection-failure event log rather than adding a new, redundant logging path. Also fixes CertificateCallbackPreferPEM, which handed the full PEM certificate to the verify callbacks instead of a short fingerprint — harmless while those callbacks were dead code, but would have flooded the event log with multi-KB certificate dumps once actually exercised. Verified end-to-end against real infrastructure: first connection trusts and stores the certificate silently, a simulated changed certificate (server key swapped) is correctly refused with a clear message, and restoring the original certificate reconnects normally. Co-Authored-By: Claude Sonnet 5 --- src/rdp_session_backend.cpp | 64 ++++++++++++++++++++++++++++++------- src/rdp_session_backend.h | 9 ++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/rdp_session_backend.cpp b/src/rdp_session_backend.cpp index e5d9048..f61724d 100644 --- a/src/rdp_session_backend.cpp +++ b/src/rdp_session_backend.cpp @@ -420,9 +420,19 @@ BOOL orbitPreConnect(freerdp* instance) } rdpSettings* settings = instance->context->settings; - freerdp_settings_set_bool(settings, FreeRDP_CertificateCallbackPreferPEM, TRUE); + // FALSE gives the verify callbacks a short SHA256 fingerprint string + // (e.g. "ab:cd:12:...") instead of the full PEM certificate blob, which + // is what actually belongs in a user-facing changed-certificate message. + freerdp_settings_set_bool(settings, FreeRDP_CertificateCallbackPreferPEM, FALSE); freerdp_settings_set_bool(settings, FreeRDP_DesktopResize, TRUE); - freerdp_settings_set_bool(settings, FreeRDP_IgnoreCertificate, TRUE); + // Trust servers on first connection (matching SSH's "accept-new" known-hosts + // policy) and persist that trust via FreeRDP's own certificate store, but + // actually verify it on every subsequent connection: IgnoreCertificate + // previously bypassed verification entirely, so a changed certificate + // (e.g. an active MITM) was silently accepted, indistinguishable from a + // legitimate server. See orbitVerifyChangedCertificateEx below. + freerdp_settings_set_bool(settings, FreeRDP_IgnoreCertificate, FALSE); + freerdp_settings_set_bool(settings, FreeRDP_AutoAcceptCertificate, TRUE); return TRUE; } @@ -708,10 +718,13 @@ DWORD orbitVerifyCertificateEx(freerdp* instance, const char*, DWORD) { + // With FreeRDP_AutoAcceptCertificate set, FreeRDP accepts and stores a + // first-seen certificate itself without calling this callback at all; + // it is kept as a defensive fallback for any code path that reaches it. if (instance != nullptr && instance->context != nullptr) { if (RdpSessionBackend* backend = backendFromContext(instance->context)) { emit backend->eventLogged( - QStringLiteral("Accepting server certificate for %1:%2.") + QStringLiteral("Trusting new server certificate for %1:%2.") .arg(QString::fromUtf8(host == nullptr ? "" : host)) .arg(port)); } @@ -725,21 +738,42 @@ DWORD orbitVerifyChangedCertificateEx(freerdp* instance, const char*, const char*, const char*, + const char* newFingerprint, const char*, const char*, - const char*, - const char*, + const char* oldFingerprint, DWORD) { + // The server's certificate no longer matches the one trusted on a prior + // connection. This is exactly the scenario TLS verification exists to + // catch: either the server legitimately rotated its certificate, or an + // active man-in-the-middle is presenting a different one. Refuse the + // connection by default rather than silently trusting it. + const QString hostStr = QString::fromUtf8(host == nullptr ? "" : host); + const QString message = QStringLiteral( + "Server certificate for %1:%2 has changed since it was last trusted. " + "Connection refused for safety — this could mean the server's " + "certificate was legitimately renewed, or that a different host is " + "impersonating it. Previously trusted fingerprint: %3 — now " + "presented: %4. If this change is expected, remove the stored entry " + "for this host from FreeRDP's certificate store " + "(~/.config/freerdp/server) and reconnect.") + .arg(hostStr) + .arg(port) + .arg(QString::fromUtf8(oldFingerprint == nullptr ? "unknown" : oldFingerprint)) + .arg(QString::fromUtf8(newFingerprint == nullptr ? "unknown" : newFingerprint)); + + // Not logged directly here: recordCertificateRejection() feeds this + // message into the generic connect-failure handling in workerMain(), + // which already reports it through eventLogged/connectionError/setState + // (the same channels every other RDP connection failure uses) — an + // extra direct log call here would just duplicate that. if (instance != nullptr && instance->context != nullptr) { if (RdpSessionBackend* backend = backendFromContext(instance->context)) { - emit backend->eventLogged( - QStringLiteral("Accepting changed server certificate for %1:%2.") - .arg(QString::fromUtf8(host == nullptr ? "" : host)) - .arg(port)); + backend->recordCertificateRejection(message); } } - return 1; + return 0; } const char* authReasonName(rdp_auth_reason reason) @@ -1451,6 +1485,11 @@ void RdpSessionBackend::setState(SessionState state, const QString& message) emit eventLogged(message); } +void RdpSessionBackend::recordCertificateRejection(const QString& reason) +{ + m_certificateRejectionReason = reason; +} + bool RdpSessionBackend::validateProfile(QString& message) const { const Profile& p = profile(); @@ -1510,6 +1549,7 @@ void RdpSessionBackend::workerMain() m_workerRunning.store(false); return; #else + m_certificateRejectionReason.clear(); ensureFreeRdpRuntimeInitialized(); freerdp* instance = freerdp_new(); @@ -1671,7 +1711,9 @@ void RdpSessionBackend::workerMain() emit eventLogged(QStringLiteral("RDP connect aborted: %1").arg(raw)); emitStateAsync(SessionState::Disconnected, disconnectMessageForCode(code)); } else { - const QString mapped = mapRdpError(code); + const QString mapped = m_certificateRejectionReason.isEmpty() + ? mapRdpError(code) + : m_certificateRejectionReason; emit eventLogged(QStringLiteral("RDP connect failure detail: %1").arg(raw)); emitConnectionFailureAsync(mapped, raw); emitStateAsync(SessionState::Failed, mapped); diff --git a/src/rdp_session_backend.h b/src/rdp_session_backend.h index 1153ad4..a52a371 100644 --- a/src/rdp_session_backend.h +++ b/src/rdp_session_backend.h @@ -90,6 +90,14 @@ private: void* m_cliprdrContext; QString m_pendingLocalClipboardText; + // Set synchronously by orbitVerifyChangedCertificateEx (called from this + // object's own worker thread during freerdp_connect) when a server's TLS + // certificate has changed since a prior trusted connection. Read back by + // workerMain() right after freerdp_connect() fails, to show the specific + // reason instead of a generic "TLS negotiation failed" message. Cleared + // at the start of every connect attempt. + QString m_certificateRejectionReason; + void setState(SessionState state, const QString& message); bool validateProfile(QString& message) const; void startWorker(); @@ -109,6 +117,7 @@ public: void onCliprdrServerFormatList(bool hasUnicodeText); void onCliprdrServerFormatDataRequest(uint32_t requestedFormatId); void onCliprdrServerFormatDataResponse(bool success, const uint8_t* data, uint32_t size); + void recordCertificateRejection(const QString& reason); private: void emitStateAsync(SessionState state, const QString& message); void emitConnectionFailureAsync(const QString& displayMessage, const QString& rawMessage);