From c2d17d3a98ef43c9da47ab41ca9116acf774042a Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Thu, 30 Nov 2023 01:39:44 -0800 Subject: [PATCH 1/8] Fall back to DefaultTelemetryConnectionString --- src/BinSkim.Driver/AnalyzeOptions.cs | 5 + .../MultithreadedAnalyzeCommand.cs | 8 +- src/BinSkim.Sdk/BinSkim.Sdk.csproj | 9 ++ src/BinSkim.Sdk/BinaryAnalyzerContext.cs | 6 + .../EnvironmentResources.Designer.cs | 72 ++++++++++ src/BinSkim.Sdk/EnvironmentResources.resx | 123 ++++++++++++++++++ src/BinSkim.Sdk/Telemetry.cs | 7 + src/BinaryParsers/BinaryParsersProperties.cs | 5 + 8 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 src/BinSkim.Sdk/EnvironmentResources.Designer.cs create mode 100644 src/BinSkim.Sdk/EnvironmentResources.resx diff --git a/src/BinSkim.Driver/AnalyzeOptions.cs b/src/BinSkim.Driver/AnalyzeOptions.cs index c1e1a01d..f19b3f29 100644 --- a/src/BinSkim.Driver/AnalyzeOptions.cs +++ b/src/BinSkim.Driver/AnalyzeOptions.cs @@ -53,6 +53,11 @@ public class AnalyzeOptions : AnalyzeOptionsBase HelpText = "If enabled, BinSkim won't break if we have a 'PdbLoadingException'.")] public bool? IgnorePdbLoadError { get; set; } + [Option( + "disable-telemetry", + HelpText = "If enabled, BinSkim will disable telemetry.")] + public bool? DisableTelemetry { get; set; } + [Option( 's', "statistics", diff --git a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs index 3a685923..725bfeb9 100644 --- a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs +++ b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs @@ -70,6 +70,7 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze // Update context object based on command-line parameters. context.SymbolPath = options.SymbolsPath ?? context.SymbolPath; context.IgnorePdbLoadError = options.IgnorePdbLoadError != null ? options.IgnorePdbLoadError.Value : context.IgnorePdbLoadError; + context.DisableTelemetry = options.DisableTelemetry != null ? options.DisableTelemetry.Value : context.DisableTelemetry; context.LocalSymbolDirectories = options.LocalSymbolDirectories ?? context.LocalSymbolDirectories; context.TracePdbLoads = options.Trace.Contains(nameof(Traces.PdbLoad)); @@ -172,6 +173,11 @@ public override int Run(AnalyzeOptions analyzeOptions) { Stopwatch stopwatch = null; + if (analyzeOptions.DisableTelemetry == true) + { + this.Telemetry = null; + } + if (analyzeOptions.Trace.Where(s => s == nameof(DefaultTraces.ScanTime)).Any()) { stopwatch = Stopwatch.StartNew(); @@ -243,6 +249,6 @@ public override int Run(AnalyzeOptions analyzeOptions) internal Sarif.SarifVersion UnitTestOutputVersion { get; set; } - private Sdk.Telemetry Telemetry { get; } + private Sdk.Telemetry Telemetry { get; set; } } } diff --git a/src/BinSkim.Sdk/BinSkim.Sdk.csproj b/src/BinSkim.Sdk/BinSkim.Sdk.csproj index b593b9f3..e6e7f07f 100644 --- a/src/BinSkim.Sdk/BinSkim.Sdk.csproj +++ b/src/BinSkim.Sdk/BinSkim.Sdk.csproj @@ -20,6 +20,11 @@ + + True + True + EnvironmentResources.resx + True True @@ -28,6 +33,10 @@ + + ResXFileCodeGenerator + EnvironmentResources.Designer.cs + ResXFileCodeGenerator SdkResources.Designer.cs diff --git a/src/BinSkim.Sdk/BinaryAnalyzerContext.cs b/src/BinSkim.Sdk/BinaryAnalyzerContext.cs index 027af485..8a8daaf2 100644 --- a/src/BinSkim.Sdk/BinaryAnalyzerContext.cs +++ b/src/BinSkim.Sdk/BinaryAnalyzerContext.cs @@ -82,6 +82,12 @@ public bool IgnorePdbLoadError set => this.Policy.SetProperty(BinaryParsersProperties.IgnorePdbLoadError, value); } + public bool DisableTelemetry + { + get => this.Policy?.GetProperty(BinaryParsersProperties.DisableTelemetry) == true; + set => this.Policy.SetProperty(BinaryParsersProperties.DisableTelemetry, value); + } + public bool IncludeWixBinaries { get => this.Policy?.GetProperty(BinaryParsersProperties.IncludeWixBinaries) == true; diff --git a/src/BinSkim.Sdk/EnvironmentResources.Designer.cs b/src/BinSkim.Sdk/EnvironmentResources.Designer.cs new file mode 100644 index 00000000..cde750f6 --- /dev/null +++ b/src/BinSkim.Sdk/EnvironmentResources.Designer.cs @@ -0,0 +1,72 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by a tool. +// Runtime Version:4.0.30319.42000 +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + +namespace Microsoft.CodeAnalysis.IL.Sdk { + using System; + + + /// + /// A strongly-typed resource class, for looking up localized strings, etc. + /// + // This class was auto-generated by the StronglyTypedResourceBuilder + // 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", "17.0.0.0")] + [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] + [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] + internal class EnvironmentResources { + + private static global::System.Resources.ResourceManager resourceMan; + + private static global::System.Globalization.CultureInfo resourceCulture; + + [global::System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] + internal EnvironmentResources() { + } + + /// + /// Returns the cached ResourceManager instance used by this class. + /// + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] + internal static global::System.Resources.ResourceManager ResourceManager { + get { + if (object.ReferenceEquals(resourceMan, null)) { + global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("Microsoft.CodeAnalysis.IL.Sdk.EnvironmentResources", typeof(EnvironmentResources).Assembly); + resourceMan = temp; + } + return resourceMan; + } + } + + /// + /// Overrides the current thread's CurrentUICulture property for all + /// resource lookups using this strongly typed resource class. + /// + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] + internal static global::System.Globalization.CultureInfo Culture { + get { + return resourceCulture; + } + set { + resourceCulture = value; + } + } + + /// + /// Looks up a localized string similar to . + /// + internal static string DefaultTelemetryConnectionString { + get { + return ResourceManager.GetString("DefaultTelemetryConnectionString", resourceCulture); + } + } + } +} diff --git a/src/BinSkim.Sdk/EnvironmentResources.resx b/src/BinSkim.Sdk/EnvironmentResources.resx new file mode 100644 index 00000000..3f29582a --- /dev/null +++ b/src/BinSkim.Sdk/EnvironmentResources.resx @@ -0,0 +1,123 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + text/microsoft-resx + + + 2.0 + + + System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + + + \ No newline at end of file diff --git a/src/BinSkim.Sdk/Telemetry.cs b/src/BinSkim.Sdk/Telemetry.cs index 82e487a9..cdbd0ba2 100644 --- a/src/BinSkim.Sdk/Telemetry.cs +++ b/src/BinSkim.Sdk/Telemetry.cs @@ -124,6 +124,13 @@ private static void ConfigureTelemetryContext(TelemetryContext context) return "InstrumentationKey=" + appInsightsKey; } + // Fall back to DefaultTelemetryConnectionString. + string defaultTelemetryConnectionString = EnvironmentResources.DefaultTelemetryConnectionString; + if (!string.IsNullOrWhiteSpace(defaultTelemetryConnectionString)) + { + return defaultTelemetryConnectionString; + } + return null; } diff --git a/src/BinaryParsers/BinaryParsersProperties.cs b/src/BinaryParsers/BinaryParsersProperties.cs index 6afd2b43..e993b9a5 100644 --- a/src/BinaryParsers/BinaryParsersProperties.cs +++ b/src/BinaryParsers/BinaryParsersProperties.cs @@ -34,6 +34,11 @@ public IEnumerable GetOptions() "BinaryParsers", nameof(IgnorePdbLoadError), defaultValue: () => false, "Set this value to 'true' to don't break if we have a 'PdbLoadingException'."); + public static PerLanguageOption DisableTelemetry { get; } = + new PerLanguageOption( + "BinaryParsers", nameof(DisableTelemetry), defaultValue: () => false, + "Set this value to 'true' to disable telemetry."); + public static PerLanguageOption IncludeWixBinaries { get; } = new PerLanguageOption( "BinaryParsers", nameof(IncludeWixBinaries), defaultValue: () => false, From 2effd967054aca3af8d2c37b1a01179af62cda81 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Thu, 30 Nov 2023 20:40:28 -0800 Subject: [PATCH 2/8] Release History --- ReleaseHistory.md | 1 + .../CommandLineTests.cs | 37 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/ReleaseHistory.md b/ReleaseHistory.md index e48c5494..191d104d 100644 --- a/ReleaseHistory.md +++ b/ReleaseHistory.md @@ -16,6 +16,7 @@ - NEW => new feature ## UNRELEASED +* NEW: Add `--disable-telemetry` argument to disable telemetry collection. ## **v4.2.1** * FPS: `BA2004.EnableSecureSourceCodeHashing` now will no longer generate false positives on precompiled headers, they are always without hash. [#965](https://github.com/microsoft/binskim/pull/965) diff --git a/src/Test.FunctionalTests.BinSkim.Driver/CommandLineTests.cs b/src/Test.FunctionalTests.BinSkim.Driver/CommandLineTests.cs index c81084ea..370dbdd2 100644 --- a/src/Test.FunctionalTests.BinSkim.Driver/CommandLineTests.cs +++ b/src/Test.FunctionalTests.BinSkim.Driver/CommandLineTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System; using System.Collections.Generic; using System.Linq; using System.Text; @@ -10,7 +11,6 @@ using FluentAssertions; using Microsoft.CodeAnalysis.IL; -using Microsoft.CodeAnalysis.Sarif.Driver; using Xunit; @@ -70,5 +70,40 @@ public void MostlyFunctionlessCommandlineTest() builder.Length.Should().Be(0, $"all test cases should pass, but the following test cases failed:\n{builder}"); } + + [Fact] + public void DisableTelemetryCommandlineTest() + { + var testCases = new List>() + { + new Tuple(@"analyze C:\Native.exe -o C:\result.sarif", null), + new Tuple(@"analyze C:\Native.exe -o C:\result.sarif --disable-telemetry true", true), + new Tuple(@"analyze C:\Native.exe -o C:\result.sarif --disable-telemetry false", false) + }; + + var builder = new StringBuilder(); + + foreach (Tuple testCase in testCases) + { + string[] args = testCase.Item1.Split(' '); + bool parser = new Parser(cfg => cfg.CaseInsensitiveEnumValues = true).ParseArguments(args) + .MapResult( + options => + { + if (options.DisableTelemetry != testCase.Item2) + { + builder.AppendLine($"\u2022 {testCase.Item1}"); + } + return true; + }, + err => + { + builder.AppendLine($"\u2022 {testCase.Item1}"); + return true; + }); + } + builder.Length.Should().Be(0, + $"all test cases should pass, but the following test cases failed:\n{builder}"); + } } } From 9ca92520b5b9f44c647cacab521dcdd153cdda5e Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Thu, 30 Nov 2023 20:49:29 -0800 Subject: [PATCH 3/8] Update usage --- src/BinaryParsers/BinaryParsersProperties.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/BinaryParsers/BinaryParsersProperties.cs b/src/BinaryParsers/BinaryParsersProperties.cs index e993b9a5..d45b1469 100644 --- a/src/BinaryParsers/BinaryParsersProperties.cs +++ b/src/BinaryParsers/BinaryParsersProperties.cs @@ -17,6 +17,7 @@ public IEnumerable GetOptions() { ComprehensiveBinaryParsing, IgnorePdbLoadError, + DisableTelemetry, IncludeWixBinaries, LocalSymbolDirectories, SymbolPath From 98e4fe7a2e5d705b74ba32685f5a632d24c62160 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Fri, 1 Dec 2023 14:48:57 -0800 Subject: [PATCH 4/8] Add test --- .../TelemetryTests.cs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/Test.UnitTests.BinSkim.Driver/TelemetryTests.cs b/src/Test.UnitTests.BinSkim.Driver/TelemetryTests.cs index 456730f9..c8ee9ce0 100644 --- a/src/Test.UnitTests.BinSkim.Driver/TelemetryTests.cs +++ b/src/Test.UnitTests.BinSkim.Driver/TelemetryTests.cs @@ -2,10 +2,16 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.IO; using FluentAssertions; +using FluentAssertions.Execution; using Microsoft.ApplicationInsights.Extensibility; +using Microsoft.CodeAnalysis.BinaryParsers; +using Microsoft.CodeAnalysis.IL; +using Microsoft.CodeAnalysis.IL.Rules; +using Microsoft.CodeAnalysis.Sarif; using Xunit; @@ -71,5 +77,47 @@ public void Telemetry_ShouldDisposeTelemetryConfigurationInDispose() telemetryConfiguration.TelemetryChannel.Should().BeNull(); } + + [Fact] + public void Telemetry_ShouldHaveConsistentResultsEnabledOrDisabled() + { + if (!PlatformSpecificHelpers.RunningOnWindows()) { return; } + + WindowsBinaryAndPdbSkimmerBase.s_PdbExceptions.Clear(); + string fileName = Path.Combine(Path.GetTempPath(), "AnalyzeCommand_TelemetryEnableDisableTest.sarif"); + string pathToTestFile = Path.Combine(PEBinaryTests.TestData, "PE", "Native_x64_VS2019_CPlusPlus_DEBUG_DEFAULT.dll"); + var options = new AnalyzeOptions + { + TargetFileSpecifiers = new string[] { + pathToTestFile + }, + Recurse = true, + OutputFilePath = fileName, + OutputFileOptions = new[] { FilePersistenceOptions.ForceOverwrite }, + IgnorePdbLoadError = false, + DataToInsert = new[] { OptionallyEmittedData.Hashes } + }; + + var command = new MultithreadedAnalyzeCommand(); + options.DisableTelemetry = null; + command.Run(options); + var logWithDisableTelemetryNull = SarifLog.Load(fileName); + + options.DisableTelemetry = true; + command.Run(options); + var logWithDisableTelemetryTrue = SarifLog.Load(fileName); + + options.DisableTelemetry = false; + command.Run(options); + var logWithDisableTelemetryFalse = SarifLog.Load(fileName); + + using (new AssertionScope()) + { + logWithDisableTelemetryTrue.Runs[0].Results.Should().BeEquivalentTo(logWithDisableTelemetryFalse.Runs[0].Results, + "Whether DisableTelemetry is true or false, the results should be the same."); + logWithDisableTelemetryFalse.Runs[0].Results.Should().BeEquivalentTo(logWithDisableTelemetryNull.Runs[0].Results, + "Whether DisableTelemetry is explicitly set to false or not, the results should be the same."); + } + } } } From bf8372bf3817605d364475ca98357ee4cebe5141 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Tue, 5 Dec 2023 00:28:49 -0800 Subject: [PATCH 5/8] update sub module --- src/sarif-sdk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sarif-sdk b/src/sarif-sdk index bc8cb578..5e2bc5ff 160000 --- a/src/sarif-sdk +++ b/src/sarif-sdk @@ -1 +1 @@ -Subproject commit bc8cb578c3d020f927f0db9118ecab46f4bb257b +Subproject commit 5e2bc5ff4a8a39e862937e4f2f085f8cfbfa9b39 From cdeb185df2187ee38d8e8d9db05c5e1ae1b7fe98 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Tue, 5 Dec 2023 01:40:38 -0800 Subject: [PATCH 6/8] Revert "update sub module" This reverts commit bf8372bf3817605d364475ca98357ee4cebe5141. --- src/sarif-sdk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sarif-sdk b/src/sarif-sdk index 5e2bc5ff..bc8cb578 160000 --- a/src/sarif-sdk +++ b/src/sarif-sdk @@ -1 +1 @@ -Subproject commit 5e2bc5ff4a8a39e862937e4f2f085f8cfbfa9b39 +Subproject commit bc8cb578c3d020f927f0db9118ecab46f4bb257b From 867240c7771ce51345418c071a5912a8e6d3a685 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Tue, 5 Dec 2023 01:41:11 -0800 Subject: [PATCH 7/8] revert update submodule --- src/sarif-sdk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sarif-sdk b/src/sarif-sdk index bc8cb578..5e2bc5ff 160000 --- a/src/sarif-sdk +++ b/src/sarif-sdk @@ -1 +1 @@ -Subproject commit bc8cb578c3d020f927f0db9118ecab46f4bb257b +Subproject commit 5e2bc5ff4a8a39e862937e4f2f085f8cfbfa9b39 From bbd1dd20d1ef3bbcc68df573ed1d898b363a6c87 Mon Sep 17 00:00:00 2001 From: Shaopeng Li Date: Tue, 5 Dec 2023 01:44:02 -0800 Subject: [PATCH 8/8] Revert "revert update submodule" This reverts commit 867240c7771ce51345418c071a5912a8e6d3a685. --- src/sarif-sdk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sarif-sdk b/src/sarif-sdk index 5e2bc5ff..bc8cb578 160000 --- a/src/sarif-sdk +++ b/src/sarif-sdk @@ -1 +1 @@ -Subproject commit 5e2bc5ff4a8a39e862937e4f2f085f8cfbfa9b39 +Subproject commit bc8cb578c3d020f927f0db9118ecab46f4bb257b