From ab27c7b0d8ce20f84d2e060b9226f7d88580c396 Mon Sep 17 00:00:00 2001 From: Jonas Reinwald Date: Thu, 2 Aug 2018 19:06:02 +0200 Subject: [PATCH 1/3] static analysis fixes --- SharkCage/CageConfigurator/App.config | 2 +- .../CageConfigurator/CageConfiguratorForm.cs | 100 ++++++++++-------- .../Properties/Settings.Designer.cs | 6 +- .../Properties/Settings.settings | 2 +- SharkCage/CageManager/FullWorkArea.h | 2 +- SharkCage/CageManager/SecuritySetup.h | 8 ++ SharkCage/CageService/CageService.cpp | 18 +++- .../CageServiceInstaller/ServiceInstaller.cs | 1 - 8 files changed, 81 insertions(+), 58 deletions(-) diff --git a/SharkCage/CageConfigurator/App.config b/SharkCage/CageConfigurator/App.config index aeaf16ed..a6ef7220 100644 --- a/SharkCage/CageConfigurator/App.config +++ b/SharkCage/CageConfigurator/App.config @@ -10,7 +10,7 @@ - + diff --git a/SharkCage/CageConfigurator/CageConfiguratorForm.cs b/SharkCage/CageConfigurator/CageConfiguratorForm.cs index 8a2e3031..7ef30206 100644 --- a/SharkCage/CageConfigurator/CageConfiguratorForm.cs +++ b/SharkCage/CageConfigurator/CageConfiguratorForm.cs @@ -124,24 +124,26 @@ private void applicationBrowseButton_Click(object sender, System.EventArgs e) { const string file_type = "*.exe"; - var file_dialog = new OpenFileDialog(); - file_dialog.CheckFileExists = true; - file_dialog.Filter = $"Application|{file_type}"; - var result = file_dialog.ShowDialog(); - if (result == DialogResult.OK) - { - var chosen_file = file_dialog.FileName; - CheckPath(chosen_file, file_type, (string path) => + using (var file_dialog = new OpenFileDialog()) + { + file_dialog.CheckFileExists = true; + file_dialog.Filter = $"Application|{file_type}"; + var result = file_dialog.ShowDialog(); + if (result == DialogResult.OK) { - if (applicationPath.Text != path) + var chosen_file = file_dialog.FileName; + CheckPath(chosen_file, file_type, (string path) => { - applicationPath.Text = path; - SetUnsavedData(true); - } + if (applicationPath.Text != path) + { + applicationPath.Text = path; + SetUnsavedData(true); + } - applicationPath.SelectionStart = applicationPath.Text.Length; - applicationPath.ScrollToCaret(); - }); + applicationPath.SelectionStart = applicationPath.Text.Length; + applicationPath.ScrollToCaret(); + }); + } } } @@ -191,14 +193,16 @@ private void openToolStripMenuItem_Click(object sender, EventArgs e) const string file_type = ".sconfig"; - var file_dialog = new OpenFileDialog(); - file_dialog.CheckFileExists = true; - file_dialog.Filter = $"SharkCage configuration|*{file_type}"; - var result = file_dialog.ShowDialog(); - if (result == DialogResult.OK) + using (var file_dialog = new OpenFileDialog()) { - var chosen_file = file_dialog.FileName; - CheckPath(chosen_file, file_type, LoadConfig); + file_dialog.CheckFileExists = true; + file_dialog.Filter = $"SharkCage configuration|*{file_type}"; + var result = file_dialog.ShowDialog(); + if (result == DialogResult.OK) + { + var chosen_file = file_dialog.FileName; + CheckPath(chosen_file, file_type, LoadConfig); + } } } @@ -250,7 +254,7 @@ private void LoadAdditionalApp(string additional_app, string additional_app_path switch (additional_app) { case "Keepass": - Settings.Default.PeristentKeepassPath = additional_app_path; + Settings.Default.PersistentKeepassPath = additional_app_path; secureSecondaryPrograms.SelectedIndex = 1; break; default: @@ -288,16 +292,18 @@ private void secureSecondaryPrograms_SelectedIndexChanged(object sender, EventAr switch (secureSecondaryPrograms.SelectedItem.ToString()) { case "Keepass": - if (Settings.Default.PeristentKeepassPath.Length == 0) + if (Settings.Default.PersistentKeepassPath == String.Empty) { MessageBox.Show("There is no path saved for this program, please select it now.", "Shark Cage", MessageBoxButtons.OK, MessageBoxIcon.Information); - var keepass_file_dialog = new OpenFileDialog(); - keepass_file_dialog.CheckFileExists = true; - keepass_file_dialog.Filter = "Keepass|Keepass.exe"; - var result = keepass_file_dialog.ShowDialog(); - if (result == DialogResult.OK) + using (var keepass_file_dialog = new OpenFileDialog()) { - Settings.Default.PeristentKeepassPath = keepass_file_dialog.FileName; + keepass_file_dialog.CheckFileExists = true; + keepass_file_dialog.Filter = "Keepass|Keepass.exe"; + var result = keepass_file_dialog.ShowDialog(); + if (result == DialogResult.OK) + { + Settings.Default.PersistentKeepassPath = keepass_file_dialog.FileName; + } } } break; @@ -317,22 +323,24 @@ private void tokenBrowseButton_Click(object sender, EventArgs e) { string[] file_types = { "*.bmp", "*.png", "*.jpeg", "*.jpg" }; - var file_dialog = new OpenFileDialog(); - file_dialog.CheckFileExists = true; - file_dialog.Filter = $"Picture|{String.Join(";", file_types)}"; - var result = file_dialog.ShowDialog(); - if (result == DialogResult.OK) + using (var file_dialog = new OpenFileDialog()) { - var chosen_file = file_dialog.FileName; - CheckPath(chosen_file, file_types, (string path) => + file_dialog.CheckFileExists = true; + file_dialog.Filter = $"Picture|{String.Join(";", file_types)}"; + var result = file_dialog.ShowDialog(); + if (result == DialogResult.OK) { - if (tokenBox.ImageLocation != path) + var chosen_file = file_dialog.FileName; + CheckPath(chosen_file, file_types, (string path) => { - tokenBox.ImageLocation = path; - tokenBox.Load(); - SetUnsavedData(true); - } - }); + if (tokenBox.ImageLocation != path) + { + tokenBox.ImageLocation = path; + tokenBox.Load(); + SetUnsavedData(true); + } + }); + } } } @@ -391,7 +399,7 @@ private void openCageChooserButton_Click(object sender, EventArgs e) { var install_dir = (Registry.GetValue(REGISTRY_KEY, "InstallDir", "") as string) ?? String.Empty; - if (install_dir.Length == 0) + if (install_dir == String.Empty) { MessageBox.Show("Could not read installation directory from registry, opening CageChooser not possible", "Shark Cage", MessageBoxButtons.OK, MessageBoxIcon.Information); return; @@ -422,7 +430,7 @@ private void saveButton_Click(object sender, EventArgs e) } var secondary_path = GetSecondaryApplicationPath(secureSecondaryPrograms.Text); - if (secondary_path.Length == 0 && secureSecondaryPrograms.SelectedIndex != 0) + if (String.IsNullOrEmpty(secondary_path) && secureSecondaryPrograms.SelectedIndex != 0) { MessageBox.Show("You specified a secondary program but did not provide a corresponding location. Please reselect the application, provide the location and try again.", "SharkCage", MessageBoxButtons.OK, MessageBoxIcon.Information); @@ -552,7 +560,7 @@ private string GetSecondaryApplicationPath(string name) switch (name) { case "Keepass": - return Settings.Default.PeristentKeepassPath; + return Settings.Default.PersistentKeepassPath; default: return String.Empty; } diff --git a/SharkCage/CageConfigurator/Properties/Settings.Designer.cs b/SharkCage/CageConfigurator/Properties/Settings.Designer.cs index 355d784e..95c2c56d 100644 --- a/SharkCage/CageConfigurator/Properties/Settings.Designer.cs +++ b/SharkCage/CageConfigurator/Properties/Settings.Designer.cs @@ -26,12 +26,12 @@ public static Settings Default { [global::System.Configuration.UserScopedSettingAttribute()] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] - public string PeristentKeepassPath { + public string PersistentKeepassPath { get { - return ((string)(this["PeristentKeepassPath"])); + return ((string)(this["PersistentKeepassPath"])); } set { - this["PeristentKeepassPath"] = value; + this["PersistentKeepassPath"] = value; } } } diff --git a/SharkCage/CageConfigurator/Properties/Settings.settings b/SharkCage/CageConfigurator/Properties/Settings.settings index 6ed15ac0..53b1d47b 100644 --- a/SharkCage/CageConfigurator/Properties/Settings.settings +++ b/SharkCage/CageConfigurator/Properties/Settings.settings @@ -2,7 +2,7 @@ - + diff --git a/SharkCage/CageManager/FullWorkArea.h b/SharkCage/CageManager/FullWorkArea.h index c5ca7302..1176a8fd 100644 --- a/SharkCage/CageManager/FullWorkArea.h +++ b/SharkCage/CageManager/FullWorkArea.h @@ -9,7 +9,7 @@ class FullWorkArea ~FullWorkArea(); bool Init(); private: - RECT rect; + RECT rect = { 0 }; bool GetBottomFromMonitor(int &monitor_bottom); int cage_width; }; \ No newline at end of file diff --git a/SharkCage/CageManager/SecuritySetup.h b/SharkCage/CageManager/SecuritySetup.h index 1e43570d..dea7ce48 100644 --- a/SharkCage/CageManager/SecuritySetup.h +++ b/SharkCage/CageManager/SecuritySetup.h @@ -9,11 +9,19 @@ auto local_free_deleter = [&](T resource) { ::LocalFree(resource); }; class SecuritySetup { public: + SecuritySetup() {}; + + // these need to be customized if needed so security_descriptor also gets copied + // (otherwise there could be use-after-frees in the destructor) + SecuritySetup(const SecuritySetup &) = delete; + SecuritySetup& operator= (const SecuritySetup &) = delete; + ~SecuritySetup() { if (security_descriptor) { ::LocalFree(security_descriptor); + security_descriptor = nullptr; } } diff --git a/SharkCage/CageService/CageService.cpp b/SharkCage/CageService/CageService.cpp index 83904a3c..bb2a6b48 100644 --- a/SharkCage/CageService/CageService.cpp +++ b/SharkCage/CageService/CageService.cpp @@ -173,6 +173,8 @@ void CageService::StopCageManager() std::wostringstream os; os << "Terminated CageManager: 55" << std::endl; ::OutputDebugString(os.str().c_str()); + + ::CloseHandle(cage_manager_handle); } else { @@ -187,7 +189,7 @@ std::wstring CageService::GetLastErrorAsString(DWORD error_id) { // Get the error message, if any. LPWSTR message_buffer = nullptr; - size_t size = FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, + size_t size = ::FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, error_id, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), @@ -195,12 +197,17 @@ std::wstring CageService::GetLastErrorAsString(DWORD error_id) 0, NULL); - std::wstring message(message_buffer, size); + if (message_buffer && size > 0) + { + std::wstring message(message_buffer, size); + + // Free the buffer. + ::LocalFree(message_buffer); - // Free the buffer. - ::LocalFree(message_buffer); + return message; + } - return message; + return L""; } void CageService::HandleMessage(const std::wstring &message, NetworkManager &network_manager) @@ -271,6 +278,7 @@ void CageService::HandleMessage(const std::wstring &message, NetworkManager &net cage_manager_process_id = 0; + ::CloseHandle(cage_manager_handle); ::CloseHandle(created_token); } diff --git a/SharkCage/CageServiceInstaller/ServiceInstaller.cs b/SharkCage/CageServiceInstaller/ServiceInstaller.cs index 6d036b07..0438bff5 100644 --- a/SharkCage/CageServiceInstaller/ServiceInstaller.cs +++ b/SharkCage/CageServiceInstaller/ServiceInstaller.cs @@ -7,7 +7,6 @@ public static class ServiceInstaller { - private const int STANDARD_RIGHTS_REQUIRED = 0xF0000; private const int SERVICE_WIN32_OWN_PROCESS = 0x00000010; [StructLayout(LayoutKind.Sequential)] From 7511d3f3e1c405c01e12517c21ef73d72298f57f Mon Sep 17 00:00:00 2001 From: Jonas Reinwald Date: Thu, 2 Aug 2018 19:39:03 +0200 Subject: [PATCH 2/3] static analysis fixex (tokenLib) --- SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp b/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp index 9816ca4f..1ce04d5c 100644 --- a/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp +++ b/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp @@ -65,9 +65,13 @@ class tokenTemplate { public: tokenTemplate(HANDLE &userToken); - ~tokenTemplate(); + // these need to be customized if needed so memory of member pointers also gets copied + // (otherwise there could be use-after-frees in the destructor) + tokenTemplate(const tokenTemplate &) = delete; + tokenTemplate operator=(const tokenTemplate &) = delete; + bool addGroup(PSID sid); bool generateToken(HANDLE & token); @@ -438,8 +442,9 @@ bool changeTokenCreationPrivilege(bool privilegeStatus) { wprintf(L"Error getting token for privilege escalation\n"); return false; } - return setPrivilege(userTokenHandle, SE_CREATE_TOKEN_NAME, privilegeStatus); + auto set_privilege_status = setPrivilege(userTokenHandle, SE_CREATE_TOKEN_NAME, privilegeStatus); CloseHandle(userTokenHandle); + return set_privilege_status; } tokenTemplate::tokenTemplate(HANDLE &userToken) { @@ -554,7 +559,7 @@ tokenTemplate::tokenTemplate(HANDLE &userToken) { } tokenTemplate::~tokenTemplate() { - delete objectAttributes->SecurityQualityOfService; + delete static_cast(objectAttributes->SecurityQualityOfService); delete objectAttributes; delete authenticationId; delete expirationTime; From b3ce0a884c2e65fbfdee9a2bf4703e51079a617a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Ben=C4=8D=C3=ADk?= Date: Mon, 6 Aug 2018 17:21:13 +0200 Subject: [PATCH 3/3] static analysis fixes 2 (tokenLib) --- .../SharedFunctionality/tokenLib/tokenLib.cpp | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp b/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp index d09b0c15..8b55bc22 100644 --- a/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp +++ b/SharkCage/SharedFunctionality/tokenLib/tokenLib.cpp @@ -232,9 +232,17 @@ std::optional> getProcessesWithBothPrivileges(const std::vect { HANDLE processHandle; if ((processHandle = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, processPid)) == NULL) continue; - if (hasSeCreateTokenPrivilege(processHandle) && hasSeTcbPrivilege(processHandle)) + try + { + if (hasSeCreateTokenPrivilege(processHandle) && hasSeTcbPrivilege(processHandle)) + { + processes.push_back(processPid); + } + } + catch (const std::exception&) { - processes.push_back(processPid); + CloseHandle(processHandle); + throw; } CloseHandle(processHandle); } @@ -336,9 +344,17 @@ std::optional getProcessUnderLocalSystem(std::vector processes){ { HANDLE processHandle; if ((processHandle = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, processPid)) == NULL) continue; - if (processIsLocalSystem(processHandle)) + try + { + if (processIsLocalSystem(processHandle)) + { + return processHandle; + } + } + catch (const std::exception&) { - return processHandle; + CloseHandle(processHandle); + throw; } CloseHandle(processHandle); }