From d5f34ad49d345803767d4a1166d764f9f8485541 Mon Sep 17 00:00:00 2001 From: Mounir IDRASSI Date: Sun, 8 Feb 2015 23:46:04 +0100 Subject: Static Code Analysis: Avoid over-flaw in arithmetic operations by adding more checks. Add extra checks. Solve various issues. --- src/Format/InPlace.c | 12 ++++++ src/Format/Tcformat.c | 14 +++--- src/Mount/MainCom.cpp | 35 +++++++++++++-- src/Mount/Mount.c | 117 ++++++++++++++++++++++++++++++++++++++++---------- src/Setup/Dir.c | 21 ++++----- src/Setup/Setup.c | 31 ++++++------- 6 files changed, 172 insertions(+), 58 deletions(-) diff --git a/src/Format/InPlace.c b/src/Format/InPlace.c index 4c5491e3..3998c2a5 100644 --- a/src/Format/InPlace.c +++ b/src/Format/InPlace.c @@ -21,6 +21,7 @@ IMPORTANT: Due to this issue, functions in this file must not directly interact #include #include #include +#include #include "Tcdefs.h" #include "Platform/Finally.h" @@ -71,6 +72,17 @@ static __int64 NewFileSysSizeAfterShrink (HANDLE dev, const char *devicePath, in return -1; } + if ( (ntfsVolData.NumberSectors.QuadPart <= 0) + || (ntfsVolData.NumberSectors.QuadPart > (INT64_MAX / (__int64) ntfsVolData.BytesPerSector)) // overflow test + ) + { + SetLastError (ERROR_INTERNAL_ERROR); + if (!silent) + handleWin32Error (MainDlg); + + return -1; + } + fileSysSize = ntfsVolData.NumberSectors.QuadPart * ntfsVolData.BytesPerSector; desiredNbrSectors = (fileSysSize - TC_TOTAL_VOLUME_HEADERS_SIZE) / ntfsVolData.BytesPerSector; diff --git a/src/Format/Tcformat.c b/src/Format/Tcformat.c index 4984e6cc..995222de 100644 --- a/src/Format/Tcformat.c +++ b/src/Format/Tcformat.c @@ -2537,13 +2537,12 @@ static void __cdecl volTransformThreadFunction (void *hwndDlgArg) if (!bInPlaceEncNonSys) SetTimer (hwndDlg, TIMER_ID_RANDVIEW, TIMER_INTERVAL_RANDVIEW, NULL); - if (volParams != NULL) - { - burn ((LPVOID) volParams, sizeof(FORMAT_VOL_PARAMETERS)); - VirtualUnlock ((LPVOID) volParams, sizeof(FORMAT_VOL_PARAMETERS)); - free ((LPVOID) volParams); - volParams = NULL; - } + + // volParams is ensured to be non NULL at this stage + burn ((LPVOID) volParams, sizeof(FORMAT_VOL_PARAMETERS)); + VirtualUnlock ((LPVOID) volParams, sizeof(FORMAT_VOL_PARAMETERS)); + free ((LPVOID) volParams); + volParams = NULL; bVolTransformThreadRunning = FALSE; bVolTransformThreadCancel = FALSE; @@ -9027,6 +9026,7 @@ int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, char *lpszComm DialogBoxParamW (hInstance, MAKEINTRESOURCEW (IDD_VOL_CREATION_WIZARD_DLG), NULL, (DLGPROC) MainDialogProc, (LPARAM)lpszCommandLine); + FinalizeApp (); return 0; } diff --git a/src/Mount/MainCom.cpp b/src/Mount/MainCom.cpp index b2dfe89c..5a43d36f 100644 --- a/src/Mount/MainCom.cpp +++ b/src/Mount/MainCom.cpp @@ -255,7 +255,17 @@ extern "C" int UacBackupVolumeHeader (HWND hwndDlg, BOOL bRequireConfirmation, c CoInitialize (NULL); if (ComGetInstance (hwndDlg, &tc)) - r = tc->BackupVolumeHeader ((LONG_PTR) hwndDlg, bRequireConfirmation, CComBSTR (lpszVolume)); + { + CComBSTR volumeBstr; + BSTR bstr = A2WBSTR(lpszVolume); + if (bstr) + { + volumeBstr.Attach (bstr); + r = tc->BackupVolumeHeader ((LONG_PTR) hwndDlg, bRequireConfirmation, volumeBstr); + } + else + r = ERR_OUTOFMEMORY; + } else r = -1; @@ -273,7 +283,17 @@ extern "C" int UacRestoreVolumeHeader (HWND hwndDlg, char *lpszVolume) CoInitialize (NULL); if (ComGetInstance (hwndDlg, &tc)) - r = tc->RestoreVolumeHeader ((LONG_PTR) hwndDlg, CComBSTR (lpszVolume)); + { + CComBSTR volumeBstr; + BSTR bstr = A2WBSTR(lpszVolume); + if (bstr) + { + volumeBstr.Attach (bstr); + r = tc->RestoreVolumeHeader ((LONG_PTR) hwndDlg, volumeBstr); + } + else + r = ERR_OUTOFMEMORY; + } else r = -1; @@ -291,7 +311,16 @@ extern "C" int UacChangePwd (char *lpszVolume, Password *oldPassword, int old_pk if (ComGetInstance (hwndDlg, &tc)) { WaitCursor (); - r = tc->ChangePasswordEx2 (CComBSTR (lpszVolume), oldPassword, old_pkcs5, truecryptMode, newPassword, pkcs5, wipePassCount, (LONG_PTR) hwndDlg); + CComBSTR volumeBstr; + BSTR bstr = A2WBSTR(lpszVolume); + if (bstr) + { + volumeBstr.Attach (bstr); + + r = tc->ChangePasswordEx2 (volumeBstr, oldPassword, old_pkcs5, truecryptMode, newPassword, pkcs5, wipePassCount, (LONG_PTR) hwndDlg); + } + else + r = ERR_OUTOFMEMORY; NormalCursor (); } else diff --git a/src/Mount/Mount.c b/src/Mount/Mount.c index c6feb3b5..eb14d76e 100644 --- a/src/Mount/Mount.c +++ b/src/Mount/Mount.c @@ -719,8 +719,11 @@ BOOL WholeSysDriveEncryption (BOOL bSilent) { BootEncStatus = BootEncObj->GetStatus(); - return (BootEncStatus.ConfiguredEncryptedAreaStart == TC_BOOT_LOADER_AREA_SIZE - && BootEncStatus.ConfiguredEncryptedAreaEnd >= BootEncStatus.BootDriveLength.QuadPart - 1); + if (BootEncStatus.BootDriveLength.QuadPart < 1) // paranoid check + return FALSE; + else + return (BootEncStatus.ConfiguredEncryptedAreaStart == TC_BOOT_LOADER_AREA_SIZE + && BootEncStatus.ConfiguredEncryptedAreaEnd >= BootEncStatus.BootDriveLength.QuadPart - 1); } catch (Exception &e) { @@ -742,9 +745,16 @@ unsigned __int64 GetSysEncDeviceSize (BOOL bSilent) { if (!bSilent) e.Show (MainDlg); + return 1; } - return (BootEncStatus.ConfiguredEncryptedAreaEnd - BootEncStatus.ConfiguredEncryptedAreaStart + 1); + if ( BootEncStatus.ConfiguredEncryptedAreaEnd < 0 + || BootEncStatus.ConfiguredEncryptedAreaStart < 0 + || BootEncStatus.ConfiguredEncryptedAreaEnd < BootEncStatus.ConfiguredEncryptedAreaStart + ) + return 1; // we return 1 to avoid devision by zero + else + return ((unsigned __int64)(BootEncStatus.ConfiguredEncryptedAreaEnd - BootEncStatus.ConfiguredEncryptedAreaStart)) + 1; } // Returns the current size of the encrypted area of the system drive/partition in bytes @@ -758,9 +768,16 @@ unsigned __int64 GetSysEncDeviceEncryptedPartSize (BOOL bSilent) { if (!bSilent) e.Show (MainDlg); + return 0; } - return (BootEncStatus.EncryptedAreaEnd - BootEncStatus.EncryptedAreaStart + 1); + if ( BootEncStatus.EncryptedAreaEnd < 0 + || BootEncStatus.EncryptedAreaStart < 0 + || BootEncStatus.EncryptedAreaEnd < BootEncStatus.EncryptedAreaStart + ) + return 0; + else + return ((unsigned __int64)(BootEncStatus.EncryptedAreaEnd - BootEncStatus.EncryptedAreaStart)) + 1; } @@ -2885,14 +2902,19 @@ int GetCipherBlockSizeByDriveNo (int nDosDriveNo) if (DeviceIoControl (hDriver, TC_IOCTL_GET_VOLUME_PROPERTIES, &prop, sizeof (prop), &prop, sizeof (prop), &dwResult, NULL)) { - for (cipherID = EAGetLastCipher (prop.ea); - cipherID != 0; - cipherID = EAGetPreviousCipher (prop.ea, cipherID)) + if ( (prop.driveNo == nDosDriveNo) + && (prop.ea >= EAGetFirst() && prop.ea <= EAGetCount()) + ) { - if (blockSize > 0) - blockSize = min (blockSize, CipherGetBlockSize (cipherID) * 8); - else - blockSize = CipherGetBlockSize (cipherID) * 8; + for (cipherID = EAGetLastCipher (prop.ea); + cipherID != 0; + cipherID = EAGetPreviousCipher (prop.ea, cipherID)) + { + if (blockSize > 0) + blockSize = min (blockSize, CipherGetBlockSize (cipherID) * 8); + else + blockSize = CipherGetBlockSize (cipherID) * 8; + } } } @@ -2911,7 +2933,13 @@ int GetModeOfOperationByDriveNo (int nDosDriveNo) if (DeviceIoControl (hDriver, TC_IOCTL_GET_VOLUME_PROPERTIES, &prop, sizeof (prop), &prop, sizeof (prop), &dwResult, NULL)) { - return prop.mode; + if ( (prop.driveNo == nDosDriveNo) + && (prop.ea >= EAGetFirst() && prop.ea <= EAGetCount()) + && (prop.mode >= FIRST_MODE_OF_OPERATION_ID && prop.mode < MODE_ENUM_END_ID) + ) + { + return prop.mode; + } } return 0; @@ -3359,7 +3387,7 @@ BOOL CALLBACK TravelerDlgProc (HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lPa GetDlgItemText (hwndDlg, IDC_DIRECTORY, dstDir, sizeof dstDir); volName[0] = 0; - GetDlgItemText (hwndDlg, IDC_VOLUME_NAME, volName + 1, sizeof volName); + GetDlgItemText (hwndDlg, IDC_VOLUME_NAME, volName + 1, (sizeof volName) - 1); drive = SendDlgItemMessage (hwndDlg, IDC_DRIVELIST, CB_GETCURSEL, 0, 0); drive = SendDlgItemMessage (hwndDlg, IDC_DRIVELIST, CB_GETITEMDATA, drive, 0); @@ -3872,9 +3900,9 @@ void __cdecl mountThreadFunction (void *hwndDlgArg) static BOOL DismountAll (HWND hwndDlg, BOOL forceUnmount, BOOL interact, int dismountMaxRetries, int dismountAutoRetryDelay) { BOOL status = TRUE; - MOUNT_LIST_STRUCT mountList; + MOUNT_LIST_STRUCT mountList = {0}; DWORD dwResult; - UNMOUNT_STRUCT unmount; + UNMOUNT_STRUCT unmount = {0}; BOOL bResult; unsigned __int32 prevMountedDrives = 0; int i; @@ -3911,6 +3939,17 @@ retry: bResult = DeviceIoControl (hDriver, TC_IOCTL_DISMOUNT_ALL_VOLUMES, &unmount, sizeof (unmount), &unmount, sizeof (unmount), &dwResult, NULL); + if ( unmount.nDosDriveNo < 0 || unmount.nDosDriveNo > 25 + || (unmount.ignoreOpenFiles != TRUE && unmount.ignoreOpenFiles != FALSE) + || (unmount.HiddenVolumeProtectionTriggered != TRUE && unmount.HiddenVolumeProtectionTriggered != FALSE) + || (unmount.nReturnCode < 0) + ) + { + if (bResult) + SetLastError (ERROR_INTERNAL_ERROR); + bResult = FALSE; + } + if (bResult == FALSE) { NormalCursor(); @@ -5847,7 +5886,7 @@ BOOL CALLBACK MainDialogProc (HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lPa if (IsVolumeDeviceHosted (volp)) { - OPEN_TEST_STRUCT ots; + OPEN_TEST_STRUCT ots = {0}; if (!OpenDevice (volp, &ots, FALSE)) { @@ -7394,6 +7433,7 @@ int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, char *lpszComm DialogBoxParamW (hInstance, MAKEINTRESOURCEW (IDD_MOUNT_DLG), NULL, (DLGPROC) MainDialogProc, (LPARAM) lpszCommandLine); + FinalizeApp (); /* Terminate */ return 0; } @@ -7412,7 +7452,7 @@ BOOL TaskBarIconAdd (HWND hwnd) TaskBarIconMutex = CreateMutex (NULL, TRUE, "VeraCryptTaskBarIcon"); if (TaskBarIconMutex == NULL || GetLastError () == ERROR_ALREADY_EXISTS) { - if (TaskBarIconMutex) + if (TaskBarIconMutex != NULL) { CloseHandle(TaskBarIconMutex); TaskBarIconMutex = NULL; @@ -7516,7 +7556,15 @@ void DismountIdleVolumes () bResult = DeviceIoControl (hDriver, TC_IOCTL_GET_VOLUME_PROPERTIES, &prop, sizeof (prop), &prop, sizeof (prop), &dwResult, NULL); - if (bResult) + if ( bResult + && ( (prop.driveNo == i) && prop.uniqueId >= 0 + && prop.ea >= EAGetFirst() && prop.ea <= EAGetCount() + && prop.mode >= FIRST_MODE_OF_OPERATION_ID && prop.mode <= LAST_MODE_OF_OPERATION + && prop.pkcs5 >= FIRST_PRF_ID && prop.pkcs5 <= LAST_PRF_ID + && prop.pkcs5Iterations > 0 + && prop.hiddenVolProtection >= 0 && prop.volFormatVersion >= 0 + ) + ) { if (LastRead[i] == prop.totalBytesRead && LastWritten[i] == prop.totalBytesWritten @@ -9145,9 +9193,9 @@ void AnalyzeKernelMiniDump (HWND hwndDlg) return; if (Is64BitOs()) - sDbgCmd = "msiexec.exe /qb /i http://www.idrix.fr/Root/MSDebug/dbg_amd64_6.11.1.404.msi"; + sDbgCmd = "msiexec.exe /qb /i https://www.idrix.fr/Root/MSDebug/dbg_amd64_6.11.1.404.msi"; else - sDbgCmd = "msiexec.exe /qb /i http://www.idrix.fr/Root/MSDebug/dbg_x86_6.11.1.404.msi"; + sDbgCmd = "msiexec.exe /qb /i https://www.idrix.fr/Root/MSDebug/dbg_x86_6.11.1.404.msi"; if (!CreateProcess (NULL, (LPSTR) sDbgCmd.c_str(), NULL, NULL, FALSE, 0, NULL, NULL, &startupInfo, &procInfo)) @@ -9253,14 +9301,31 @@ void AnalyzeKernelMiniDump (HWND hwndDlg) CloseHandle (hChildStdoutWrite); string output; + BOOL bIsValidResponse = TRUE; while (TRUE) { - DWORD bytesReceived; - char pipeBuffer [4096]; + DWORD bytesReceived = 0, i; + char pipeBuffer [4096] = {0}; + + unsigned char uc; if (!ReadFile (hChildStdoutRead, pipeBuffer, sizeof (pipeBuffer), &bytesReceived, NULL)) - break; + break; + + /* check if the buffer contains printable characters only*/ + for (i = 0; i < bytesReceived; i++) + { + uc = (unsigned char) pipeBuffer [i]; + if ( uc >= 0x7f || uc < 0x20) // A non-ASCII or non-printable character? + { + bIsValidResponse = FALSE; + break; + } + } + + if (!bIsValidResponse) + break; output.insert (output.size(), pipeBuffer, bytesReceived); } @@ -9269,6 +9334,12 @@ void AnalyzeKernelMiniDump (HWND hwndDlg) NormalCursor(); + if (!bIsValidResponse) + { + Error ("ERR_PARAMETER_INCORRECT", hwndDlg); + return; + } + bool otherDriver = (StringToUpperCase (output).find (StringToUpperCase (TC_APP_NAME)) == string::npos); size_t p, p2; diff --git a/src/Setup/Dir.c b/src/Setup/Dir.c index a0380ef2..ec530df4 100644 --- a/src/Setup/Dir.c +++ b/src/Setup/Dir.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "Dir.h" @@ -28,7 +29,7 @@ mkfulldir (char *oriPath, BOOL bCheckonly) char *uniq_file; char path [TC_MAX_PATH]; - strcpy (path, oriPath); + StringCbCopyA (path, TC_MAX_PATH, oriPath); if (strlen (path) == 3 && path[1] == ':') goto is_root; /* keep final slash in root if present */ @@ -63,7 +64,7 @@ mkfulldir_internal (char *path) static char tokpath[_MAX_PATH]; static char trail[_MAX_PATH]; - strcpy (tokpath, path); + StringCbCopyA (tokpath, _MAX_PATH, path); trail[0] = '\0'; token = strtok (tokpath, "\\/"); @@ -75,13 +76,13 @@ mkfulldir_internal (char *path) trail[2] = '\0'; if (token) { - strcat (trail, token); - strcat (trail, "\\"); + StringCbCatA (trail, _MAX_PATH, token); + StringCbCatA (trail, _MAX_PATH, "\\"); token = strtok (NULL, "\\/"); if (token) { /* get share name */ - strcat (trail, token); - strcat (trail, "\\"); + StringCbCatA (trail, _MAX_PATH, token); + StringCbCatA (trail, _MAX_PATH, "\\"); } token = strtok (NULL, "\\/"); } @@ -89,17 +90,17 @@ mkfulldir_internal (char *path) if (tokpath[1] == ':') { /* drive letter */ - strcat (trail, tokpath); - strcat (trail, "\\"); + StringCbCatA (trail, _MAX_PATH, tokpath); + StringCbCatA (trail, _MAX_PATH, "\\"); token = strtok (NULL, "\\/"); } while (token != NULL) { int x; - strcat (trail, token); + StringCbCatA (trail, _MAX_PATH, token); x = _mkdir (trail); - strcat (trail, "\\"); + StringCbCatA (trail, _MAX_PATH, "\\"); token = strtok (NULL, "\\/"); } diff --git a/src/Setup/Setup.c b/src/Setup/Setup.c index 9f5fbfdd..dc8123c6 100644 --- a/src/Setup/Setup.c +++ b/src/Setup/Setup.c @@ -251,11 +251,12 @@ void IconMessage (HWND hwndDlg, char *txt) void DetermineUpgradeDowngradeStatus (BOOL bCloseDriverHandle, LONG *driverVersionPtr) { LONG driverVersion = VERSION_NUM; + int status = 0; if (hDriver == INVALID_HANDLE_VALUE) - DriverAttach(); + status = DriverAttach(); - if (hDriver != INVALID_HANDLE_VALUE) + if ((status == 0) && (hDriver != INVALID_HANDLE_VALUE)) { DWORD dwResult; BOOL bResult = DeviceIoControl (hDriver, TC_IOCTL_GET_DRIVER_VERSION, NULL, 0, &driverVersion, sizeof (driverVersion), &dwResult, NULL); @@ -745,7 +746,6 @@ BOOL DoApplicationDataUninstall (HWND hwndDlg) BOOL DoRegUninstall (HWND hwndDlg, BOOL bRemoveDeprecated) { - BOOL bOK = FALSE; char regk [64]; // Unregister COM servers @@ -775,17 +775,7 @@ BOOL DoRegUninstall (HWND hwndDlg, BOOL bRemoveDeprecated) SHChangeNotify (SHCNE_ASSOCCHANGED, SHCNF_IDLIST, NULL, NULL); } - bOK = TRUE; - - if (bOK == FALSE && GetLastError ()!= ERROR_NO_TOKEN && GetLastError ()!= ERROR_FILE_NOT_FOUND - && GetLastError ()!= ERROR_PATH_NOT_FOUND) - { - handleWin32Error (hwndDlg); - } - else - bOK = TRUE; - - return bOK; + return TRUE; } @@ -876,10 +866,16 @@ try_delete: StatusMessageParam (hwndDlg, "REMOVING", lpszService); if (hService != NULL) + { CloseServiceHandle (hService); + hService = NULL; + } if (hManager != NULL) + { CloseServiceHandle (hManager); + hManager = NULL; + } hManager = OpenSCManager (NULL, NULL, SC_MANAGER_ALL_ACCESS); if (hManager == NULL) @@ -897,6 +893,8 @@ try_delete: // Second try for an eventual no-install driver instance CloseServiceHandle (hService); CloseServiceHandle (hManager); + hService = NULL; + hManager = NULL; Sleep(1000); firstTry = FALSE; @@ -1944,6 +1942,7 @@ int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, char *lpszComm if (IsAdmin () != TRUE) if (MessageBoxW (NULL, GetString ("SETUP_ADMIN"), lpszTitle, MB_YESNO | MB_ICONQUESTION) != IDYES) { + FinalizeApp (); exit (1); } @@ -2009,6 +2008,7 @@ int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, char *lpszComm else if (!bDevm) { MessageBox (NULL, "Error: This installer file does not contain any compressed files.\n\nTo create a self-extracting installation package (with embedded compressed files), run:\n\"VeraCrypt Setup.exe\" /p", "VeraCrypt", MB_ICONERROR | MB_SETFOREGROUND | MB_TOPMOST); + FinalizeApp (); exit (1); } @@ -2028,6 +2028,7 @@ int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, char *lpszComm bUninstall = TRUE; break; default: + FinalizeApp (); exit (1); } } @@ -2076,6 +2077,6 @@ int WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, char *lpszComm } } } - + FinalizeApp (); return 0; } -- cgit v1.2.3