Fixing `NullReferenceException` when `--Hashes` and telemetry rules are enabled #531

eddynaka · 2021-12-16T01:13:02Z

With the single thread analysis, the argument --Hashes was translated to --insert Hashes, adding the deprecated message.
When we moved to the multithreaded analysis, without that condition we would not generate the artifacts.
Since the summary was expecting the artifacts, it was accessing the number of elements directly.

eddynaka · 2021-12-16T01:17:37Z

src/BinSkim.Driver/AnalyzeCommand.cs

-                EndTimeUtc = invocation.EndTimeUtc,
-                TimeConsumed = invocation.EndTimeUtc - invocation.StartTimeUtc,
-            };
-        }


this code was moved to the AnalysisSummaryExtractor in previous PRs. #ByDesign

eddynaka · 2021-12-16T01:19:49Z

src/BinSkim.Driver/AnalysisSummaryExtractor.cs

@@ -40,7 +40,7 @@ public static AnalysisSummary ExtractAnalysisSummary(SarifLog sarifLog, AnalyzeO
                ToolVersion = tool.Driver.Version,
                NormalizedPath = string.Join(";", options.TargetFileSpecifiers.Select(p => System.IO.Path.GetDirectoryName(p)).Distinct()),
                SymbolPath = options.SymbolsPath,
-                FileAnalyzed = artifacts.Count,
+                FileAnalyzed = artifacts?.Count ?? 0,


artifacts?.Count ?? 0

this will prevent the exception in case the SARIF does not have artifacts.

This would happen if: you don't have the --insert hashes or --Hashes option enabled but you have the telemetry rules. #ByDesign

eddynaka · 2021-12-16T01:20:47Z

src/BinSkim.Driver/AnalyzeCommand.cs

@@ -85,6 +85,17 @@ public override int Run(AnalyzeOptions analyzeOptions)
                analyzeOptions.Kind = new List<ResultKind> { ResultKind.Fail, ResultKind.NotApplicable, ResultKind.Pass };
            }

+#pragma warning disable CS0618 // Type or member is obsolete
+            if (analyzeOptions.ComputeFileHashes)


analyzeOptions.ComputeFileHashes

this change is needed to prevent a breaking change in the guardian tasks.
We need to support --Hashes #WontFix

maybe we should make this change in SDK instead?
AnalyzeCommandBase has this check,
MultithreadedAnalyzeCommandBase does not,

if the change is made in Binskim then only Binskim is fixed but not SDK

I was wondering about that.
Since we want to deprecate --hashes, i would move that logic to AnalyzeCommand and remove the reference from AnalyzeCommandBase.

With that, in the futhre, the options would not have --hashes by default and binskim itself would implement it.

eddynaka · 2021-12-17T15:46:37Z

src/BinSkim.Driver/AnalyzeOptions.cs

+        [Option(
+            "binskimAppInsightsKey",
+            HelpText = "An ApplicationInsightsKey to which will be send data. If not available, it will read from the environment variable.")]
+        public string BinskimAppInsightsKey { get; set; }


BinskimAppInsightsKey

To facilitate the guardian integration, we will expose a new parameter and also be able to read the environment variable.

The reason: from azure pipelines, creating the variable it isn't passing the value correctly (the variable is always unavailable). #Closed

We can't accept a change that encourages users to explicitly provide sensitive data on the command-line, this is a very poor security practice, as command-lines are persisted/logged in many contexts. In general, you do not want to compromise your own project quality/security to workaround a bug in another system. If there is a problem with ADO environment variable handling, let's get that reported and see what we can do to resolve. ADO also provides other mechanism for sharing data, including ingestion data from secured network shares (that we could lock down to a build agent account, perhaps). Let's follow up with some folks with relevant expertise offline.

thanks for the feedback.
i will remove the change and keep it clean to fix the issue that i faced.

Yes, and this is a very good point, thanks for making it. This isn't the kind of change that we'd want to fold into another PR, it would warrant its own issue. You can follow up with me and Ryley offline about your ADO issue.

Just updated and sent an email with the details.
thanks for the help!

marmegh

michaelcfanning · 2022-01-06T23:07:39Z

src/BinSkim.Driver/AnalysisSummaryExtractor.cs

