Merged PR 787219: CodeQL cleanup

Changes to make CodeQL happy. The majority of the changes are suppression of CodeQL warnings:
1. eval/function constructions - we use these exclusively in tests
2. weak cryptography - we are not using it for the crypto purposes

Related work items: #2152932

Public/Src/Cache/ContentStore/Test/Exceptions/CacheExceptionBaseTests.cs

@@ -77,6 +77,7 @@ namespace ContentStoreTest.Exceptions
 
                 using (var stream2 = new MemoryStream(stream.ToArray()))
                 {
+                    // CodeQL [SM04191] It is fine not to use Binder here. This is a test, we serialize our own class, and we control its content.
                     var dex = (CacheException)formatter.Deserialize(stream2);
                     dex.Should().NotBeNull();
                 }

Public/Src/Engine/Dll/AppDeployment.cs

@@ -224,11 +224,9 @@ namespace BuildXL.Engine
 
                 try
                 {
-#pragma warning disable CA5351 // Disable CA5351 Do not use insecure cryptographic algorithm MD5
-                    using (MD5 md5Hash = MD5.Create())
-#pragma warning restore CA5351 // Restore CA5351 Do not use insecure cryptographic algorithm MD5
+                    using (SHA256 sha256Hash = SHA256.Create())
                     {
-                        byte[] data = md5Hash.ComputeHash(Encoding.UTF8.GetBytes(File.ReadAllText(Path.Combine(BaseDirectory, path))));
+                        byte[] data = sha256Hash.ComputeHash(Encoding.UTF8.GetBytes(File.ReadAllText(Path.Combine(BaseDirectory, path))));
                         foreach (var b in data)
                         {
                             sb.Append(b.ToString("x2"));

Public/Src/FrontEnd/Core/FrontEndHostController.Nuget.cs

@@ -747,6 +747,7 @@ namespace BuildXL.FrontEnd.Core
                         var response = await client.GetAsync(sourceUri);
                         var stream = await response.Content.ReadAsStreamAsync();
 
+                        // CodeQL [SM00414] The path is prepared by the caller and is controlled elsewhere in FrontEnd.
                         using (var targetStream = new FileStream(targetFilePath, FileMode.Create, FileAccess.Write, FileShare.None))
                         {
                             await stream.CopyToAsync(targetStream);

Public/Src/FrontEnd/TypeScript.Net/TypeScript.Net.UnitTests/Cases/functionType.ts

@@ -1,5 +1,6 @@
 function salt() {}
 salt.apply("hello", []);
+// CodeQL [SM04509] Intentional usage of a function constructor in a test. The executed code is static.
 (new Function("return 5"))();
  
  

Public/Src/FrontEnd/TypeScript.Net/TypeScript.Net.UnitTests/Cases/objectLitGetterSetter.ts

@@ -1,6 +1,7 @@
             var obj = {};
             Object.defineProperty(obj, "accProperty", <PropertyDescriptor>({
                 get: function () {
+                    // CodeQL [SM04509] Intentional usage of eval in a test. The executed code is static.
                     eval("public = 1;");
                     return 11;
                 },

Public/Src/FrontEnd/TypeScript.Net/TypeScript.Net.UnitTests/FailingCases/sourceMapValidationStatements.ts

@@ -74,6 +74,7 @@ function f() {
     (x == 1) ? x + 1 : x - 1;
     x === 1;
     x = z = 40;
+    // CodeQL [SM04509] Intentional usage of eval in a test. The executed code is static.
     eval("y");
     return;
 }

Public/Src/FrontEnd/TypeScript.Net/TypeScript.Net/Incrementality/SpecBindingSymbols.cs

@@ -100,6 +100,7 @@ namespace TypeScript.Net.Incrementality
         private static string ComputeFingerprint(byte[] data, int size)
         {
 #pragma warning disable SYSLIB0021 // Type or member is obsolete. Temporarily suppressing the warning for .net 6. Work item: 1885580
+            // CodeQL [SM02196] The hash is not used for a cryptographic purpose
             using (SHA1CryptoServiceProvider sha1 = new SHA1CryptoServiceProvider())
 #pragma warning restore SYSLIB0021 // Type or member is obsolete
             {

Public/Src/FrontEnd/TypeScript.Net/TypeScript.Net/TypeScriptImpl/tsc.ts

@@ -158,7 +158,7 @@ namespace ts {
                 const lineEnd = i < lastLineInFile ? getPositionOfLineAndCharacter(file, i + 1, 0) : file.text.length;
                 let lineContent = file.text.slice(lineStart, lineEnd);
                 lineContent = lineContent.replace(/\s+$/g, "");  // trim from end
-                lineContent = lineContent.replace("\t", " ");    // convert tabs to single spaces
+                lineContent = lineContent.replace(/\t/g, " ");   // convert tabs to single spaces
 
                 // Output the gutter and the actual contents of the line.
                 output += formatAndReset(padLeft(i + 1 + "", gutterWidth), gutterStyleSequence) + gutterSeparator;

Public/Src/IDE/Generator.Old/MsbuildFile.cs

@@ -99,6 +99,7 @@ namespace BuildXL.Ide.Generator.Old
         private static string GenerateGuid(string path)
         {
 #pragma warning disable CA5351 // Do not use insecure cryptographic algorithm MD5.
+            // CodeQL [SM02196] The hash is not used for a cryptographic purpose
             using (var hash = MD5.Create())
 #pragma warning restore CA5351 // Do not use insecure cryptographic algorithm MD5.
             {

Public/Src/IDE/Generator.Old/MsbuildWriter.cs

@@ -604,6 +604,7 @@ namespace BuildXL.Ide.Generator.Old
         private static string GetGuidFromString(string value)
         {
 #pragma warning disable CA5351 // Do not use insecure cryptographic algorithm MD5.
+            // CodeQL [SM02196] The hash is not used for a cryptographic purpose
             using (var hash = MD5.Create())
 #pragma warning restore CA5351 // Do not use insecure cryptographic algorithm MD5.
             {

Public/Src/IDE/Generator/MsbuildFile.cs

@@ -106,6 +106,7 @@ namespace BuildXL.Ide.Generator
         private static string GenerateGuid(string path)
         {
 #pragma warning disable CA5351 // Do not use insecure cryptographic algorithm MD5.
+            // CodeQL [SM02196] The hash is not used for a cryptographic purpose
             using (var hash = MD5.Create())
 #pragma warning restore CA5351 // Do not use insecure cryptographic algorithm MD5.
             {

Public/Src/IDE/Generator/MsbuildWriter.cs

@@ -616,6 +616,7 @@ namespace BuildXL.Ide.Generator
         private static string GetGuidFromString(string value)
         {
 #pragma warning disable CA5351 // Do not use insecure cryptographic algorithm MD5.
+            // CodeQL [SM02196] The hash is not used for a cryptographic purpose
             using (var hash = MD5.Create())
 #pragma warning restore CA5351 // Do not use insecure cryptographic algorithm MD5.
             {

Public/Src/Sandbox/Windows/Detours/Lib/uimports.cpp

@@ -18,7 +18,7 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
                               HMODULE hModule,
                               __in_ecount(nDlls) LPCSTR *plpDlls,
                               DWORD nDlls,
-							  PBYTE* pbClr)
+                              PBYTE* pbClr)
 {
     BOOL fSucceeded = FALSE;
     BYTE * pbNew = NULL;
@@ -59,16 +59,16 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
     }
 
     // Zero out the bound table so loader doesn't use it instead of our new table.
-	// When the Windows loader loads a PE file into memory, it examines the list of
-	// import descriptors and their associated import address table (IAT) to load 
-	// the required DLLs into the process address space. More precisely, the loader
-	// overwrites entries of IAT, IMAGE_THUNK_DATA, with the addresses of imported
-	// functions. As this step takes time, one can use bind.exe to calculate and
-	// bound these addresses apriori. The information that the loader uses to determine
-	// if the bound addresses are valid is kept in IMAGE_BOUND_IMPORT_DESCRIPTOR
-	// structure, and the address to the first element of array of 
-	// IMAGE_BOUND_IMPORT_DESCRIPTOR sturctures is recoreded in 
-	// OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BOUND_IMPORT].
+    // When the Windows loader loads a PE file into memory, it examines the list of
+    // import descriptors and their associated import address table (IAT) to load 
+    // the required DLLs into the process address space. More precisely, the loader
+    // overwrites entries of IAT, IMAGE_THUNK_DATA, with the addresses of imported
+    // functions. As this step takes time, one can use bind.exe to calculate and
+    // bound these addresses apriori. The information that the loader uses to determine
+    // if the bound addresses are valid is kept in IMAGE_BOUND_IMPORT_DESCRIPTOR
+    // structure, and the address to the first element of array of 
+    // IMAGE_BOUND_IMPORT_DESCRIPTOR sturctures is recoreded in 
+    // OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BOUND_IMPORT].
     inh.BOUND_DIRECTORY.VirtualAddress = 0;
     inh.BOUND_DIRECTORY.Size = 0;
 
@@ -94,10 +94,10 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
         DETOUR_TRACE(("ish[%d] : va=%08x sr=%d\n", i, ish.VirtualAddress, ish.SizeOfRawData));
 
         // If the file didn't have an IAT_DIRECTORY, we assign it...
-		// It is known that some linkers do not set this directory entry and the application
-		// will run nevertheless. The loader only uses this to temporarily mark the IATs
-		// as read-write during import resolution, but can resolve the imports without it.
-		// It is still unknown why we have to assign IAT_DIRECTORY if we don't have one.
+        // It is known that some linkers do not set this directory entry and the application
+        // will run nevertheless. The loader only uses this to temporarily mark the IATs
+        // as read-write during import resolution, but can resolve the imports without it.
+        // It is still unknown why we have to assign IAT_DIRECTORY if we don't have one.
         if (inh.IAT_DIRECTORY.VirtualAddress == 0 &&
             inh.IMPORT_DIRECTORY.VirtualAddress >= ish.VirtualAddress &&
             inh.IMPORT_DIRECTORY.VirtualAddress < ish.VirtualAddress + ish.SizeOfRawData) {
@@ -124,30 +124,30 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
                   (DWORD_PTR)pbModule + inh.IMPORT_DIRECTORY.VirtualAddress +
                   inh.IMPORT_DIRECTORY.Size));
 
-	// We need to move all the IMAGE_IMPORT_DESCRIPTORs (IIDs) to a location where
-	// there is a plenty of space. We first have to calculate the amount of space
-	// that we need.
+    // We need to move all the IMAGE_IMPORT_DESCRIPTORs (IIDs) to a location where
+    // there is a plenty of space. We first have to calculate the amount of space
+    // that we need.
 
-	// Space for IIDs of DLLs to be injected.
+    // Space for IIDs of DLLs to be injected.
     DWORD obRem = sizeof(IMAGE_IMPORT_DESCRIPTOR) * nDlls;
 
-	// Space for IIDs of all DLLs (existing and to-be injected ones).
+    // Space for IIDs of all DLLs (existing and to-be injected ones).
     DWORD obTab = PadToDwordPtr(obRem +
                                 inh.IMPORT_DIRECTORY.Size +
                                 sizeof(IMAGE_IMPORT_DESCRIPTOR));
 
-	// For XX-bit process, we need 2 * XX-bit space for each to-be injected DLL
-	// for storing IMAGE_ORDINAL_FLAG_XX for the IMAGE_THUNK_DATA_XX.
+    // For XX-bit process, we need 2 * XX-bit space for each to-be injected DLL
+    // for storing IMAGE_ORDINAL_FLAG_XX for the IMAGE_THUNK_DATA_XX.
     DWORD obDll = obTab + sizeof(DWORD_XX) * 4 * nDlls;
     DWORD obStr = obDll;
     DWORD cbNew = obStr;
 
-	// Space for the name of the DLLs that are going to be injected (see Name1 field of IID).
+    // Space for the name of the DLLs that are going to be injected (see Name1 field of IID).
     for (DWORD n = 0; n < nDlls; n++) {
         cbNew += PadToDword((DWORD)strlen(plpDlls[n]) + 1);
     }
 
-	// Allocate in-memory buffer.
+    // Allocate in-memory buffer.
     pbNew = new BYTE [cbNew];
     if (pbNew == NULL) {
         DETOUR_TRACE(("new BYTE [cbNew] failed.\n"));
@@ -166,8 +166,8 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
     }
     DETOUR_TRACE(("pbBase = %p\n", pbBase));
 
-	// Allocate space in the PE file for moving the IIDs.
-	PBYTE pbNewIid = FindAndAllocateNearBase(hProcess, pbBase, cbNew);
+    // Allocate space in the PE file for moving the IIDs.
+    PBYTE pbNewIid = FindAndAllocateNearBase(hProcess, pbBase, cbNew);
     if (pbNewIid == NULL) {
         DETOUR_TRACE(("FindAndAllocateNearBase failed.\n"));
         goto finish;
@@ -187,8 +187,8 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
 #endif
         DETOUR_TRACE(("IMPORT_DIRECTORY perms=%x\n", dwProtect));
 
-		// Read existing IIDs into the in-memory buffer, but place them past
-		// the space for the IIDs of the to-be injected DLLs.
+        // Read existing IIDs into the in-memory buffer, but place them past
+        // the space for the IIDs of the to-be injected DLLs.
         if (!ReadProcessMemory(hProcess,
                                pbModule + inh.IMPORT_DIRECTORY.VirtualAddress,
                                pbNew + obRem,
@@ -201,7 +201,7 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
     PIMAGE_IMPORT_DESCRIPTOR piid = (PIMAGE_IMPORT_DESCRIPTOR)pbNew;
     DWORD_XX *pt;
 
-	// Create an IID for each DLL to be injected.
+    // Create an IID for each DLL to be injected.
     for (DWORD n = 0; n < nDlls; n++) {
 
         if (cbNew < obStr) {
@@ -209,7 +209,7 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
             goto finish;
         }
 
-		// Copy the string name of the DLL to the in-memory buffer.
+        // Copy the string name of the DLL to the in-memory buffer.
         HRESULT hrRet = StringCchCopyA((char*)pbNew + obStr, cbNew - obStr, plpDlls[n]);
         if (FAILED(hrRet))
         {
@@ -217,7 +217,7 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
             goto finish;
         }
 
-		// Set values for the IID.
+        // Set values for the IID.
         DWORD nOffset = obTab + (sizeof(DWORD_XX) * (4 * n));
         piid[n].OriginalFirstThunk = obBase + nOffset;
         pt = ((DWORD_XX*)(pbNew + nOffset));
@@ -233,12 +233,13 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
         piid[n].ForwarderChain = 0;
         piid[n].Name = obBase + obStr;
 
-		// Update available buffer for copying string name of next DLL.
+        // Update available buffer for copying string name of next DLL.
         obStr += PadToDword((DWORD)strlen(plpDlls[n]) + 1);
     }
 
-	// Print the IIDs in the in-memory buffer.
-    for (DWORD i = 0; i < nDlls + (inh.IMPORT_DIRECTORY.Size / sizeof(*piid)); i++) {
+    // Print the IIDs in the in-memory buffer.
+    DWORD nOldDlls = inh.IMPORT_DIRECTORY.Size / sizeof(*piid);
+    for (DWORD i = 0; i < nDlls + nOldDlls; i++) {
         DETOUR_TRACE(("%8d. Look=%08x Time=%08x Fore=%08x Name=%08x Addr=%08x\n",
                       i,
                       piid[i].OriginalFirstThunk,
@@ -251,7 +252,7 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
         }
     }
 
-	// Write the IIDs in the in-memory buffer to the allocated space in the PE file.
+    // Write the IIDs in the in-memory buffer to the allocated space in the PE file.
     if (!WriteProcessMemory(hProcess, pbNewIid, pbNew, obStr, NULL)) {
         DETOUR_TRACE_ERROR(L"WriteProcessMemory(iid) failed: %d\n", GetLastError());
         goto finish;
@@ -268,22 +269,22 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
         inh.IAT_DIRECTORY.Size = cbNew;
     }
 
-	// Update the import directory in the PE header.
+    // Update the import directory in the PE header.
     inh.IMPORT_DIRECTORY.VirtualAddress = obBase;
     inh.IMPORT_DIRECTORY.Size = cbNew;
 
-	//////////////////////// Get the CLR header.
+    //////////////////////// Get the CLR header.
 #if BUILDXL_DETOURS
 
-	*pbClr = NULL;
+    *pbClr = NULL;
 
-	if (inh.CLR_DIRECTORY.VirtualAddress != 0 && inh.CLR_DIRECTORY.Size != 0) {
-		DETOUR_TRACE(("CLR.VirtAddr=%x, CLR.Size=%x\n", inh.CLR_DIRECTORY.VirtualAddress, inh.CLR_DIRECTORY.Size));
-		*pbClr = ((PBYTE)hModule) + inh.CLR_DIRECTORY.VirtualAddress;
-	}
+    if (inh.CLR_DIRECTORY.VirtualAddress != 0 && inh.CLR_DIRECTORY.Size != 0) {
+        DETOUR_TRACE(("CLR.VirtAddr=%x, CLR.Size=%x\n", inh.CLR_DIRECTORY.VirtualAddress, inh.CLR_DIRECTORY.Size));
+        *pbClr = ((PBYTE)hModule) + inh.CLR_DIRECTORY.VirtualAddress;
+    }
 
 #else
-	*pbClr += 0;
+    *pbClr += 0;
 #endif // BUILDXL_DETOURS
 
     /////////////////////// Update the NT header for the new import directory.
@@ -301,14 +302,14 @@ static BOOL UPDATE_IMPORTS_XX(HANDLE hProcess,
     inh.OptionalHeader.CheckSum = 0;
 #endif // IGNORE_CHECKSUMS
 
-	// Overwrite DOS header with the updated one.
+    // Overwrite DOS header with the updated one.
     if (!WriteProcessMemory(hProcess, pbModule, &idh, sizeof(idh), NULL)) {
         DETOUR_TRACE_ERROR(L"WriteProcessMemory(idh) failed: %d\n", GetLastError());
         goto finish;
     }
     DETOUR_TRACE(("WriteProcessMemory(idh:%p..%p)\n", pbModule, pbModule + sizeof(idh)));
 
-	// Overwrite PE header with the updated one.
+    // Overwrite PE header with the updated one.
     if (!WriteProcessMemory(hProcess, pbModule + idh.e_lfanew, &inh, sizeof(inh), NULL)) {
         DETOUR_TRACE_ERROR(L"WriteProcessMemory(inh) failed: %d\n", GetLastError());
         goto finish;

Public/Src/Sandbox/Windows/DetoursServices/PathTree.cpp

@@ -51,7 +51,7 @@ bool PathTree::TryInsert(const std::wstring& path)
 
     TreeNode* currentNode = m_root;
 
-    for (unsigned int i = 0; i < elements.size(); i++)
+    for (std::vector<std::wstring>::size_type i = 0; i < elements.size(); i++)
     {
         // Only the last element is a final node
         const bool isIntermediate = (i != (elements.size() - 1));
@@ -199,7 +199,7 @@ bool PathTree::TryFind(const std::wstring& path, std::vector<std::pair<std::wstr
 
     nodeTrace.push_back(std::make_pair(L"", m_root));
 
-    for (unsigned int i = 0; i < elements.size(); i++)
+    for (std::vector<std::wstring>::size_type i = 0; i < elements.size(); i++)
     {
         std::pair<std::wstring, TreeNode*> search;
         if (!currentNode->children.find(elements[i], search))

Public/Src/Utilities/KeyValueStore/KeyValueStoreAccessor.cs

@@ -224,9 +224,7 @@ namespace BuildXL.Engine.Cache.KeyValueStores
             {
                 if (!s_accessorVersionHash.HasValue)
                 {
-#pragma warning disable CA5351 // Disable Warning Do Not Use Broken Cryptographic Algorithms
-                    using (var hashAlgorithm = MD5.Create())
-#pragma warning restore CA5351 // Restore Warning Do Not Use Broken Cryptographic Algorithms
+                    using (var hashAlgorithm = SHA256.Create())
                     {
                         byte[] data = hashAlgorithm.ComputeHash(System.Text.Encoding.UTF8.GetBytes(AccessorVersionString));
 

Public/Src/Utilities/Native/IO/Unix/FileSystem.Unix.cs

@@ -956,6 +956,7 @@ namespace BuildXL.Native.IO.Unix
 
             if (followSymlink)
             {
+                // CodeQL [SM00414] Path is controlled in FrontEnd (marked by code ql due to a code path from FrontEndHostController.Nuget.cs).
                 return Directory.Exists(path)
                     ? PathExistence.ExistsAsDirectory
                     : PathExistence.ExistsAsFile;

Public/Src/Utilities/Native/IO/Unix/FileUtilities.Unix.cs

@@ -453,6 +453,7 @@ namespace BuildXL.Native.IO.Unix
 
                         if (onCompletion != null)
                         {
+                            // CodeQL [SM00414] Path is controlled in FrontEnd (marked by code ql due to a code path from FrontEndHostController.Nuget.cs).
                             using (var dest = File.Open(destination, FileMode.Open, FileAccess.Read, FileShare.Delete))
                             {
                                 onCompletion(sourceStream.SafeFileHandle, dest.SafeFileHandle);

Public/Src/Utilities/Storage/Fingerprints/FingerprintUtilities.cs

@@ -26,6 +26,7 @@ namespace BuildXL.Storage.Fingerprints
         /// by itself that causes a lots of allocations (that supposedly should be avoided all together).
         /// </remarks>
 #pragma warning disable SYSLIB0021, CA5350 // Type or member is obsolete. Temporarily suppressing the warning for .net 6. Work item: 1885580, disable CA5350 Do Not Use Weak Cryptographic Algorithms
+        // CodeQL [SM02196] The hashes are not used for a cryptographic purpose
         private static readonly ObjectPool<SHA1Managed> s_hashers = new ObjectPool<SHA1Managed>(() => new SHA1Managed(), hasher => { return hasher; });
 #pragma warning restore SYSLIB0021, CA5350 // Type or member is obsolete
         private const string UnexpectedLengthContractViolationMessage = "Created Fingerprint is of length different from 'FingerprintLength'";

Public/Src/Utilities/Storage/Fingerprints/HashAlgorithmExtensions.cs

@@ -17,6 +17,7 @@ namespace BuildXL.Storage.Fingerprints
             {
                 case HashAlgorithmType.SHA1Managed:
 #pragma warning disable SYSLIB0021, CA5350 // Type or member is obsolete. Temporarily suppressing the warning for .net 6. Work item: 1885580, disable CA5350 Do Not Use Weak Cryptographic Algorithms
+                    // CodeQL [SM02196] The hash is not used for a cryptographic purpose
                     return new SHA1Managed();
 #pragma warning restore SYSLIB0021, CA5350 // Type or member is obsolete
                 case HashAlgorithmType.MurmurHash3: