Bug 1402: Verify TLS certificate against cached fingerprints before Windows certificate store

https://winscp.net/tracker/1402

Source commit: a518c66fd9409e7577c3025904c7831ccafb1639

source/core/FtpFileSystem.cpp

@@ -4199,13 +4199,13 @@ bool __fastcall TFTPFileSystem::HandleAsynchRequestVerifyCertificate(
       UnicodeString CertificateSubject = Data.Subject.Organization;
       FTerminal->LogEvent(FORMAT(L"Verifying certificate for \"%s\" with fingerprint %s and %d failures", (CertificateSubject, FSessionInfo.CertificateFingerprint, Data.VerificationResult)));
 
-      bool VerificationResult = false;
+      bool Trusted = false;
       bool TryWindowsSystemCertificateStore = false;
       UnicodeString VerificationResultStr;
       switch (Data.VerificationResult)
       {
         case X509_V_OK:
-          VerificationResult = true;
+          Trusted = true;
           break;
         case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT:
           VerificationResultStr = LoadStr(CERT_ERR_UNABLE_TO_GET_ISSUER_CERT);
@@ -4277,35 +4277,62 @@ bool __fastcall TFTPFileSystem::HandleAsynchRequestVerifyCertificate(
           break;
       }
 
+      bool IsHostNameIPAddress = IsIPAddress(FTerminal->SessionData->HostNameExpanded);
+      bool CertificateHostNameVerified = !IsHostNameIPAddress && VerifyCertificateHostName(Data);
+
+      bool VerificationResult = Trusted;
+
+      if (IsHostNameIPAddress || !CertificateHostNameVerified)
+      {
+        VerificationResult = false;
+        TryWindowsSystemCertificateStore = false;
+      }
+
+      UnicodeString SiteKey = FTerminal->SessionData->SiteKey;
+
+      if (!VerificationResult)
+      {
+        if (FTerminal->VerifyCertificate(CertificateStorageKey, SiteKey,
+              FSessionInfo.CertificateFingerprint, CertificateSubject, Data.VerificationResult))
+        {
+          // certificate is trusted, but for not purposes of info dialog
+          VerificationResult = true;
+        }
+      }
+
       // TryWindowsSystemCertificateStore is set for the same set of failures
-      // as trigger NE_SSL_UNTRUSTED flag in ne_openssl.c's verify_callback()
+      // as trigger NE_SSL_UNTRUSTED flag in ne_openssl.c's verify_callback().
+      // Use WindowsValidateCertificate only as a last resort (after checking the cached fiungerprint)
+      // as it can take a very long time (up to 1 minute).
       if (!VerificationResult && TryWindowsSystemCertificateStore)
       {
         if (WindowsValidateCertificate(Data.Certificate, Data.CertificateLen))
         {
           FTerminal->LogEvent(L"Certificate verified against Windows certificate store");
           VerificationResult = true;
+          // certificate is trusted for all purposes
+          Trusted = true;
         }
       }
 
+      const UnicodeString SummarySeparator = L"\n\n";
       UnicodeString Summary;
-      if (!VerificationResult)
+      // even if the fingerprint is cached, the certificate is still not trusted for a purposes of the info dialog.
+      if (!Trusted)
       {
-        Summary = VerificationResultStr + L" " + FMTLOAD(CERT_ERRDEPTH, (Data.VerificationDepth + 1));
+        AddToList(Summary, VerificationResultStr + L" " + FMTLOAD(CERT_ERRDEPTH, (Data.VerificationDepth + 1)), SummarySeparator);
       }
 
-      if (IsIPAddress(FTerminal->SessionData->HostNameExpanded))
+      if (IsHostNameIPAddress)
       {
-        VerificationResult = false;
-        AddToList(Summary, FMTLOAD(CERT_IP_CANNOT_VERIFY, (FTerminal->SessionData->HostNameExpanded)), L"\n\n");
+        AddToList(Summary, FMTLOAD(CERT_IP_CANNOT_VERIFY, (FTerminal->SessionData->HostNameExpanded)), SummarySeparator);
       }
-      else if (!VerifyCertificateHostName(Data))
+      else if (!CertificateHostNameVerified)
       {
-        VerificationResult = false;
-        AddToList(Summary, FMTLOAD(CERT_NAME_MISMATCH, (FTerminal->SessionData->HostNameExpanded)), L"\n\n");
+        AddToList(Summary, FMTLOAD(CERT_NAME_MISMATCH, (FTerminal->SessionData->HostNameExpanded)), SummarySeparator);
       }
 
-      if (VerificationResult)
+      if (Summary.IsEmpty())
       {
         Summary = LoadStr(CERT_OK);
       }
@@ -4319,22 +4346,7 @@ bool __fastcall TFTPFileSystem::HandleAsynchRequestVerifyCertificate(
           FSessionInfo.CertificateFingerprint,
           Summary));
 
-      RequestResult = 0;
-
-      if (VerificationResult)
-      {
-        RequestResult = 1;
-      }
-
-      UnicodeString SiteKey = FTerminal->SessionData->SiteKey;
-      if (RequestResult == 0)
-      {
-        if (FTerminal->VerifyCertificate(CertificateStorageKey, SiteKey,
-              FSessionInfo.CertificateFingerprint, CertificateSubject, Data.VerificationResult))
-        {
-          RequestResult = 1;
-        }
-      }
+      RequestResult = VerificationResult ? 1 : 0;
 
       if (RequestResult == 0)
       {
@@ -4386,7 +4398,7 @@ bool __fastcall TFTPFileSystem::HandleAsynchRequestVerifyCertificate(
         }
       }
 
-      // Cache only if the certificate was not automatically accepted
+      // Cache only if the certificate was accepted manually
       if (!VerificationResult && (RequestResult != 0))
       {
         FTerminal->Configuration->RememberLastFingerprint(

source/core/WebDAVFileSystem.cpp

@@ -2239,13 +2239,22 @@ bool TWebDAVFileSystem::VerifyCertificate(const TWebDAVCertificateData & Data)
         (Data.Subject, Data.Fingerprint, Data.Failures)));
 
     int Failures = Data.Failures;
-    if (NeonWindowsValidateCertificate(Failures, Data.AsciiCert))
+
+    UnicodeString SiteKey = TSessionData::FormatSiteKey(FHostName, FPortNumber);
+    Result =
+      FTerminal->VerifyCertificate(CertificateStorageKey, SiteKey, Data.Fingerprint, Data.Subject, Failures);
+
+    if (!Result)
     {
-      FTerminal->LogEvent(L"Certificate verified against Windows certificate store");
+      if (NeonWindowsValidateCertificate(Failures, Data.AsciiCert))
+      {
+        FTerminal->LogEvent(L"Certificate verified against Windows certificate store");
+        // There can be also other flags, not just the NE_SSL_UNTRUSTED.
+        Result = (Failures == 0);
+      }
     }
 
     UnicodeString Summary;
-
     if (Failures == 0)
     {
       Summary = LoadStr(CERT_OK);
@@ -2265,53 +2274,41 @@ bool TWebDAVFileSystem::VerifyCertificate(const TWebDAVCertificateData & Data)
         Data.Fingerprint,
         Summary));
 
