Make CodeQL Clean (release/6.0) (#8536)

Resolve remaining CodeQL errors.

This also brings in #8247 to get the same template arrangement as the main branch.

Co-authored-by: Michelle McDaniel <michelm@microsoft.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>

azure-pipelines-codeql.yml

@@ -44,4 +44,4 @@ stages:
             -TsaIterationPath $(_TsaIterationPath)
             -TsaRepositoryName "Arcade"
             -TsaCodebaseName "Arcade"
-            -TsaPublish $False'
+            -TsaPublish $True'

eng/common/templates/job/execute-sdl.yml

@@ -43,14 +43,9 @@ jobs:
       value: ${{ parameters.AzDOPipelineId }}
     - name: AzDOBuildId
       value: ${{ parameters.AzDOBuildId }}
-    # The Guardian version specified in 'eng/common/sdl/packages.config'. This value must be kept in
-    # sync with the packages.config file.
-    - name: DefaultGuardianVersion
-      value: 0.110.1
+    - template: /eng/common/templates/variables/sdl-variables.yml
     - name: GuardianVersion
       value: ${{ coalesce(parameters.overrideGuardianVersion, '$(DefaultGuardianVersion)') }}
-    - name: GuardianPackagesConfigFile
-      value: $(Build.SourcesDirectory)\eng\common\sdl\packages.config
   pool:
     # We don't use the collection uri here because it might vary (.visualstudio.com vs. dev.azure.com)
     ${{ if eq(variables['System.TeamProject'], 'DevDiv') }}:
@@ -126,57 +121,11 @@ jobs:
       displayName: Extract Archive Artifacts
       continueOnError: ${{ parameters.sdlContinueOnError }}
 
-  - ${{ if ne(parameters.overrideGuardianVersion, '') }}:
-    - powershell: |
-        $content = Get-Content $(GuardianPackagesConfigFile)
-
-        Write-Host "packages.config content was:`n$content"
-
-        $content = $content.Replace('$(DefaultGuardianVersion)', '$(GuardianVersion)')
-        $content | Set-Content $(GuardianPackagesConfigFile)
-
-        Write-Host "packages.config content updated to:`n$content"
-      displayName: Use overridden Guardian version ${{ parameters.overrideGuardianVersion }}
-
-  - task: NuGetToolInstaller@1
-    displayName: 'Install NuGet.exe'
-  - task: NuGetCommand@2
-    displayName: 'Install Guardian'
-    inputs:
-      restoreSolution: $(Build.SourcesDirectory)\eng\common\sdl\packages.config
-      feedsToUse: config
-      nugetConfigPath: $(Build.SourcesDirectory)\eng\common\sdl\NuGet.config
-      externalFeedCredentials: GuardianConnect
-      restoreDirectory: $(Build.SourcesDirectory)\.packages
-
-  - ${{ if ne(parameters.overrideParameters, '') }}:
-    - powershell: ${{ parameters.executeAllSdlToolsScript }} ${{ parameters.overrideParameters }}
-      displayName: Execute SDL
-      continueOnError: ${{ parameters.sdlContinueOnError }}
-  - ${{ if eq(parameters.overrideParameters, '') }}:
-    - powershell: ${{ parameters.executeAllSdlToolsScript }}
-        -GuardianPackageName Microsoft.Guardian.Cli.$(GuardianVersion)
-        -NugetPackageDirectory $(Build.SourcesDirectory)\.packages
-        -AzureDevOpsAccessToken $(dn-bot-dotnet-build-rw-code-rw)
-        ${{ parameters.additionalParameters }}
-      displayName: Execute SDL
-      continueOnError: ${{ parameters.sdlContinueOnError }}
-
-  - ${{ if ne(parameters.publishGuardianDirectoryToPipeline, 'false') }}:
-    # We want to publish the Guardian results and configuration for easy diagnosis. However, the
-    # '.gdn' dir is a mix of configuration, results, extracted dependencies, and Guardian default
-    # tooling files. Some of these files are large and aren't useful during an investigation, so
-    # exclude them by simply deleting them before publishing. (As of writing, there is no documented
-    # way to selectively exclude a dir from the pipeline artifact publish task.)
-    - task: DeleteFiles@1
-      displayName: Delete Guardian dependencies to avoid uploading
-      inputs:
-        SourceFolder: $(Agent.BuildDirectory)/.gdn
-        Contents: |
-          c
-          i
-      condition: succeededOrFailed()
-    - publish: $(Agent.BuildDirectory)/.gdn
-      artifact: GuardianConfiguration
-      displayName: Publish GuardianConfiguration
-      condition: succeededOrFailed()
+  - template: /eng/common/templates/steps/execute-sdl.yml
+    parameters:
+      overrideGuardianVersion: ${{ parameters.overrideGuardianVersion }}
+      executeAllSdlToolsScript: ${{ parameters.executeAllSdlToolsScript }}
+      overrideParameters: ${{ parameters.overrideParameters }}
+      additionalParameters: ${{ parameters.additionalParameters }}
+      publishGuardianDirectoryToPipeline: ${{ parameters.publishGuardianDirectoryToPipeline }}
+      sdlContinueOnError: ${{ parameters.sdlContinueOnError }}

eng/common/templates/jobs/codeql-build.yml

@@ -21,7 +21,7 @@ jobs:
       # The Guardian version specified in 'eng/common/sdl/packages.config'. This value must be kept in
       # sync with the packages.config file.
       - name: DefaultGuardianVersion
-        value: 0.109.0
+        value: 0.110.1
       - name: GuardianPackagesConfigFile
         value: $(Build.SourcesDirectory)\eng\common\sdl\packages.config
       - name: GuardianVersion

eng/common/templates/steps/execute-sdl.yml

@@ -0,0 +1,68 @@
+parameters:
+  overrideGuardianVersion: ''
+  executeAllSdlToolsScript: ''
+  overrideParameters: ''
+  additionalParameters: ''
+  publishGuardianDirectoryToPipeline: false
+  sdlContinueOnError: false
+  condition: ''
+
+steps:
+- ${{ if ne(parameters.overrideGuardianVersion, '') }}:
+  - powershell: |
+      $content = Get-Content $(GuardianPackagesConfigFile)
+
+      Write-Host "packages.config content was:`n$content"
+
+      $content = $content.Replace('$(DefaultGuardianVersion)', '$(GuardianVersion)')
+      $content | Set-Content $(GuardianPackagesConfigFile)
+
+      Write-Host "packages.config content updated to:`n$content"
+    displayName: Use overridden Guardian version ${{ parameters.overrideGuardianVersion }}
+
+- task: NuGetToolInstaller@1
+  displayName: 'Install NuGet.exe'
+
+- task: NuGetCommand@2
+  displayName: 'Install Guardian'
+  inputs:
+    restoreSolution: $(Build.SourcesDirectory)\eng\common\sdl\packages.config
+    feedsToUse: config
+    nugetConfigPath: $(Build.SourcesDirectory)\eng\common\sdl\NuGet.config
+    externalFeedCredentials: GuardianConnect
+    restoreDirectory: $(Build.SourcesDirectory)\.packages
+
+- ${{ if ne(parameters.overrideParameters, '') }}:
+  - powershell: ${{ parameters.executeAllSdlToolsScript }} ${{ parameters.overrideParameters }}
+    displayName: Execute SDL
+    continueOnError: ${{ parameters.sdlContinueOnError }}
+    condition: ${{ parameters.condition }}
+
+- ${{ if eq(parameters.overrideParameters, '') }}:
+  - powershell: ${{ parameters.executeAllSdlToolsScript }}
+      -GuardianPackageName Microsoft.Guardian.Cli.$(GuardianVersion)
+      -NugetPackageDirectory $(Build.SourcesDirectory)\.packages
+      -AzureDevOpsAccessToken $(dn-bot-dotnet-build-rw-code-rw)
+      ${{ parameters.additionalParameters }}
+    displayName: Execute SDL
+    continueOnError: ${{ parameters.sdlContinueOnError }}
+    condition: ${{ parameters.condition }}
+
+- ${{ if ne(parameters.publishGuardianDirectoryToPipeline, 'false') }}:
+  # We want to publish the Guardian results and configuration for easy diagnosis. However, the
+  # '.gdn' dir is a mix of configuration, results, extracted dependencies, and Guardian default
+  # tooling files. Some of these files are large and aren't useful during an investigation, so
+  # exclude them by simply deleting them before publishing. (As of writing, there is no documented
+  # way to selectively exclude a dir from the pipeline artifact publish task.)
+  - task: DeleteFiles@1
+    displayName: Delete Guardian dependencies to avoid uploading
+    inputs:
+      SourceFolder: $(Agent.BuildDirectory)/.gdn
+      Contents: |
+        c
+        i
+    condition: succeededOrFailed()
+  - publish: $(Agent.BuildDirectory)/.gdn
+    artifact: GuardianConfiguration
+    displayName: Publish GuardianConfiguration
+    condition: succeededOrFailed()

eng/common/templates/variables/sdl-variables.yml

@@ -0,0 +1,7 @@
+variables:
+# The Guardian version specified in 'eng/common/sdl/packages.config'. This value must be kept in
+# sync with the packages.config file.
+- name: DefaultGuardianVersion
+  value: 0.110.1
+- name: GuardianPackagesConfigFile
+  value: $(Build.SourcesDirectory)\eng\common\sdl\packages.config

src/Common/Microsoft.Arcade.Test.Common/FakeHttpClient.cs

@@ -12,8 +12,7 @@ namespace Microsoft.DotNet.Arcade.Test.Common
     public static class FakeHttpClient
     {
         public static HttpClient WithResponses(params HttpResponseMessage[] responses)
-            => new HttpClient(
-                new FakeHttpMessageHandler(responses));
+            => new HttpClient(new FakeHttpMessageHandler(responses)); // lgtm [cs/httpclient-checkcertrevlist-disabled] This is used for unit tests
 
         private class FakeHttpMessageHandler : HttpMessageHandler
         {

src/Microsoft.DotNet.Build.Tasks.Feed.Tests/BuildModelFactoryTests.cs

@@ -497,7 +497,7 @@ public void RoundTripFromTaskItemsToFileToXml()
                     item =>
                     {
                         item.Include.Should().Be("Microsoft.DiaSymReader.dll");
-                        item.CertificateName.Should().Be("Microsoft101240624");
+                        item.CertificateName.Should().Be("Microsoft101240624"); // lgtm [cs/common-default-passwords] Safe, these are certificate names
                         item.TargetFramework.Should().Be(".NETStandard,Version=v1.1");
                         item.PublicKeyToken.Should().Be("31bf3856ad364e35");
                     },

src/Microsoft.DotNet.Build.Tasks.Feed/src/ExecWithRetriesForNuGetPush.cs

@@ -202,7 +202,7 @@ private bool FeedContainsIdenticalPackage()
                     $"Downloading package from '{packageUrl}' " +
                     $"to check if identical to '{PackageFile}'");
 
-                using (var client = new HttpClient
+                using (var client = new HttpClient(new HttpClientHandler() { CheckCertificateRevocationList = true })
                 {
                     Timeout = TimeSpan.FromMinutes(10)
                 })

src/Microsoft.DotNet.Build.Tasks.Feed/src/common/AzureStorageUtils.cs

@@ -37,7 +37,11 @@ public class AzureStorageUtils
 
         public BlobContainerClient Container { get; set; }
 
-        private static readonly HttpClient s_httpClient = new HttpClient { Timeout = TimeSpan.FromSeconds(300) };
+        private static readonly HttpClient s_httpClient = new HttpClient(
+            new HttpClientHandler() { CheckCertificateRevocationList = true }) 
+            {
+                Timeout = TimeSpan.FromSeconds(300) 
+            };
         private static readonly BlobClientOptions s_clientOptions = new BlobClientOptions()
         {
             Transport = new HttpClientTransport(s_httpClient)
@@ -59,7 +63,7 @@ public BlockBlobClient GetBlockBlob(string destinationBlob) =>
 
         public static string CalculateMD5(string filename)
         {
-            using (var md5 = MD5.Create())
+            using (var md5 = MD5.Create()) // lgtm [cs/weak-crypto] Azure Storage specifies use of MD5
             {
                 using (var stream = File.OpenRead(filename))
                 {

src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateGuidFromName.cs

@@ -21,7 +21,7 @@ public override bool Execute()
             // Any fixed GUID will do for a namespace.
             Guid namespaceId = new Guid("28F1468D-672B-489A-8E0C-7C5B3030630C");
 
-            using (SHA1 hasher = SHA1.Create())
+            using (SHA1 hasher = SHA1.Create()) // lgtm [cs/weak-crypto] Algorithm required by specification
             {
                 var nameBytes = System.Text.Encoding.UTF8.GetBytes(Name ?? string.Empty);
                 var namespaceBytes = namespaceId.ToByteArray();

src/Microsoft.DotNet.GitSync/Program.cs

@@ -18,6 +18,7 @@
 using Repository = LibGit2Sharp.Repository;
 using Microsoft.Azure.Storage;
 using Microsoft.Azure.CosmosDB.Table;
+using System.Collections.Concurrent;
 
 namespace Microsoft.DotNet.GitSync
 {
@@ -26,8 +27,8 @@ internal class Program
         private const string TableName = "CommitHistory";
         private const string RepoTableName = "MirrorBranchRepos";
         private static CloudTable s_table;
-        private static Dictionary<(string, string), List<string>> s_repos { get; set; } = new Dictionary<(string, string), List<string>>();
-        private static Dictionary<string, HashSet<string>> s_branchRepoPairs = new Dictionary<string, HashSet<string>>();
+        private static ConcurrentDictionary<(string, string), List<string>> s_repos { get; set; } = new ConcurrentDictionary<(string, string), List<string>>();
+        private static ConcurrentDictionary<string, HashSet<string>> s_branchRepoPairs = new ConcurrentDictionary<string, HashSet<string>>();
         private ConfigFile ConfigFile { get; }
         private static Lazy<GitHubClient> _lazyClient;
         private static EmailManager s_emailManager;
@@ -556,7 +557,7 @@ private static void Setup(string connectionString, string server, string destina
                 string branchName = item["Branch"].StringValue;
                 string[] targetRepos = item["ReposToMirrorInto"].StringValue.Split(';');
 
-                s_repos.Add((item.PartitionKey, branchName), targetRepos.ToList());
+                s_repos[(item.PartitionKey, branchName)] = targetRepos.ToList();
 
                 if (s_branchRepoPairs.ContainsKey(branchName))
                 {
@@ -567,7 +568,7 @@ private static void Setup(string connectionString, string server, string destina
                 }
                 else
                 {
-                    s_branchRepoPairs.Add(branchName, targetRepos.ToHashSet());
+                    s_branchRepoPairs[branchName] = targetRepos.ToHashSet();
                 }
 
                 s_logger.Info($"The commits in  {item.PartitionKey} repo will be mirrored into {item["ReposToMirrorInto"].StringValue} Repos");

src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

@@ -646,7 +646,7 @@ public void CrossGenerated()
             var strongNameSignInfo = new Dictionary<string, List<SignInfo>>()
             {
                 { "7cec85d7bea7798e", new List<SignInfo>{ new SignInfo("ArcadeCertTest", "ArcadeStrongTest", "123") } },
-                { "adb9793829ddae60", new List<SignInfo>{ new SignInfo("Microsoft400", "AspNetCore", "123") } }
+                { "adb9793829ddae60", new List<SignInfo>{ new SignInfo("Microsoft400", "AspNetCore", "123") } } // lgtm [cs/common-default-passwords] Safe, the are certificate names
             };
 
             // Overriding information

src/Microsoft.DotNet.SignTool/src/Configuration.cs

@@ -643,7 +643,7 @@ private bool TryBuildZipData(FileSignInfo zipFileSignInfo, out ZipData zipData,
 
                     foreach (ZipArchiveEntry entry in archive.Entries)
                     {
-                        string relativePath = entry.FullName;
+                        string relativePath = entry.FullName; // lgtm [cs/zipslip] Archive from trusted source
 
                         // `entry` might be just a pointer to a folder. We skip those.
                         if (relativePath.EndsWith("/") && entry.Name == "")

src/Microsoft.DotNet.VersionTools/lib/src/Automation/GitHubAuth.cs

@@ -22,7 +22,7 @@ public GitHubAuth(
             }
             AuthToken = authToken;
             User = user ?? "dotnet-bot";
-            Email = email ?? "dotnet-bot@microsoft.com";
+            Email = email ?? "dotnet-bot@microsoft.com"; // lgtm [cs/hard-coded-id] ID is correct for this tool
         }
     }
 }

src/Microsoft.DotNet.VersionTools/tests/BuildManifest/ManifestModelTests.cs

@@ -762,7 +762,7 @@ private BuildModel CreateSigningInformationBuildManifestModel()
                         {
                             Include = "StrongButKindName",
                             PublicKeyToken = "fedcba9876543210",
-                            CertificateName = "Microsoft404",
+                            CertificateName = "Microsoft404",  // lgtm [cs/common-default-passwords] Safe, these are certificate names
                         },
                     },
                 },

src/SignCheck/Microsoft.SignCheck/Verification/ArchiveVerifier.cs

@@ -37,13 +37,13 @@ protected void VerifyContent(SignatureVerificationResult svr)
                         // Generate an alias for the actual file that has the same extension. We do this to avoid path too long errors so that
                         // containers can be flattened.
                         string directoryName = Path.GetDirectoryName(archiveEntry.FullName);
-                        string hashedPath = String.IsNullOrEmpty(directoryName) ? Utils.GetHash(@".\", HashAlgorithmName.MD5.Name) :
-                            Utils.GetHash(directoryName, HashAlgorithmName.MD5.Name);
+                        string hashedPath = String.IsNullOrEmpty(directoryName) ? Utils.GetHash(@".\", HashAlgorithmName.SHA256.Name).Substring(0, 32) :
+                            Utils.GetHash(directoryName, HashAlgorithmName.SHA256.Name).Substring(0, 32);
                         string extension = Path.GetExtension(archiveEntry.FullName);
 
                         // CAB files cannot be aliased since they're referred to from the Media table inside the MSI
                         string aliasFileName = String.Equals(extension.ToLowerInvariant(), ".cab") ? Path.GetFileName(archiveEntry.FullName) :
-                            Utils.GetHash(archiveEntry.FullName, HashAlgorithmName.MD5.Name) + Path.GetExtension(archiveEntry.FullName);
+                            Utils.GetHash(archiveEntry.FullName, HashAlgorithmName.SHA256.Name) + Path.GetExtension(archiveEntry.FullName);  // lgtm [cs/zipslip] Archive from trusted source
                         string aliasFullName = Path.Combine(tempPath, hashedPath, aliasFileName);
 
                         if (File.Exists(aliasFullName))
@@ -53,7 +53,7 @@ protected void VerifyContent(SignatureVerificationResult svr)
                         else
                         {
                             CreateDirectory(Path.GetDirectoryName(aliasFullName));
-                            archiveEntry.ExtractToFile(aliasFullName);
+                            archiveEntry.ExtractToFile(aliasFullName); // lgtm [cs/microsoft/zipslip] Archive from trusted source
                             archiveMap[archiveEntry.FullName] = aliasFullName;
                         }
                     }

src/SignCheck/Microsoft.SignCheck/Verification/Jar/JarSignatureFile.cs

@@ -209,7 +209,7 @@ private bool VerifySignatureDsa()
             byte[] signatureBlockBytes = JarUtils.ReadBytes(ArchivePath, SignatureBlockFilePath);
             byte[] signatureFileBytes = JarUtils.ReadBytes(ArchivePath, SignatureFilePath);
 
-            SHA1Managed sha = new SHA1Managed();
+            SHA1Managed sha = new SHA1Managed(); // lgtm [cs/weak-crypto] Algorithm required by specification
             byte[] hash = sha.ComputeHash(signatureFileBytes);
 
             ContentInfo ci = new ContentInfo(signatureFileBytes);

src/SignCheck/SignCheck/SignCheck.cs

@@ -462,6 +462,8 @@ private async Task DownloadFileAsync(Uri uri)
         {
             try
             {
+                ServicePointManager.CheckCertificateRevocationList = true;
+
                 using (var wc = new WebClient())
                 {
                     string downloadPath = Path.Combine(_appData, Path.GetFileName(uri.LocalPath));