* NEW: Add XML configuration option `IncludeWixBinaries` to include Wix binaries in the analysis. #944

shaopeng-gh · 2023-07-18T04:29:29Z

Added test file for Wix v4.
Test will only pass after my fix is merged: #941

This reverts commit 075af75.

…n Unix-like OS.

michaelcfanning · 2023-07-18T15:41:58Z

src/BinSkim.Sdk/BinaryAnalyzerContext.cs

@@ -79,6 +79,8 @@ public CompilerDataLogger CompilerDataLogger

        public bool IgnorePdbLoadError { get; set; }

+        public bool IncludeWixBinaries { get; set; }


IncludeWixBinaries

You didn't add a PerLanguageOption for this, as a result no one can configure this property via XML configuration. #Resolved

michaelcfanning · 2023-07-18T15:42:46Z

    public CompilerDataLogger CompilerDataLogger

this is a useless utilization of the context object because this runtime type doesn't serialize. whoever added this didn't understand the context XML system.

In reply to: 1640480352

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:69 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning · 2023-07-18T15:43:06Z

    public bool ComprehensiveBinaryParsing

Here is a correct example of property utilization.

In reply to: 1640480894

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:43 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning

🕐

michaelcfanning · 2023-07-18T16:12:50Z

    public string LocalSymbolDirectories { get; set; }

please make this a config-capable property.

In reply to: 1640531469

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:41 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning · 2023-07-18T16:13:07Z

    public bool TracePdbLoads { get; set; }

add XML property

In reply to: 1640531872

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:49 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning · 2023-07-18T16:13:18Z

    public string SymbolPath { get; set; }

Add XML property

In reply to: 1640532133

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:51 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning · 2023-07-18T16:13:36Z

src/BinSkim.Sdk/BinaryAnalyzerContext.cs

@@ -79,6 +79,8 @@ public CompilerDataLogger CompilerDataLogger

        public bool IgnorePdbLoadError { get; set; }


IgnorePdbLoadError

add xml property #Resolved

michaelcfanning · 2023-07-18T16:13:47Z

                ? this.Policy.GetProperty(SharedCompilerDataLoggerProperty)

remove xml property

In reply to: 1640532812

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:74 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning · 2023-07-18T16:14:00Z

src/BinSkim.Driver/AnalyzeOptions.cs

+        [Option(
+            "includeWixBinaries",
+            HelpText = "If enabled, BinSkim will include Wix binaries in the analysis.")]
+        public bool IncludeWixBinaries { get; set; }


IncludeWixBinaries

so what should we do with the next 10 file types of this kind? do we propose to add 10 more command-line arguments?

what's the audience for this change? since the production of wix binaries is owned by the wix toolset, isn't it true that this feature is mostly for that narrow audience?

what I'd suggest here is either we have a single --include argument that would accept an enumeration of binary kinds to fault in eventually. For now, however, with only a single case to address, we could simply proceed with a configuration XML solution. we can add the argument later if necessary. #Resolved

as last conversation in the sync meeting, remove the option and use the xml config

shaopeng-gh · 2023-07-19T07:26:01Z

    public bool ComprehensiveBinaryParsing

Using this format now, thanks!

In reply to: 1640480894

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:43 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

shaopeng-gh · 2023-07-19T07:34:47Z

    public string LocalSymbolDirectories { get; set; }

added

In reply to: 1640531469

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:41 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

shaopeng-gh · 2023-07-19T07:40:46Z

    public string SymbolPath { get; set; }

added

In reply to: 1640532133

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:51 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

shaopeng-gh · 2023-07-19T08:28:13Z

    public CompilerDataLogger CompilerDataLogger

removed

In reply to: 1640480352

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:69 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

shaopeng-gh · 2023-07-19T08:28:30Z

                ? this.Policy.GetProperty(SharedCompilerDataLoggerProperty)

removed

In reply to: 1640532812

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:74 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

shaopeng-gh · 2023-07-19T08:40:02Z

    public bool TracePdbLoads { get; set; }

fixed all others, this one is a bit different, it depends on the --trace, proposed to leave it another pr.

In reply to: 1640531872

Refers to: src/BinSkim.Sdk/BinaryAnalyzerContext.cs:49 in 60f6f7c. [](commit_id = 60f6f7c, deletion_comment = False)

michaelcfanning

shaopeng-gh added 6 commits July 16, 2023 21:46

Add --includeWixBinaries

ba7f662

Release History

c0a6623

Add test

41060c5

Windows only for Wix test

075af75

Revert "Windows only for Wix test"

46e61c8

This reverts commit 075af75.

Modified test for: // Notifications of WRN998.UnsupportedPlatform i…

