Issue 2327 – Failure when opening TLS connection with invalid OpenSSL configuration file

https://winscp.net/tracker/2327

Source commit: ee33d2badfff95c721fe412c86e7b305674e6703

source/core/Common.cpp

@@ -3524,12 +3524,35 @@ UnicodeString __fastcall FindIdent(const UnicodeString & Ident, TStrings * Ident
   return Ident;
 }
 //---------------------------------------------------------------------------
-static UnicodeString __fastcall GetTlsErrorStr(int Err)
+UnicodeString GetTlsErrorStr(unsigned long Err)
 {
-  char * Buffer = new char[512];
-  ERR_error_string(Err, Buffer);
-  // not sure about the UTF8
-  return UnicodeString(UTF8String(Buffer));
+  char Buffer[512];
+  ERR_error_string_n(Err, Buffer, sizeof(Buffer));
+  UnicodeString S = UnicodeString(UTF8String(Buffer));
+  for (int I = 0; I < 4; I++)
+  {
+    CutToChar(S, L':', false);
+  }
+  UnicodeString ErrStr = IntToHex(static_cast<unsigned int>(Err));
+  return FORMAT(L"OpenSSL %s: %s", (ErrStr, S.TrimRight()));
+}
+//---------------------------------------------------------------------------
+UnicodeString GetTlsErrorStrs()
+{
+  UnicodeString Result;
+  int Error;
+  const char * Data;
+  while ((Error = ERR_get_error_all(NULL, NULL, NULL, &Data, NULL)) != 0)
+  {
+    UnicodeString S = GetTlsErrorStr(Error);
+    if ((Data != NULL) && (strlen(Data) > 0))
+    {
+      UnicodeString DataStr = UnicodeString(UTF8String(Data)).TrimRight();
+      S += FORMAT(L" (%s)", (DataStr));
+    }
+    AddToList(Result, S, L"\n");
+  }
+  return Result;
 }
 //---------------------------------------------------------------------------
 static FILE * __fastcall OpenCertificate(const UnicodeString & Path)
@@ -3559,7 +3582,7 @@ static int PemPasswordCallback(char * Buf, int Size, int /*RWFlag*/, void * User
   return strlen(Buf);
 }
 //---------------------------------------------------------------------------
