From 762065917f3ac47c3bdcacdb608d35b36dfb3973 Mon Sep 17 00:00:00 2001 From: Mounir IDRASSI Date: Sat, 26 Mar 2022 20:03:19 +0100 Subject: Windows: Add various checks to address Coverity reported issues. --- src/Common/Dlgcode.c | 10 +++++--- src/Common/Language.c | 3 ++- src/Common/Tests.c | 54 +++++++++++++++++++++++++++++------------ src/Crypto/cpu.c | 4 +-- src/ExpandVolume/ExpandVolume.c | 6 +++++ src/Format/InPlace.c | 2 ++ src/Format/Tcformat.c | 17 +++++++++---- src/Mount/Mount.c | 10 +++++++- src/Setup/Dir.c | 12 +++++++++ src/Setup/Setup.c | 3 ++- src/SetupDLL/Dir.c | 12 +++++++++ 11 files changed, 105 insertions(+), 28 deletions(-) diff --git a/src/Common/Dlgcode.c b/src/Common/Dlgcode.c index 9d5c0d06..7b3d2d45 100644 --- a/src/Common/Dlgcode.c +++ b/src/Common/Dlgcode.c @@ -611,6 +611,7 @@ char *LoadFile (const wchar_t *fileName, DWORD *size) char *buf; DWORD fileSize = INVALID_FILE_SIZE; HANDLE h = CreateFile (fileName, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); + *size = 0; if (h == INVALID_HANDLE_VALUE) return NULL; @@ -620,8 +621,7 @@ char *LoadFile (const wchar_t *fileName, DWORD *size) return NULL; } - *size = fileSize; - buf = (char *) calloc (*size + 1, 1); + buf = (char *) calloc (fileSize + 1, 1); if (buf == NULL) { @@ -629,11 +629,15 @@ char *LoadFile (const wchar_t *fileName, DWORD *size) return NULL; } - if (!ReadFile (h, buf, *size, size, NULL)) + if (!ReadFile (h, buf, fileSize, size, NULL)) { free (buf); buf = NULL; } + else + { + buf[*size] = 0; //make coverity happy eventhough buf is guaranteed to be null terminated because of fileSize+1 in calloc call + } CloseHandle (h); return buf; diff --git a/src/Common/Language.c b/src/Common/Language.c index 844f4dad..278b7dd1 100644 --- a/src/Common/Language.c +++ b/src/Common/Language.c @@ -611,7 +611,8 @@ char *GetPreferredLangId () void SetPreferredLangId (char *langId) { - StringCbCopyA (PreferredLangId, sizeof(PreferredLangId), langId); + if (strlen(langId) < sizeof(PreferredLangId)) + StringCbCopyA (PreferredLangId, sizeof(PreferredLangId), langId); } diff --git a/src/Common/Tests.c b/src/Common/Tests.c index 0fcd93ce..4f53d4ed 100644 --- a/src/Common/Tests.c +++ b/src/Common/Tests.c @@ -1519,12 +1519,20 @@ BOOL test_hmac_sha256 () for (i = 0; i < sizeof (hmac_sha256_test_data) / sizeof(char *); i++) { char digest[1024]; /* large enough to hold digets and test vector inputs */ - memcpy (digest, hmac_sha256_test_data[i], strlen (hmac_sha256_test_data[i])); - hmac_sha256 (hmac_sha256_test_keys[i], (int) strlen (hmac_sha256_test_keys[i]), digest, (int) strlen (hmac_sha256_test_data[i])); - if (memcmp (digest, hmac_sha256_test_vectors[i], SHA256_DIGESTSIZE) != 0) - return FALSE; + size_t dataLen = strlen (hmac_sha256_test_data[i]); + if (dataLen <= sizeof(digest)) + { + memcpy (digest, hmac_sha256_test_data[i], dataLen); + hmac_sha256 (hmac_sha256_test_keys[i], (int) strlen (hmac_sha256_test_keys[i]), digest, (int) dataLen); + if (memcmp (digest, hmac_sha256_test_vectors[i], SHA256_DIGESTSIZE) != 0) + return FALSE; + else + nTestsPerformed++; + } else - nTestsPerformed++; + { + return FALSE; + } } return (nTestsPerformed == 6); @@ -1538,12 +1546,20 @@ BOOL test_hmac_sha512 () for (i = 0; i < sizeof (hmac_sha512_test_data) / sizeof(char *); i++) { char digest[1024]; /* large enough to hold digets and test vector inputs */ - memcpy (digest, hmac_sha512_test_data[i], (int) strlen (hmac_sha512_test_data[i])); - hmac_sha512 (hmac_sha512_test_keys[i], (int) strlen (hmac_sha512_test_keys[i]), digest, (int) strlen (hmac_sha512_test_data[i])); - if (memcmp (digest, hmac_sha512_test_vectors[i], SHA512_DIGESTSIZE) != 0) - return FALSE; + size_t dataLen = strlen (hmac_sha512_test_data[i]); + if (dataLen <= sizeof(digest)) + { + memcpy (digest, hmac_sha512_test_data[i], dataLen ); + hmac_sha512 (hmac_sha512_test_keys[i], (int) strlen (hmac_sha512_test_keys[i]), digest, (int) dataLen); + if (memcmp (digest, hmac_sha512_test_vectors[i], SHA512_DIGESTSIZE) != 0) + return FALSE; + else + nTestsPerformed++; + } else - nTestsPerformed++; + { + return FALSE; + } } return (nTestsPerformed == 6); @@ -1557,12 +1573,20 @@ BOOL test_hmac_blake2s () for (i = 0; i < sizeof (hmac_blake2s_test_data) / sizeof(char *); i++) { char digest[1024]; /* large enough to hold digets and test vector inputs */ - memcpy (digest, hmac_blake2s_test_data[i], strlen (hmac_blake2s_test_data[i])); - hmac_blake2s (hmac_blake2s_test_keys[i], (int) strlen (hmac_blake2s_test_keys[i]), digest, (int) strlen (hmac_blake2s_test_data[i])); - if (memcmp (digest, hmac_blake2s_test_vectors[i], BLAKE2S_DIGESTSIZE) != 0) - return FALSE; + size_t dataLen = strlen (hmac_blake2s_test_data[i]); + if (dataLen <= sizeof(digest)) + { + memcpy (digest, hmac_blake2s_test_data[i], dataLen); + hmac_blake2s (hmac_blake2s_test_keys[i], (int) strlen (hmac_blake2s_test_keys[i]), digest, (int) dataLen); + if (memcmp (digest, hmac_blake2s_test_vectors[i], BLAKE2S_DIGESTSIZE) != 0) + return FALSE; + else + nTestsPerformed++; + } else - nTestsPerformed++; + { + return FALSE; + } } return (nTestsPerformed == 6); diff --git a/src/Crypto/cpu.c b/src/Crypto/cpu.c index 27d62e43..c3a769c8 100644 --- a/src/Crypto/cpu.c +++ b/src/Crypto/cpu.c @@ -280,7 +280,7 @@ static int Detect_MS_HyperV_AES () // when Hyper-V is enabled on older versions of Windows Server (i.e. 2008 R2), the AES-NI capability // gets masked out for all applications, even running on the host. // We try to detect Hyper-V virtual CPU and perform a dummy AES-NI operation to check its real presence - uint32 cpuid[4]; + uint32 cpuid[4] = {0}; char HvProductName[13]; CpuId(0x40000000, cpuid); @@ -348,7 +348,7 @@ void DetectX86Features() g_hasISSE = 1; else { - uint32 cpuid2[4]; + uint32 cpuid2[4] = {0}; CpuId(0x080000000, cpuid2); if (cpuid2[0] >= 0x080000001) { diff --git a/src/ExpandVolume/ExpandVolume.c b/src/ExpandVolume/ExpandVolume.c index 84f6cfe8..f62d93ae 100644 --- a/src/ExpandVolume/ExpandVolume.c +++ b/src/ExpandVolume/ExpandVolume.c @@ -442,6 +442,12 @@ int ExtendFileSystem (HWND hwndDlg , wchar_t *lpszVolume, Password *pVolumePassw goto error; } + if ((BytesPerSector == 0) || (BytesPerSector > (DWORD)INT_MAX)) + { + nStatus = ERR_SECTOR_SIZE_INCOMPATIBLE; + goto error; + } + DebugAddProgressDlgStatus (hwndDlg, L"Extending file system ...\r\n"); // extend volume diff --git a/src/Format/InPlace.c b/src/Format/InPlace.c index 4a16fd4f..f6166dab 100644 --- a/src/Format/InPlace.c +++ b/src/Format/InPlace.c @@ -89,6 +89,8 @@ static __int64 NewFileSysSizeAfterShrink (HANDLE dev, const wchar_t *devicePath, } if ( (ntfsVolData.NumberSectors.QuadPart <= 0) + || (ntfsVolData.BytesPerSector == 0) + || (ntfsVolData.BytesPerSector >= (DWORD) UINT_MAX) || (ntfsVolData.NumberSectors.QuadPart > (INT64_MAX / (__int64) ntfsVolData.BytesPerSector)) // overflow test ) { diff --git a/src/Format/Tcformat.c b/src/Format/Tcformat.c index 477306ea..efd95caf 100644 --- a/src/Format/Tcformat.c +++ b/src/Format/Tcformat.c @@ -9756,11 +9756,18 @@ int AnalyzeHiddenVolumeHost (HWND hwndDlg, int *driveNo, __int64 hiddenVolHostSi // The map will be scanned to determine the size of the uninterrupted block of free // space (provided there is any) whose end is aligned with the end of the volume. // The value will then be used to determine the maximum possible size of the hidden volume. - - return ScanVolClusterBitmap (hwndDlg, - driveNo, - hiddenVolHostSize / *realClusterSize, - pnbrFreeClusters); + if (*realClusterSize > 0) + { + return ScanVolClusterBitmap (hwndDlg, + driveNo, + hiddenVolHostSize / *realClusterSize, + pnbrFreeClusters); + } + else + { + // should never happen + return -1; + } } else if (!wcsncmp (szFileSystemNameBuffer, L"NTFS", 4) || !_wcsnicmp (szFileSystemNameBuffer, L"exFAT", 5)) { diff --git a/src/Mount/Mount.c b/src/Mount/Mount.c index a0370861..a5798afc 100644 --- a/src/Mount/Mount.c +++ b/src/Mount/Mount.c @@ -6583,6 +6583,14 @@ static void ShowSystemEncryptionStatus (HWND hwndDlg) if (GetAsyncKeyState (VK_SHIFT) < 0 && GetAsyncKeyState (VK_CONTROL) < 0) { // Ctrl+Shift held (for debugging purposes) + int64 encryptedRatio = 0; + if (BootEncStatus.DriveEncrypted + && (BootEncStatus.ConfiguredEncryptedAreaStart >= 0) + && (BootEncStatus.ConfiguredEncryptedAreaEnd >= BootEncStatus.ConfiguredEncryptedAreaStart) + ) + { + encryptedRatio = (BootEncStatus.EncryptedAreaEnd + 1 - BootEncStatus.EncryptedAreaStart) * 100I64 / (BootEncStatus.ConfiguredEncryptedAreaEnd + 1 - BootEncStatus.ConfiguredEncryptedAreaStart); + } DebugMsgBox ("Debugging information for system encryption:\n\nDeviceFilterActive: %d\nBootLoaderVersion: %x\nSetupInProgress: %d\nSetupMode: %d\nVolumeHeaderPresent: %d\nDriveMounted: %d\nDriveEncrypted: %d\n" "HiddenSystem: %d\nHiddenSystemPartitionStart: %I64d\n" @@ -6600,7 +6608,7 @@ static void ShowSystemEncryptionStatus (HWND hwndDlg) BootEncStatus.ConfiguredEncryptedAreaEnd, BootEncStatus.EncryptedAreaStart, BootEncStatus.EncryptedAreaEnd, - !BootEncStatus.DriveEncrypted ? 0 : (BootEncStatus.EncryptedAreaEnd + 1 - BootEncStatus.EncryptedAreaStart) * 100I64 / (BootEncStatus.ConfiguredEncryptedAreaEnd + 1 - BootEncStatus.ConfiguredEncryptedAreaStart)); + encryptedRatio); } if (!BootEncStatus.DriveEncrypted && !BootEncStatus.DriveMounted) diff --git a/src/Setup/Dir.c b/src/Setup/Dir.c index 2d4feecd..3275567f 100644 --- a/src/Setup/Dir.c +++ b/src/Setup/Dir.c @@ -31,6 +31,12 @@ mkfulldir (wchar_t *oriPath, BOOL bCheckonly) wchar_t *uniq_file; wchar_t path [TC_MAX_PATH]; + if (wcslen(oriPath) >= TC_MAX_PATH) + { + // directory name will be truncated so return failure to avoid unexepected behavior + return -1; + } + StringCbCopyW (path, TC_MAX_PATH, oriPath); if (wcslen (path) == 3 && path[1] == L':') @@ -66,6 +72,12 @@ mkfulldir_internal (wchar_t *path) static wchar_t tokpath[_MAX_PATH]; static wchar_t trail[_MAX_PATH]; + if (wcslen(path) >= _MAX_PATH) + { + // directory name will be truncated so return failure to avoid unexepected behavior + return -1; + } + StringCbCopyW (tokpath, _MAX_PATH, path); trail[0] = L'\0'; diff --git a/src/Setup/Setup.c b/src/Setup/Setup.c index 9433bd40..43c951f5 100644 --- a/src/Setup/Setup.c +++ b/src/Setup/Setup.c @@ -819,7 +819,8 @@ BOOL DoFilesInstall (HWND hwndDlg, wchar_t *szDestDir) if (Is64BitOs ()) driver64 = TRUE; - GetSystemDirectory (szDir, ARRAYSIZE (szDir)); + if (!GetSystemDirectory (szDir, ARRAYSIZE (szDir))) + StringCbCopyW(szDir, sizeof(szDir), L"C:\\Windows\\System32"); x = wcslen (szDir); if (szDir[x - 1] != L'\\') diff --git a/src/SetupDLL/Dir.c b/src/SetupDLL/Dir.c index 2d4feecd..3275567f 100644 --- a/src/SetupDLL/Dir.c +++ b/src/SetupDLL/Dir.c @@ -31,6 +31,12 @@ mkfulldir (wchar_t *oriPath, BOOL bCheckonly) wchar_t *uniq_file; wchar_t path [TC_MAX_PATH]; + if (wcslen(oriPath) >= TC_MAX_PATH) + { + // directory name will be truncated so return failure to avoid unexepected behavior + return -1; + } + StringCbCopyW (path, TC_MAX_PATH, oriPath); if (wcslen (path) == 3 && path[1] == L':') @@ -66,6 +72,12 @@ mkfulldir_internal (wchar_t *path) static wchar_t tokpath[_MAX_PATH]; static wchar_t trail[_MAX_PATH]; + if (wcslen(path) >= _MAX_PATH) + { + // directory name will be truncated so return failure to avoid unexepected behavior + return -1; + } + StringCbCopyW (tokpath, _MAX_PATH, path); trail[0] = L'\0'; -- cgit v1.2.3