@@ -40,7 +40,7 @@ public static AnalysisSummary ExtractAnalysisSummary(SarifLog sarifLog, AnalyzeO
                ToolVersion = tool.Driver.Version,
                NormalizedPath = string.Join(";", options.TargetFileSpecifiers.Select(p => System.IO.Path.GetDirectoryName(p)).Distinct()),
                SymbolPath = options.SymbolsPath,
-                FileAnalyzed = artifacts.Count,
+                FileAnalyzed = artifacts?.Count ?? 0,
                // FileNotAnalyzed =


// FileNotAnalyzed =

can you remove this please? it isn't helpful to have it here.

please add a TODO, or just open an issue #Closed

created a work item for that.

michaelcfanning

Fixing NullReferenceException when --Hashes is enabled

07ef687

eddynaka changed the title ~~Fixing NullReferenceException when --Hashes is enabled~~ Fixing NullReferenceException when --Hashes and telemetry rules are enabled Dec 16, 2021

Updating release history

15171be

eddynaka commented Dec 16, 2021

View reviewed changes

eddynaka marked this pull request as ready for review December 16, 2021 01:20

eddynaka requested a review from michaelcfanning as a code owner December 16, 2021 01:20

eddynaka added 3 commits December 17, 2021 11:56

Trying to read envvariable directly

72443c9

Adding new parameter

5f8e580

Adding new parameter to facilitate integration

0903e78

eddynaka commented Dec 17, 2021

View reviewed changes

Reverting for now.

374f95f

marmegh approved these changes Jan 6, 2022

View reviewed changes

michaelcfanning reviewed Jan 6, 2022

View reviewed changes

michaelcfanning approved these changes Jan 6, 2022

View reviewed changes

eddynaka added 4 commits January 6, 2022 16:06

Merge branch 'main' into users/ednakamu/fixing-nullreference-exception

bfc24c1

Removing unused comment

96a483a

Merge branch 'main' into users/ednakamu/fixing-nullreference-exception

26a5ca3

Addressing PR feedback

5f65569

eddynaka enabled auto-merge (squash) January 7, 2022 01:26

eddynaka merged commit 006f96b into main Jan 7, 2022

eddynaka deleted the users/ednakamu/fixing-nullreference-exception branch January 7, 2022 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing `NullReferenceException` when `--Hashes` and telemetry rules are enabled #531

Fixing `NullReferenceException` when `--Hashes` and telemetry rules are enabled #531

eddynaka commented Dec 16, 2021

eddynaka Dec 16, 2021 •

edited

Loading

eddynaka Dec 16, 2021 •

edited

Loading

eddynaka Dec 16, 2021 •

edited

Loading

shaopeng-gh Jan 3, 2022

eddynaka Jan 3, 2022

eddynaka Dec 17, 2021 •

edited

Loading

michaelcfanning Dec 17, 2021 •

edited

Loading

eddynaka Dec 17, 2021

michaelcfanning Dec 17, 2021

eddynaka Dec 17, 2021

marmegh left a comment

michaelcfanning Jan 6, 2022 •

edited by eddynaka

Loading

eddynaka Jan 7, 2022

michaelcfanning left a comment

Fixing NullReferenceException when --Hashes and telemetry rules are enabled #531

Fixing NullReferenceException when --Hashes and telemetry rules are enabled #531

Conversation

eddynaka commented Dec 16, 2021

eddynaka Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

eddynaka Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

eddynaka Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh Jan 3, 2022

Choose a reason for hiding this comment

eddynaka Jan 3, 2022

Choose a reason for hiding this comment

eddynaka Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

michaelcfanning Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

eddynaka Dec 17, 2021

Choose a reason for hiding this comment

michaelcfanning Dec 17, 2021

Choose a reason for hiding this comment

eddynaka Dec 17, 2021

Choose a reason for hiding this comment

marmegh left a comment

Choose a reason for hiding this comment

michaelcfanning Jan 6, 2022 • edited by eddynaka Loading

Choose a reason for hiding this comment

eddynaka Jan 7, 2022

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

Fixing `NullReferenceException` when `--Hashes` and telemetry rules are enabled #531

Fixing `NullReferenceException` when `--Hashes` and telemetry rules are enabled #531

eddynaka Dec 16, 2021 •

edited

Loading

eddynaka Dec 16, 2021 •

edited

Loading

eddynaka Dec 16, 2021 •

edited

Loading

eddynaka Dec 17, 2021 •

edited

Loading

michaelcfanning Dec 17, 2021 •

edited

Loading

michaelcfanning Jan 6, 2022 •

edited by eddynaka

Loading