-static bool __fastcall IsTlsPassphraseError(int Error, bool HasPassphrase)
+static bool __fastcall IsTlsPassphraseError(unsigned long Error, bool HasPassphrase)
 {
   int ErrorLib = ERR_GET_LIB(Error);
   int ErrorReason = ERR_GET_REASON(Error);
@@ -3577,7 +3600,7 @@ static bool __fastcall IsTlsPassphraseError(int Error, bool HasPassphrase)
 //---------------------------------------------------------------------------
 static void __fastcall ThrowTlsCertificateErrorIgnorePassphraseErrors(const UnicodeString & Path, bool HasPassphrase)
 {
-  int Error = ERR_get_error();
+  unsigned long Error = ERR_get_error();
   if (!IsTlsPassphraseError(Error, HasPassphrase))
   {
     throw ExtException(MainInstructions(FMTLOAD(CERTIFICATE_READ_ERROR, (Path))), GetTlsErrorStr(Error));
@@ -3664,7 +3687,7 @@ void __fastcall ParseCertificate(const UnicodeString & Path,
 
       if (Certificate == NULL)
       {
-        int Error = ERR_get_error();
+        unsigned long Error = ERR_get_error();
         // unlikely
         if (IsTlsPassphraseError(Error, HasPassphrase))
         {
@@ -3693,7 +3716,7 @@ void __fastcall ParseCertificate(const UnicodeString & Path,
 
             if (Certificate == NULL)
             {
-              int Base64Error = ERR_get_error();
+              unsigned long Base64Error = ERR_get_error();
 
               File = OpenCertificate(CertificatePath);
               // Binary DER-encoded certificate
@@ -3704,7 +3727,7 @@ void __fastcall ParseCertificate(const UnicodeString & Path,
 
               if (Certificate == NULL)
               {
-                int DERError = ERR_get_error();
+                unsigned long DERError = ERR_get_error();
 
                 UnicodeString Message = MainInstructions(FMTLOAD(CERTIFICATE_READ_ERROR, (CertificatePath)));
                 UnicodeString MoreMessages =

source/core/Common.h

@@ -185,6 +185,8 @@ int __fastcall ParseShortEngMonthName(const UnicodeString & MonthStr);
 TStringList * __fastcall CreateSortedStringList(bool CaseSensitive = false, System::Types::TDuplicates Duplicates = dupIgnore);
 bool SameIdent(const UnicodeString & Ident1, const UnicodeString & Ident2);
 UnicodeString __fastcall FindIdent(const UnicodeString & Ident, TStrings * Idents);
+UnicodeString GetTlsErrorStr(unsigned long Err);
+UnicodeString GetTlsErrorStrs();
 void __fastcall CheckCertificate(const UnicodeString & Path);
 typedef struct x509_st X509;
 typedef struct evp_pkey_st EVP_PKEY;

source/core/Cryptography.cpp

@@ -7,7 +7,11 @@
 #include "Cryptography.h"
 #include "FileBuffer.h"
 #include "TextsCore.h"
+#include "CoreMain.h"
+#include "Exceptions.h"
 #include <openssl\rand.h>
+#include <openssl\err.h>
+#include <openssl\ssl.h>
 #include <process.h>
 #include <Soap.EncdDecd.hpp>
 #include <System.StrUtils.hpp>
@@ -600,6 +604,19 @@ bool __fastcall UnscramblePassword(RawByteString Scrambled, UnicodeString & Pass
   return Result;
 }
 //---------------------------------------------------------------------------
+UnicodeString OpensslInitializationErrors;
+//---------------------------------------------------------------------------
+static bool InitOpenssl()
+{
+  // RAND_poll already calls OPENSSL_init_crypto with OPENSSL_INIT_LOAD_CONFIG and other flags.
+  // OPENSSL_init_ssl does not do much more, so it is not really big overhead.
+  // And we need to call OPENSSL_init_ssl, as it we need to use OPENSSL_INIT_LOAD_SSL_STRINGS to match what SSL_CTX_new does
+  // OPENSSL_init_ssl passes all flags to OPENSSL_init_crypto (even those it does not understand, like the very  OPENSSL_INIT_LOAD_SSL_STRINGS).
+  // And OPENSSL_init_ssl caches the initialization results based on the flags
+  ERR_clear_error();
+  return OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS, NULL);
+}
+//---------------------------------------------------------------------------
 void __fastcall CryptographyInitialize()
 {
   ScrambleTable = SScrambleTable;
@@ -609,6 +626,39 @@ void __fastcall CryptographyInitialize()
     UnscrambleTable[SScrambleTable[Index]] = (unsigned char)Index;
   }
   srand((unsigned int)time(NULL) ^ (unsigned int)getpid());
+
+  // The results are partly cached. So when later some OpenSSL function is called, which internally
+  // calls OPENSSL_init_crypto, it will fail, without doing full initialization. So afterwards
+  // the ERR_get_error might return 0, even if the function itself returns failure.
+  // So we have to remember here what went wrong.
+  // But there seems to be an issue in OpenSSL that when OPENSSL_init_crypto is called again
+  // with OPENSSL_INIT_LOAD_CONFIG, after the loading failed before, _from the same thread_, the
+  // initialization succeeds. It seems to be because the in_init_config_local recursion fuest is never cleared.
+  // So for example, is the configuration is invalid, the foreground updates check still work,
+  // as the foreground thread always initialized OpenSSL here (via RAND_poll), and the later
+  // OPENSSL_init_crypto from withing updates TLS code succeeds. Similarly scripting TLS connections work.
+  // But opening GUI TLS connections (WebDAV, S3...) fail, as they are opened on background thread,
+  // and there the OPENSSL_init_crypto is first called from TLS connection.
+  // Clean solution would be to fail any TLS connection,
+  // if OPENSSL_init_crypto failed when called the firts time, but that would be regression.
+  // But let's be prepared that this happens if OpenSSL is ever fixed.
+  if (!InitOpenssl())
+  {
+    OpensslInitializationErrors = GetTlsErrorStrs();
+    AppLogFmt(L"OpenSSL initialization failed (possibly wrong configuration file) - TLS connections might be failing:\n%s", (OpensslInitializationErrors));
+    char * ConfigPathBuf = CONF_get1_default_config_file();
+    UnicodeString ConfigPath = UnicodeString(UTF8String(ConfigPathBuf));
+    if (!ConfigPath.IsEmpty() && FileExists(ApiPath(ConfigPath)))
+    {
+      AppLogFmt(L"OpenSSL configuration file: %s", (ConfigPath));
+    }
+    OPENSSL_free(ConfigPathBuf);
+  }
+  else
+  {
+    AppLog(L"OpenSSL initialization succeeded");
+  }
+
   RAND_poll();
 }
 //---------------------------------------------------------------------------
@@ -619,6 +669,15 @@ void __fastcall CryptographyFinalize()
   ScrambleTable = NULL;
 }
 //---------------------------------------------------------------------------
+void RequireTls()
+{
+  if (!InitOpenssl())
+  {
+    UnicodeString Errors = DefaultStr(GetTlsErrorStrs(), OpensslInitializationErrors);
+    throw ExtException(MainInstructions(LoadStr(OPENSSL_INIT_ERROR)), OpensslInitializationErrors);
+  }
+}
+//---------------------------------------------------------------------------
 int __fastcall PasswordMaxLength()
 {
   return 128;

source/core/Cryptography.h

@@ -4,6 +4,7 @@
 //---------------------------------------------------------------------------
 void __fastcall CryptographyInitialize();
 void __fastcall CryptographyFinalize();
+void RequireTls();
 RawByteString __fastcall ScramblePassword(UnicodeString Password);
 bool __fastcall UnscramblePassword(RawByteString Scrambled, UnicodeString & Password);
 void __fastcall AES256EncyptWithMAC(RawByteString Input, UnicodeString Password,

source/core/FtpFileSystem.cpp

@@ -17,6 +17,7 @@
 #include "Security.h"
 #include "NeonIntf.h"
 #include "SessionInfo.h"
+#include "Cryptography.h"
 #include <StrUtils.hpp>
 #include <DateUtils.hpp>
 #include <openssl/x509_vfy.h>
@@ -415,30 +416,35 @@ void __fastcall TFTPFileSystem::Open()
   UnicodeString Account = Data->FtpAccount;
   UnicodeString Path = Data->RemoteDirectory;
   int ServerType;
-  switch (Data->Ftps)
+  if (Data->Ftps == ftpsNone)
   {
-    case ftpsNone:
-      ServerType = TFileZillaIntf::SERVER_FTP;
-      break;
+    ServerType = TFileZillaIntf::SERVER_FTP;
+  }
+  else
+  {
+    switch (Data->Ftps)
+    {
+      case ftpsImplicit:
+        ServerType = TFileZillaIntf::SERVER_FTP_SSL_IMPLICIT;
+        FSessionInfo.SecurityProtocolName = LoadStr(FTPS_IMPLICIT);
+        break;
 
-    case ftpsImplicit:
-      ServerType = TFileZillaIntf::SERVER_FTP_SSL_IMPLICIT;
-      FSessionInfo.SecurityProtocolName = LoadStr(FTPS_IMPLICIT);
-      break;
+      case ftpsExplicitSsl:
+        ServerType = TFileZillaIntf::SERVER_FTP_SSL_EXPLICIT;
+        FSessionInfo.SecurityProtocolName = LoadStr(FTPS_EXPLICIT);
+        break;
 
-    case ftpsExplicitSsl:
-      ServerType = TFileZillaIntf::SERVER_FTP_SSL_EXPLICIT;
-      FSessionInfo.SecurityProtocolName = LoadStr(FTPS_EXPLICIT);
-      break;
+      case ftpsExplicitTls:
+        ServerType = TFileZillaIntf::SERVER_FTP_TLS_EXPLICIT;
+        FSessionInfo.SecurityProtocolName = LoadStr(FTPS_EXPLICIT);
+        break;
 
-    case ftpsExplicitTls:
-      ServerType = TFileZillaIntf::SERVER_FTP_TLS_EXPLICIT;
-      FSessionInfo.SecurityProtocolName = LoadStr(FTPS_EXPLICIT);
-      break;
+      default:
+        DebugFail();
+        break;
+    }
 
-    default:
-      DebugFail();
-      break;
+    RequireTls();
   }
 
   int Pasv = (Data->FtpPasvMode ? 1 : 2);

source/core/NeonIntf.cpp

@@ -8,6 +8,7 @@
 #include "Exceptions.h"
 #include "Security.h"
 #include "Terminal.h"
+#include "Cryptography.h"
 #include <TextsCore.h>
 #define WINSCP
 extern "C"
@@ -73,6 +74,10 @@ static int NeonProxyAuth(
 //---------------------------------------------------------------------------
 ne_session * CreateNeonSession(const ne_uri & uri)
 {
+  if (IsTlsUri(uri))
+  {
+    RequireTls();
+  }
   return ne_session_create(uri.scheme, uri.host, uri.port);
 }
 //---------------------------------------------------------------------------

source/core/S3FileSystem.cpp

@@ -21,6 +21,7 @@
 #include <limits>
 #include "CoreMain.h"
 #include "Http.h"
+#include "Cryptography.h"
 #include <System.JSON.hpp>
 #include <System.DateUtils.hpp>
 #include "request.h"
@@ -447,7 +448,15 @@ void __fastcall TS3FileSystem::Open()
   FSessionInfo.LoginTime = Now();
   FSessionInfo.CertificateVerifiedManually = false;
 
-  FLibS3Protocol = (Data->Ftps != ftpsNone) ? S3ProtocolHTTPS : S3ProtocolHTTP;
+  if (Data->Ftps != ftpsNone)
+  {
+    FLibS3Protocol = S3ProtocolHTTPS;
+    RequireTls();
+  }
+  else
+  {
+    FLibS3Protocol = S3ProtocolHTTP;
+  }
 
   UnicodeString S3Profile;
   if (Data->S3CredentialsEnv)

source/core/WebDAVFileSystem.cpp

@@ -457,6 +457,11 @@ void TWebDAVFileSystem::ExchangeCapabilities(const char * Path, UnicodeString &
   FTerminal->SaveCapabilities(FFileSystemInfo);
 }
 //---------------------------------------------------------------------------
+TWebDAVFileSystem::TSessionContext::TSessionContext() :
+  NeonSession(NULL)
+{
+}
+//---------------------------------------------------------------------------
 TWebDAVFileSystem::TSessionContext::~TSessionContext()
 {
   if (NeonSession != NULL)

source/core/WebDAVFileSystem.h

@@ -164,6 +164,7 @@ private:
   unsigned int FCapabilities;
   struct TSessionContext
   {
+    TSessionContext();
     ~TSessionContext();
     TWebDAVFileSystem * FileSystem;
     ne_session_s * NeonSession; // The main one (there might be aux session for the same context)

source/filezilla/AsyncSslSocketLayer.cpp

@@ -13,6 +13,8 @@
 #include <openssl/x509v3.h>
 #include <openssl/err.h>
 #include <openssl/tls1.h>
+#pragma hdrstop
+#include "Common.h"
 
 /////////////////////////////////////////////////////////////////////////////
 // CAsyncSslSocketLayer
@@ -1836,12 +1838,12 @@ void CAsyncSslSocketLayer::OnClose(int nErrorCode)
 
 void CAsyncSslSocketLayer::PrintLastErrorMsg()
 {
-  int err = ERR_get_error();
+  unsigned long err = ERR_get_error();
   while (err)
   {
     USES_CONVERSION;
 
-    int aerr = err;
+    unsigned long aerr = err;
     err = ERR_get_error();
 
     char *buffer = new char[512];
@@ -1856,8 +1858,8 @@ void CAsyncSslSocketLayer::PrintLastErrorMsg()
     }
     else
     {
-      const char * reason = ERR_reason_error_string(aerr);
-      LogSocketMessageRaw(FZ_LOG_WARNING, A2T(reason));
+      UnicodeString S = GetTlsErrorStr(aerr);
+      LogSocketMessageRaw(FZ_LOG_WARNING, S.c_str());
     }
   }
 }

source/resource/TextsCore.h

@@ -292,6 +292,7 @@
 #define S3_ASSUME_ROLE_RESPONSE_ERROR 781
 #define INI_NO_SITES            782
 #define TLS_UNSUPPORTED         783
+#define OPENSSL_INIT_ERROR      784
 
 #define CORE_CONFIRMATION_STRINGS 300
 #define CONFIRM_PROLONG_TIMEOUT3 301

source/resource/TextsCore1.rc

@@ -268,6 +268,7 @@ BEGIN
   S3_ASSUME_ROLE_RESPONSE_ERROR, "Unexpected response to assume role request (%s)."
   INI_NO_SITES, "No sites found in \"%s\"."
   TLS_UNSUPPORTED, "The server is using unsupported protocol. Your WinSCP session is configured to use %s through %s. It can be configured to use %s through %s. Though, avoid using old insecure protocols, whenever possible."
+  OPENSSL_INIT_ERROR, "OpenSSL initialization failed"
 
   CORE_CONFIRMATION_STRINGS, "CORE_CONFIRMATION"
   CONFIRM_PROLONG_TIMEOUT3, "Host is not communicating for %d seconds.\n\nWait for another %0:d seconds?"