-    Result = (Failures == 0);
-
     if (!Result)
     {
-      UnicodeString SiteKey = TSessionData::FormatSiteKey(FHostName, FPortNumber);
-      if (!Result)
-      {
-        Result = FTerminal->VerifyCertificate(
-          CertificateStorageKey, SiteKey, Data.Fingerprint, Data.Subject, Failures);
-      }
-
-      if (!Result)
+      TClipboardHandler ClipboardHandler;
+      ClipboardHandler.Text = Data.Fingerprint;
+
+      TQueryButtonAlias Aliases[1];
+      Aliases[0].Button = qaRetry;
+      Aliases[0].Alias = LoadStr(COPY_KEY_BUTTON);
+      Aliases[0].OnClick = &ClipboardHandler.Copy;
+
+      TQueryParams Params;
+      Params.HelpKeyword = HELP_VERIFY_CERTIFICATE;
+      Params.NoBatchAnswers = qaYes | qaRetry;
+      Params.Aliases = Aliases;
+      Params.AliasesCount = LENOF(Aliases);
+      unsigned int Answer = FTerminal->QueryUser(
+        FMTLOAD(VERIFY_CERT_PROMPT3, (FSessionInfo.Certificate)),
+        NULL, qaYes | qaNo | qaCancel | qaRetry, &Params, qtWarning);
+      switch (Answer)
       {
-        TClipboardHandler ClipboardHandler;
-        ClipboardHandler.Text = Data.Fingerprint;
-
-        TQueryButtonAlias Aliases[1];
-        Aliases[0].Button = qaRetry;
-        Aliases[0].Alias = LoadStr(COPY_KEY_BUTTON);
-        Aliases[0].OnClick = &ClipboardHandler.Copy;
-
-        TQueryParams Params;
-        Params.HelpKeyword = HELP_VERIFY_CERTIFICATE;
-        Params.NoBatchAnswers = qaYes | qaRetry;
-        Params.Aliases = Aliases;
-        Params.AliasesCount = LENOF(Aliases);
-        unsigned int Answer = FTerminal->QueryUser(
-          FMTLOAD(VERIFY_CERT_PROMPT3, (FSessionInfo.Certificate)),
-          NULL, qaYes | qaNo | qaCancel | qaRetry, &Params, qtWarning);
-        switch (Answer)
-        {
-          case qaYes:
-            FTerminal->CacheCertificate(CertificateStorageKey, SiteKey, Data.Fingerprint, Failures);
-            Result = true;
-            break;
-
-          case qaNo:
-            Result = true;
-            break;
-
-          default:
-            DebugFail();
-          case qaCancel:
-            FTerminal->Configuration->Usage->Inc(L"HostNotVerified");
-            Result = false;
-            break;
-        }
+        case qaYes:
+          FTerminal->CacheCertificate(CertificateStorageKey, SiteKey, Data.Fingerprint, Failures);
+          Result = true;
+          break;
+
+        case qaNo:
+          Result = true;
+          break;
+
+        default:
+          DebugFail();
+        case qaCancel:
+          FTerminal->Configuration->Usage->Inc(L"HostNotVerified");
+          Result = false;
+          break;
       }
 
       if (Result)