diff --git a/src/BinSkim.Rules/BinarySkimmer.cs b/src/BinSkim.Rules/BinarySkimmer.cs index 84ca15b72..d918845c9 100644 --- a/src/BinSkim.Rules/BinarySkimmer.cs +++ b/src/BinSkim.Rules/BinarySkimmer.cs @@ -18,7 +18,7 @@ public abstract class BinarySkimmer : Skimmer // https://github.com/Microsoft/binskim/issues/192 public override MultiformatMessageString Help => this.FullDescription; - public BinarySkimmer() + protected BinarySkimmer() { // Set Binscope friendly name for backwards compatibility, if one exists. string altId = BinScopeCompatibility.GetBinScopeRuleReadableName(this.Id); diff --git a/src/BinSkim.Rules/PERules/BA2007.EnableCriticalCompilerWarnings.cs b/src/BinSkim.Rules/PERules/BA2007.EnableCriticalCompilerWarnings.cs index e7e9a9a3c..68a29fa77 100644 --- a/src/BinSkim.Rules/PERules/BA2007.EnableCriticalCompilerWarnings.cs +++ b/src/BinSkim.Rules/PERules/BA2007.EnableCriticalCompilerWarnings.cs @@ -141,7 +141,7 @@ public override void AnalyzePortableExecutableAndPdb(BinaryAnalyzerContext conte if (warningLevel < 3) { - exampleTooLowWarningCommandLine = exampleTooLowWarningCommandLine ?? omDetails.RawCommandLine; + exampleTooLowWarningCommandLine ??= omDetails.RawCommandLine; string msg = "[warning level: " + warningLevel.ToString(CultureInfo.InvariantCulture) + "]"; warningTooLowModules.Add(om.CreateCompilandRecordWithSuffix(msg)); @@ -151,7 +151,7 @@ public override void AnalyzePortableExecutableAndPdb(BinaryAnalyzerContext conte if (requiredDisabledWarnings.Count != 0) { MergeInto(overallDisabledWarnings, requiredDisabledWarnings); - exampleDisabledWarningCommandLine = exampleDisabledWarningCommandLine ?? omDetails.RawCommandLine; + exampleDisabledWarningCommandLine ??= omDetails.RawCommandLine; string msg = "[Explicitly disabled warnings: " + CreateTextWarningList(requiredDisabledWarnings) + "]"; disabledWarningModules.Add(om.CreateCompilandRecordWithSuffix(msg)); diff --git a/src/BinSkim.Rules/PERules/PEBinarySkimmerBase.cs b/src/BinSkim.Rules/PERules/PEBinarySkimmerBase.cs index 0cc78ec02..6fb14a4d9 100644 --- a/src/BinSkim.Rules/PERules/PEBinarySkimmerBase.cs +++ b/src/BinSkim.Rules/PERules/PEBinarySkimmerBase.cs @@ -13,7 +13,7 @@ public override AnalysisApplicability CanAnalyze(BinaryAnalyzerContext context, if (context.IsPE()) { PEBinary target = context.PEBinary(); - return target.PE != null && target.PE.IsPEFile + return target.PE?.IsPEFile == true ? this.CanAnalyzePE(target, context.Policy, out reasonForNotAnalyzing) : AnalysisApplicability.NotApplicableToSpecifiedTarget; } diff --git a/src/BinSkim.Rules/PERules/WindowsBinaryAndPdbSkimmerBase.cs b/src/BinSkim.Rules/PERules/WindowsBinaryAndPdbSkimmerBase.cs index f82c83c9d..c71a10073 100644 --- a/src/BinSkim.Rules/PERules/WindowsBinaryAndPdbSkimmerBase.cs +++ b/src/BinSkim.Rules/PERules/WindowsBinaryAndPdbSkimmerBase.cs @@ -2,6 +2,8 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.Collections.Concurrent; +using System.Collections.Generic; using System.Diagnostics; using Microsoft.CodeAnalysis.BinaryParsers; @@ -26,6 +28,8 @@ public abstract class WindowsBinaryAndPdbSkimmerBase : WindowsBinarySkimmerBase public virtual bool LogPdbLoadException => true; + public static readonly ConcurrentDictionary s_PdbExceptions = new ConcurrentDictionary(); + public sealed override void Analyze(BinaryAnalyzerContext context) { // Uses PDB Parsing. @@ -130,20 +134,27 @@ public static void LogExceptionLoadingPdb(IAnalysisContext context, PdbException throw new ArgumentNullException(nameof(context)); } - // '{0}' was not evaluated for check '{1}' because its PDB could not be loaded ({2}). + string path = context.TargetUri.OriginalString; + string key = $"{path}@{pdbException.ExceptionDisplayMessage}"; + if (s_PdbExceptions.ContainsKey(key)) + { + return; + } + + // '{0}' was not evaluated because its PDB could not be loaded ({1}). context.Logger.LogConfigurationNotification( Errors.CreateNotification( context.TargetUri, "ERR997.ExceptionLoadingPdb", - context.Rule.Id, + string.Empty, FailureLevel.Error, pdbException, persistExceptionStack: false, - RuleResources.ERR997_ExceptionLoadingPdb, + RuleResources.ERR997_ExceptionLoadingPdbGeneric, context.TargetUri.GetFileName(), - context.Rule.Name, pdbException.ExceptionDisplayMessage)); + s_PdbExceptions.TryAdd(key, true); context.RuntimeErrors |= RuntimeConditions.ExceptionLoadingPdb; if (!string.IsNullOrEmpty(pdbException.LoadTrace)) diff --git a/src/BinSkim.Rules/RuleResources.Designer.cs b/src/BinSkim.Rules/RuleResources.Designer.cs index bba88287e..b8bb8a304 100644 --- a/src/BinSkim.Rules/RuleResources.Designer.cs +++ b/src/BinSkim.Rules/RuleResources.Designer.cs @@ -19,7 +19,7 @@ namespace Microsoft.CodeAnalysis.IL.Rules { // class via a tool like ResGen or Visual Studio. // To add or remove a member, edit your .ResX file then rerun ResGen // with the /str option, or rebuild your VS project. - [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "16.0.0.0")] + [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "17.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] internal class RuleResources { @@ -1347,6 +1347,15 @@ internal static string ERR997_ExceptionLoadingPdb { } } + /// + /// Looks up a localized string similar to '{0}' was not evaluated because its PDB could not be loaded ({1}).. + /// + internal static string ERR997_ExceptionLoadingPdbGeneric { + get { + return ResourceManager.GetString("ERR997_ExceptionLoadingPdbGeneric", resourceCulture); + } + } + /// /// Looks up a localized string similar to '{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}.. /// diff --git a/src/BinSkim.Rules/RuleResources.resx b/src/BinSkim.Rules/RuleResources.resx index 304316668..f64a09db9 100644 --- a/src/BinSkim.Rules/RuleResources.resx +++ b/src/BinSkim.Rules/RuleResources.resx @@ -569,4 +569,7 @@ Modules did not meet the criteria: {1} Executable stack is not allowed on executable '{0}'. + + '{0}' was not evaluated because its PDB could not be loaded ({1}). + \ No newline at end of file diff --git a/src/BinSkim.Sdk/BinaryAnalyzerContext.cs b/src/BinSkim.Sdk/BinaryAnalyzerContext.cs index a82e6877c..bd5d911a3 100644 --- a/src/BinSkim.Sdk/BinaryAnalyzerContext.cs +++ b/src/BinSkim.Sdk/BinaryAnalyzerContext.cs @@ -17,8 +17,7 @@ public IBinary Binary { get { - this.iBinary = this.iBinary - ?? BinaryTargetManager.GetBinaryFromFile( + this.iBinary ??= BinaryTargetManager.GetBinaryFromFile( this.uri, this.SymbolPath, this.LocalSymbolDirectories, @@ -31,13 +30,13 @@ public IBinary Binary public Exception TargetLoadException { - get => this.Binary != null ? this.Binary.LoadException : null; + get => this.Binary?.LoadException; set => throw new InvalidOperationException(); } public bool IsValidAnalysisTarget { - get => this.Binary != null && this.Binary.Valid; + get => this.Binary?.Valid == true; set => throw new InvalidOperationException(); } diff --git a/src/BinSkim.sln b/src/BinSkim.sln index 5a7cc8907..96e190ee4 100644 --- a/src/BinSkim.sln +++ b/src/BinSkim.sln @@ -1,6 +1,6 @@ Microsoft Visual Studio Solution File, Format Version 12.00 # Visual Studio Version 17 -VisualStudioVersion = 17.0.31616.459 +VisualStudioVersion = 17.0.31622.56 MinimumVisualStudioVersion = 10.0.40219.1 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "BinSkim.Driver", "BinSkim.Driver\BinSkim.Driver.csproj", "{7B4552DC-901C-4569-BFBD-9EA129D53288}" ProjectSection(ProjectDependencies) = postProject diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md index 5d03609ba..3000b677d 100644 --- a/src/ReleaseHistory.md +++ b/src/ReleaseHistory.md @@ -1,8 +1,9 @@ # BinSkim Release History -## Unreeleased +## Unreleased * BUGFIX: Fix exception handling when PDB cannot be loaded by `IDiaDataSource`. [#461](https://github.com/microsoft/binskim/pull/461) +* BREAKING: PDB exceptions will be reported once per target. [#465](https://github.com/microsoft/binskim/pull/465) ## **v1.9.0-prerelease1** [NuGet Package](https://www.nuget.org/packages/Microsoft.CodeAnalysis.BinSkim/1.9.0-prerelease1) diff --git a/src/Test.FunctionalTests.BinSkim.Rules/RuleTests.cs b/src/Test.FunctionalTests.BinSkim.Rules/RuleTests.cs index 09b72ecd5..e8a116af4 100644 --- a/src/Test.FunctionalTests.BinSkim.Rules/RuleTests.cs +++ b/src/Test.FunctionalTests.BinSkim.Rules/RuleTests.cs @@ -276,8 +276,7 @@ private void VerifyApplicability( context.Rule = skimmer; - AnalysisApplicability applicability; - applicability = skimmer.CanAnalyze(context, out string reasonForNotAnalyzing); + AnalysisApplicability applicability = skimmer.CanAnalyze(context, out string reasonForNotAnalyzing); if (applicability != expectedApplicability) { sb.AppendLine("CanAnalyze did not correct indicate target applicability (unexpected return was " + @@ -297,8 +296,7 @@ private void VerifyApplicability( private HashSet GetTestFilesMatchingConditions(HashSet metadataConditions) { - string testFilesDirectory; - testFilesDirectory = Path.Combine(Environment.CurrentDirectory, "BaselineTestsData"); + string testFilesDirectory = Path.Combine(Environment.CurrentDirectory, "BaselineTestsData"); Assert.True(Directory.Exists(testFilesDirectory)); var result = new HashSet(StringComparer.OrdinalIgnoreCase); @@ -458,10 +456,8 @@ public void BA2002_DoNotIncorporateVulnerableDependencies_Fail() { if (BinaryParsers.PlatformSpecificHelpers.RunningOnWindows()) { - var failureConditions = new HashSet { MetadataConditions.CouldNotLoadPdb }; this.VerifyFail( new DoNotIncorporateVulnerableDependencies(), - this.GetTestFilesMatchingConditions(failureConditions), useDefaultPolicy: true); } else @@ -564,7 +560,7 @@ public void BA2005_DoNotShipVulnerableBinaries_NotApplicable() this.VerifyApplicability(new DoNotShipVulnerableBinaries(), notApplicableTo, AnalysisApplicability.ApplicableToSpecifiedTarget); } - [Fact(Skip = "commenting to release.")] + [Fact] public void BA2006_BuildWithSecureTools_Pass() { if (BinaryParsers.PlatformSpecificHelpers.RunningOnWindows()) @@ -577,15 +573,13 @@ public void BA2006_BuildWithSecureTools_Pass() } } - [Fact(Skip = "commenting to release.")] + [Fact] public void BA2006_BuildWithSecureTools_Fail() { if (BinaryParsers.PlatformSpecificHelpers.RunningOnWindows()) { - var failureConditions = new HashSet { MetadataConditions.CouldNotLoadPdb }; this.VerifyFail( new BuildWithSecureTools(), - this.GetTestFilesMatchingConditions(failureConditions), useDefaultPolicy: true); } else @@ -625,14 +619,8 @@ public void BA2007_EnableCriticalCompilerWarnings_Fail() { if (BinaryParsers.PlatformSpecificHelpers.RunningOnWindows()) { - var failureConditions = new HashSet - { - MetadataConditions.CouldNotLoadPdb - }; - this.VerifyFail( new EnableCriticalCompilerWarnings(), - this.GetTestFilesMatchingConditions(failureConditions), useDefaultPolicy: true); } else @@ -841,16 +829,14 @@ public void BA2013_InitializeStackProtection_Fail() { if (BinaryParsers.PlatformSpecificHelpers.RunningOnWindows()) { - var failureConditions = new HashSet - { - MetadataConditions.CouldNotLoadPdb - }; - this.VerifyFail( new InitializeStackProtection(), - this.GetTestFilesMatchingConditions(failureConditions), useDefaultPolicy: true); } + else + { + this.VerifyThrows(new DoNotShipVulnerableBinaries(), useDefaultPolicy: true); + } } [Fact]