36408b0

…n Unix-like OS.

shaopeng-gh requested review from suvamM and schlaman-ms July 18, 2023 04:29

shaopeng-gh requested review from michaelcfanning and marmegh as code owners July 18, 2023 04:29

shaopeng-gh added 2 commits July 17, 2023 21:34

fix HelpText

ba2af80

Update test

60f6f7c

michaelcfanning reviewed Jul 18, 2023

View reviewed changes

michaelcfanning requested changes Jul 18, 2023

View reviewed changes

michaelcfanning reviewed Jul 18, 2023

View reviewed changes

shaopeng-gh added 2 commits July 18, 2023 23:38

support config

f7063bc

remove option, update test

74a7ba8

shaopeng-gh added 3 commits July 19, 2023 00:51

resolve other comments

c3d9a8c

default string.Empty

a2f8595

Update Release History

bf31dc3

shaopeng-gh requested a review from michaelcfanning July 19, 2023 08:41

shaopeng-gh and others added 3 commits July 20, 2023 11:40

Merge branch 'main' into users/shaopeng-gh/wix

e7f6572

merge from main

d084db9

Merge branch 'main' into users/shaopeng-gh/wix

aaf4353

shaopeng-gh changed the title * NEW: Add --includeWixBinaries option to include Wix binaries in the analysis. * NEW: Add XML configuration option to include Wix binaries in the analysis. Jul 27, 2023

shaopeng-gh changed the title * NEW: Add XML configuration option to include Wix binaries in the analysis. * NEW: Add XML configuration option IncludeWixBinaries to include Wix binaries in the analysis. Jul 27, 2023

michaelcfanning approved these changes Jul 27, 2023

View reviewed changes

suvamM enabled auto-merge July 27, 2023 20:15

suvamM disabled auto-merge July 27, 2023 20:29

suvamM closed this Jul 27, 2023

suvamM reopened this Jul 27, 2023

suvamM merged commit 533b71a into main Jul 27, 2023
6 checks passed

suvamM deleted the users/shaopeng-gh/wix branch July 27, 2023 20:30

shaopeng-gh mentioned this pull request Aug 2, 2023

Release v4.2.0 #950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

* NEW: Add XML configuration option `IncludeWixBinaries` to include Wix binaries in the analysis. #944

* NEW: Add XML configuration option `IncludeWixBinaries` to include Wix binaries in the analysis. #944

shaopeng-gh commented Jul 18, 2023

michaelcfanning Jul 18, 2023 •

edited by shaopeng-gh

Loading

shaopeng-gh Jul 19, 2023

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning left a comment

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning Jul 18, 2023 •

edited by shaopeng-gh

Loading

shaopeng-gh Jul 19, 2023

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning Jul 18, 2023 •

edited by shaopeng-gh

Loading

shaopeng-gh Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

michaelcfanning left a comment

		@@ -79,6 +79,8 @@ public CompilerDataLogger CompilerDataLogger

		public bool IgnorePdbLoadError { get; set; }

		public bool IncludeWixBinaries { get; set; }

* NEW: Add XML configuration option IncludeWixBinaries to include Wix binaries in the analysis. #944

* NEW: Add XML configuration option IncludeWixBinaries to include Wix binaries in the analysis. #944

Conversation

shaopeng-gh commented Jul 18, 2023

michaelcfanning Jul 18, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Jul 19, 2023

Choose a reason for hiding this comment

michaelcfanning commented Jul 18, 2023 • edited by shaopeng-gh Loading

michaelcfanning commented Jul 18, 2023 • edited by shaopeng-gh Loading

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning commented Jul 18, 2023 • edited by shaopeng-gh Loading

michaelcfanning commented Jul 18, 2023 • edited by shaopeng-gh Loading

michaelcfanning commented Jul 18, 2023 • edited by shaopeng-gh Loading

michaelcfanning Jul 18, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Jul 19, 2023

Choose a reason for hiding this comment

michaelcfanning commented Jul 18, 2023 • edited by shaopeng-gh Loading

michaelcfanning Jul 18, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

shaopeng-gh Jul 19, 2023

Choose a reason for hiding this comment

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

shaopeng-gh commented Jul 19, 2023

michaelcfanning left a comment

Choose a reason for hiding this comment

* NEW: Add XML configuration option `IncludeWixBinaries` to include Wix binaries in the analysis. #944

* NEW: Add XML configuration option `IncludeWixBinaries` to include Wix binaries in the analysis. #944

michaelcfanning Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning commented Jul 18, 2023 •

edited by shaopeng-gh

Loading

michaelcfanning Jul 18, 2023 •

edited by shaopeng-gh

Loading