Skip to content
Permalink
Browse files Browse the repository at this point in the history
Bug 1943: Prevent loading session settings that can lead to remote co…
…de execution from handled URLs

https://winscp.net/tracker/1943
(cherry picked from commit ec584f5)

Source commit: 0f4be408b3f01132b00682da72d925d6c4ee649b
  • Loading branch information
martinprikryl committed Jan 21, 2021
1 parent 21774a0 commit faa96e8
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 33 deletions.
2 changes: 1 addition & 1 deletion source/Console.cbproj
Expand Up @@ -43,7 +43,7 @@
<ProjectType>CppConsoleApplication</ProjectType>
<SanitizedProjectName>Console</SanitizedProjectName>
<VerInfo_IncludeVerInfo>true</VerInfo_IncludeVerInfo>
<VerInfo_Keys>CompanyName=Martin Prikryl;FileDescription=Console interface for WinSCP;FileVersion=5.0.1.0;InternalName=console;LegalCopyright=(c) 2000-2020 Martin Prikryl;LegalTrademarks=;OriginalFilename=winscp.com;ProductName=WinSCP;ProductVersion=5.17.10.0;ReleaseType=stable;WWW=https://winscp.net/</VerInfo_Keys>
<VerInfo_Keys>CompanyName=Martin Prikryl;FileDescription=Console interface for WinSCP;FileVersion=5.0.1.0;InternalName=console;LegalCopyright=(c) 2000-2021 Martin Prikryl;LegalTrademarks=;OriginalFilename=winscp.com;ProductName=WinSCP;ProductVersion=5.17.10.0;ReleaseType=stable;WWW=https://winscp.net/</VerInfo_Keys>
<VerInfo_Locale>1033</VerInfo_Locale>
<VerInfo_MajorVer>5</VerInfo_MajorVer>
<VerInfo_MinorVer>0</VerInfo_MinorVer>
Expand Down
2 changes: 1 addition & 1 deletion source/DragExt.cbproj
Expand Up @@ -49,7 +49,7 @@
<SanitizedProjectName>DragExt</SanitizedProjectName>
<VerInfo_DLL>true</VerInfo_DLL>
<VerInfo_IncludeVerInfo>true</VerInfo_IncludeVerInfo>
<VerInfo_Keys>CompanyName=Martin Prikryl;FileDescription=Drag&amp;Drop shell extension for WinSCP ($(Platform));FileVersion=2.0.0.0;InternalName=dragext;LegalCopyright=(c) 2000-2020 Martin Prikryl;LegalTrademarks=;OriginalFilename=dragext.dll;ProductName=WinSCP;ProductVersion=5.17.10.0;ReleaseType=stable;WWW=https://winscp.net/</VerInfo_Keys>
<VerInfo_Keys>CompanyName=Martin Prikryl;FileDescription=Drag&amp;Drop shell extension for WinSCP ($(Platform));FileVersion=2.0.0.0;InternalName=dragext;LegalCopyright=(c) 2000-2021 Martin Prikryl;LegalTrademarks=;OriginalFilename=dragext.dll;ProductName=WinSCP;ProductVersion=5.17.10.0;ReleaseType=stable;WWW=https://winscp.net/</VerInfo_Keys>
<VerInfo_Locale>1033</VerInfo_Locale>
<VerInfo_MajorVer>2</VerInfo_MajorVer>
</PropertyGroup>
Expand Down
2 changes: 1 addition & 1 deletion source/WinSCP.cbproj
Expand Up @@ -76,7 +76,7 @@
<SanitizedProjectName>WinSCP</SanitizedProjectName>
<UsingDelphiRTL>true</UsingDelphiRTL>
<VerInfo_IncludeVerInfo>true</VerInfo_IncludeVerInfo>
<VerInfo_Keys>CompanyName=Martin Prikryl;FileDescription=WinSCP: SFTP, FTP, WebDAV, S3 and SCP client;FileVersion=5.17.10.0;InternalName=winscp;LegalCopyright=(c) 2000-2020 Martin Prikryl;LegalTrademarks=;OriginalFilename=winscp.exe;ProductName=WinSCP;ProductVersion=5.17.10.0;ReleaseType=stable;WWW=https://winscp.net/</VerInfo_Keys>
<VerInfo_Keys>CompanyName=Martin Prikryl;FileDescription=WinSCP: SFTP, FTP, WebDAV, S3 and SCP client;FileVersion=5.17.10.0;InternalName=winscp;LegalCopyright=(c) 2000-2021 Martin Prikryl;LegalTrademarks=;OriginalFilename=winscp.exe;ProductName=WinSCP;ProductVersion=5.17.10.0;ReleaseType=stable;WWW=https://winscp.net/</VerInfo_Keys>
<VerInfo_Locale>1033</VerInfo_Locale>
<VerInfo_MajorVer>5</VerInfo_MajorVer>
<VerInfo_MinorVer>17</VerInfo_MinorVer>
Expand Down
67 changes: 43 additions & 24 deletions source/core/SessionData.cpp
Expand Up @@ -596,7 +596,7 @@ bool __fastcall TSessionData::IsInFolderOrWorkspace(UnicodeString AFolder)
return StartsText(UnixIncludeTrailingBackslash(AFolder), Name);
}
//---------------------------------------------------------------------
void __fastcall TSessionData::DoLoad(THierarchicalStorage * Storage, bool PuttyImport, bool & RewritePassword)
void __fastcall TSessionData::DoLoad(THierarchicalStorage * Storage, bool PuttyImport, bool & RewritePassword, bool Unsafe)
{
// Make sure we only ever use methods supported by TOptionsStorage
// (implemented by TOptionsIniFile)
Expand Down Expand Up @@ -668,7 +668,10 @@ void __fastcall TSessionData::DoLoad(THierarchicalStorage * Storage, bool PuttyI
CipherList = Storage->ReadString(L"Cipher", CipherList);
KexList = Storage->ReadString(L"KEX", KexList);
HostKeyList = Storage->ReadString(L"HostKey", HostKeyList);
GssLibList = Storage->ReadString(L"GSSLibs", GssLibList);
if (!Unsafe)
{
GssLibList = Storage->ReadString(L"GSSLibs", GssLibList);
}
GssLibCustom = Storage->ReadString(L"GSSCustom", GssLibCustom);
PublicKeyFile = Storage->ReadString(L"PublicKeyFile", PublicKeyFile);
AddressFamily = static_cast<TAddressFamily>
Expand All @@ -690,22 +693,31 @@ void __fastcall TSessionData::DoLoad(THierarchicalStorage * Storage, bool PuttyI
DSTMode = (TDSTMode)Storage->ReadInteger(L"ConsiderDST", DSTMode);
LockInHome = Storage->ReadBool(L"LockInHome", LockInHome);
Special = Storage->ReadBool(L"Special", Special);
Shell = Storage->ReadString(L"Shell", Shell);
if (!Unsafe)
{
Shell = Storage->ReadString(L"Shell", Shell);
}
ClearAliases = Storage->ReadBool(L"ClearAliases", ClearAliases);
UnsetNationalVars = Storage->ReadBool(L"UnsetNationalVars", UnsetNationalVars);
ListingCommand = Storage->ReadString(L"ListingCommand",
Storage->ReadBool(L"AliasGroupList", false) ? UnicodeString(L"ls -gla") : ListingCommand);
if (!Unsafe)
{
ListingCommand = Storage->ReadString(L"ListingCommand",
Storage->ReadBool(L"AliasGroupList", false) ? UnicodeString(L"ls -gla") : ListingCommand);
}
IgnoreLsWarnings = Storage->ReadBool(L"IgnoreLsWarnings", IgnoreLsWarnings);
SCPLsFullTime = TAutoSwitch(Storage->ReadInteger(L"SCPLsFullTime", SCPLsFullTime));
Scp1Compatibility = Storage->ReadBool(L"Scp1Compatibility", Scp1Compatibility);
TimeDifference = Storage->ReadFloat(L"TimeDifference", TimeDifference);
TimeDifferenceAuto = Storage->ReadBool(L"TimeDifferenceAuto", (TimeDifference == TDateTime()));
DeleteToRecycleBin = Storage->ReadBool(L"DeleteToRecycleBin", DeleteToRecycleBin);
OverwrittenToRecycleBin = Storage->ReadBool(L"OverwrittenToRecycleBin", OverwrittenToRecycleBin);
RecycleBinPath = Storage->ReadString(L"RecycleBinPath", RecycleBinPath);
PostLoginCommands = Storage->ReadString(L"PostLoginCommands", PostLoginCommands);
if (!Unsafe)
{
DeleteToRecycleBin = Storage->ReadBool(L"DeleteToRecycleBin", DeleteToRecycleBin);
OverwrittenToRecycleBin = Storage->ReadBool(L"OverwrittenToRecycleBin", OverwrittenToRecycleBin);
RecycleBinPath = Storage->ReadString(L"RecycleBinPath", RecycleBinPath);
PostLoginCommands = Storage->ReadString(L"PostLoginCommands", PostLoginCommands);
ReturnVar = Storage->ReadString(L"ReturnVar", ReturnVar);
}
ReturnVar = Storage->ReadString(L"ReturnVar", ReturnVar);
ExitCode1IsError = Storage->ReadBool(L"ExitCode1IsError", ExitCode1IsError);
LookupUserGroups = TAutoSwitch(Storage->ReadInteger(L"LookupUserGroups2", LookupUserGroups));
EOLType = (TEOLType)Storage->ReadInteger(L"EOLType", EOLType);
Expand Down Expand Up @@ -740,13 +752,16 @@ void __fastcall TSessionData::DoLoad(THierarchicalStorage * Storage, bool PuttyI
RawByteString AProxyPassword = Storage->ReadStringAsBinaryData(L"ProxyPasswordEnc", FProxyPassword);
SET_SESSION_PROPERTY_FROM(ProxyPassword, AProxyPassword);
}
if (ProxyMethod == pmCmd)
if (!Unsafe)
{
ProxyLocalCommand = Storage->ReadStringRaw(L"ProxyTelnetCommand", ProxyLocalCommand);
}
else
{
ProxyTelnetCommand = Storage->ReadStringRaw(L"ProxyTelnetCommand", ProxyTelnetCommand);
if (ProxyMethod == pmCmd)
{
ProxyLocalCommand = Storage->ReadStringRaw(L"ProxyTelnetCommand", ProxyLocalCommand);
}
else
{
ProxyTelnetCommand = Storage->ReadStringRaw(L"ProxyTelnetCommand", ProxyTelnetCommand);
}
}
ProxyDNS = TAutoSwitch((Storage->ReadInteger(L"ProxyDNS", (ProxyDNS + 2) % 3) + 1) % 3);
ProxyLocalhost = Storage->ReadBool(L"ProxyLocalhost", ProxyLocalhost);
Expand Down Expand Up @@ -775,7 +790,10 @@ void __fastcall TSessionData::DoLoad(THierarchicalStorage * Storage, bool PuttyI
Bug[sbHMAC2] = asOn;
}
SftpServer = Storage->ReadString(L"SftpServer", SftpServer);
if (!Unsafe)
{
SftpServer = Storage->ReadString(L"SftpServer", SftpServer);
}
#define READ_SFTP_BUG(BUG) \
SFTPBug[sb##BUG] = TAutoSwitch(Storage->ReadInteger(L"SFTP" #BUG "Bug", SFTPBug[sb##BUG]));
READ_SFTP_BUG(Symlink);
Expand Down Expand Up @@ -920,7 +938,7 @@ void __fastcall TSessionData::Load(THierarchicalStorage * Storage, bool PuttyImp
ClearSessionPasswords();
FProxyPassword = L"";
DoLoad(Storage, PuttyImport, RewritePassword);
DoLoad(Storage, PuttyImport, RewritePassword, false);
Storage->CloseSubKey();
}
Expand Down Expand Up @@ -1941,6 +1959,7 @@ bool __fastcall TSessionData::ParseUrl(UnicodeString Url, TOptions * Options,
*AProtocolDefined = ProtocolDefined;
}

bool Unsafe = FLAGSET(Flags, pufUnsafe);
if (!Url.IsEmpty())
{
UnicodeString DecodedUrl = DecodeUrlChars(Url);
Expand Down Expand Up @@ -2105,7 +2124,7 @@ bool __fastcall TSessionData::ParseUrl(UnicodeString Url, TOptions * Options,

if (RawSettings->Count > 0) // optimization
{
ApplyRawSettings(RawSettings.get());
ApplyRawSettings(RawSettings.get(), FLAGSET(Flags, pufUnsafe));
}

bool HasPassword = (UserInfo.Pos(L':') > 0);
Expand Down Expand Up @@ -2254,24 +2273,24 @@ bool __fastcall TSessionData::ParseUrl(UnicodeString Url, TOptions * Options,
std::unique_ptr<TStrings> RawSettings(new TStringList());
if (Options->FindSwitch(RawSettingsOption, RawSettings.get()))
{
ApplyRawSettings(RawSettings.get());
ApplyRawSettings(RawSettings.get(), Unsafe);
}
}
}

return true;
}
//---------------------------------------------------------------------
void __fastcall TSessionData::ApplyRawSettings(TStrings * RawSettings)
void __fastcall TSessionData::ApplyRawSettings(TStrings * RawSettings, bool Unsafe)
{
std::unique_ptr<TOptionsStorage> OptionsStorage(new TOptionsStorage(RawSettings, false));
ApplyRawSettings(OptionsStorage.get());
ApplyRawSettings(OptionsStorage.get(), Unsafe);
}
//---------------------------------------------------------------------
void __fastcall TSessionData::ApplyRawSettings(THierarchicalStorage * Storage)
void __fastcall TSessionData::ApplyRawSettings(THierarchicalStorage * Storage, bool Unsafe)
{
bool Dummy;
DoLoad(Storage, false, Dummy);
DoLoad(Storage, false, Dummy, Unsafe);
}
//---------------------------------------------------------------------
void __fastcall TSessionData::ConfigureTunnel(int APortNumber)
Expand Down
7 changes: 4 additions & 3 deletions source/core/SessionData.h
Expand Up @@ -55,6 +55,7 @@ enum TSessionUrlFlags
enum TParseUrlFlags
{
pufAllowStoredSiteWithProtocol = 0x01,
pufUnsafe = 0x02,
};
//---------------------------------------------------------------------------
extern const UnicodeString CipherNames[CIPHER_COUNT];
Expand Down Expand Up @@ -415,7 +416,7 @@ friend class TStoredSessionList;
UnicodeString __fastcall GetFolderName();
void __fastcall Modify();
UnicodeString __fastcall GetSource();
void __fastcall DoLoad(THierarchicalStorage * Storage, bool PuttyImport, bool & RewritePassword);
void __fastcall DoLoad(THierarchicalStorage * Storage, bool PuttyImport, bool & RewritePassword, bool Unsafe);
void __fastcall DoSave(THierarchicalStorage * Storage,
bool PuttyExport, const TSessionData * Default, bool DoNotEncryptPasswords);
UnicodeString __fastcall ReadXmlNode(_di_IXMLNode Node, const UnicodeString & Name, const UnicodeString & Default);
Expand Down Expand Up @@ -468,8 +469,8 @@ friend class TStoredSessionList;
void __fastcall DefaultSettings();
void __fastcall NonPersistant();
void __fastcall Load(THierarchicalStorage * Storage, bool PuttyImport);
void __fastcall ApplyRawSettings(TStrings * RawSettings);
void __fastcall ApplyRawSettings(THierarchicalStorage * Storage);
void __fastcall ApplyRawSettings(TStrings * RawSettings, bool Unsafe);
void __fastcall ApplyRawSettings(THierarchicalStorage * Storage, bool Unsafe);
void __fastcall ImportFromFilezilla(_di_IXMLNode Node, const UnicodeString & Path, _di_IXMLNode SettingsNode);
void __fastcall Save(THierarchicalStorage * Storage, bool PuttyExport,
const TSessionData * Default = NULL);
Expand Down
2 changes: 1 addition & 1 deletion source/forms/Custom.cpp
Expand Up @@ -1507,7 +1507,7 @@ bool __fastcall TSiteRawDialog::Execute(TSessionData * Data)
Data->Password = BackupData->Password;
Data->Ftps = BackupData->Ftps;

Data->ApplyRawSettings(SettingsMemo->Lines);
Data->ApplyRawSettings(SettingsMemo->Lines, false);
}
return Result;
}
Expand Down
2 changes: 1 addition & 1 deletion source/windows/ConsoleRunner.cpp
Expand Up @@ -2368,7 +2368,7 @@ int __fastcall BatchSettings(TConsole * Console, TProgramParams * Params)
Matches++;
std::unique_ptr<TSessionData> OriginalData(new TSessionData(L""));
OriginalData->CopyDataNoRecrypt(Data);
Data->ApplyRawSettings(OptionsStorage.get());
Data->ApplyRawSettings(OptionsStorage.get(), false);
bool Changed = !OriginalData->IsSame(Data, false);
if (Changed)
{
Expand Down
5 changes: 4 additions & 1 deletion source/windows/WinMain.cpp
Expand Up @@ -1096,7 +1096,10 @@ int __fastcall Execute()
std::unique_ptr<TObjectList> DataList(new TObjectList());
try
{
GetLoginData(AutoStartSession, Params, DataList.get(), DownloadFile, NeedSession, NULL, pufAllowStoredSiteWithProtocol);
int Flags =
pufAllowStoredSiteWithProtocol |
FLAGMASK(!CheckSafe(Params), pufUnsafe);
GetLoginData(AutoStartSession, Params, DataList.get(), DownloadFile, NeedSession, NULL, Flags);
// GetLoginData now Aborts when session is needed and none is selected
if (DebugAlwaysTrue(!NeedSession || (DataList->Count > 0)))
{
Expand Down

0 comments on commit faa96e8

Please sign in to comment.