Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude ARM Binaries from BA2025 analysis #650

Merged
merged 8 commits into from
Jun 13, 2022
Merged

Exclude ARM Binaries from BA2025 analysis #650

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2412b46
Exclude ARM Binaries from BA2025 analysis
marmegh Jun 1, 2022
bbc85b1
Cleanup ResultsTests.cs
marmegh Jun 1, 2022
f26ef21
Update ReleaseHistory.md
marmegh Jun 1, 2022
bfd17a3
Remove stray whitespace
marmegh Jun 1, 2022
07444d1
Cleanup validation method
marmegh Jun 6, 2022
dd45441
Added cross compile instructions for generating ARM and ARM64 test bi…
marmegh Jun 10, 2022
075281f
Removed blank line
marmegh Jun 10, 2022
dae55f5
Updated release history per PR feedback.
marmegh Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/BinSkim.Rules/PERules/BA2025.EnableShadowStack.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ public override AnalysisApplicability CanAnalyzePE(PEBinary target, Sarif.Proper
return notApplicable;
}

if (portableExecutable.Machine == Machine.Arm || portableExecutable.Machine == Machine.ArmThumb2)
{
reasonForNotAnalyzing = MetadataConditions.ImageIsArmBinary;
return notApplicable;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only logic update for this change. Specifically, excluding ARM binaries from BA2025 analysis in a similar fashion as ARM64 binaries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice code comment! Absolutely the info I was looking for. :)


reasonForNotAnalyzing = null;
return AnalysisApplicability.ApplicableToSpecifiedTarget;
}
Expand Down
1 change: 1 addition & 0 deletions src/BinSkim.Sdk/MetadataConditions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public static class MetadataConditions
public static readonly string ImageIsNotExe = SdkResources.MetadataCondition_ImageIsNotExe;
public static readonly string ImageIsNotMachO = SdkResources.MetadataCondition_ImageIsNotMachO;
public static readonly string CouldNotLoadPdb = SdkResources.MetadataCondition_CouldNotLoadPdb;
public static readonly string ImageIsArmBinary = SdkResources.MetadataCondition_ImageIsArmBinary;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static readonly string ImageIsArmBinary = SdkResources.MetadataCondition_ImageIsArmBinary;

Add a MetadataConditions entry for ARMBinaries. This is separate as changing the previous user facing string could confuse customers by changing details in the results.

public static readonly string ImageIsDebugOnly = SdkResources.MetadataCondition_ImageIsDebugOnly;
public static readonly string ImageIsNotSigned = SdkResources.MetadataCondition_ImageIsNotSigned;
public static readonly string ImageIsWixBinary = SdkResources.MetadataCondition_ImageIsWixBinary;
Expand Down
11 changes: 10 additions & 1 deletion src/BinSkim.Sdk/SdkResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/BinSkim.Sdk/SdkResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,4 +243,7 @@
<data name="MetadataCondition_ImageIsArm64BitBinary" xml:space="preserve">
<value>image is an ARM64 binary</value>
</data>
<data name="MetadataCondition_ImageIsArmBinary" xml:space="preserve">
<value>image is an ARM binary</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Bump ELFSharp from 2.14.0 to 2.15.0. [#631](https://github.com/microsoft/binskim/pull/631)
* FEATURE: Enable BinSkim for MacOS. [#576](https://github.com/microsoft/binskim/pull/576)
* Bump Sarif.Sdk by updating submodule from [4e9f606 to fc9a9df](https://github.com/microsoft/sarif-sdk/compare/4e9f606bb0e88428866e253352cdc70dc68f98cb...fc9a9dfb865096b5aaa9fa3651854670940f7459). [#638](https://github.com/microsoft/binskim/pull/638)
* BUGFIX: Fix `BA2025.EnableShadowStack` warning for ARM Binaries which cannot use `/CETCOMPAT`. [#](https://github.com/microsoft/binskim/pull/)

## **v1.9.4** [NuGet Package](https://www.nuget.org/packages/Microsoft.CodeAnalysis.BinSkim/1.9.4)

Expand Down
Binary file added ...ionalTestData/BA2025.EnableShadowStack/NotApplicable/ARM_CETShadowStack_NotApplicable.exe
View file
Open in desktop
Binary file not shown.
Binary file added ...ionalTestData/BA2025.EnableShadowStack/NotApplicable/ARM_CETShadowStack_NotApplicable.pdb
View file
Open in desktop
Binary file not shown.
132 changes: 128 additions & 4 deletions src/Test.FunctionalTests.BinSkim.Rules/RuleTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,30 @@ private void VerifyThrows<ExceptionType>(
}
}

private void VerifyApplicabililtyWithReason(
BinarySkimmer skimmer,
HashSet<string> applicabilityConditions,
string expectedReasonForNotAnalyzing,
AnalysisApplicability expectedApplicability = AnalysisApplicability.NotApplicableToSpecifiedTarget,
bool useDefaultPolicy = false)
{

string ruleName = skimmer.GetType().Name;
string testFilesDirectory = GetTestDirectoryFor(ruleName);
testFilesDirectory = Path.Combine(Environment.CurrentDirectory, "FunctionalTestData", testFilesDirectory);
testFilesDirectory = Path.Combine(testFilesDirectory, "NotApplicable");

HashSet<string> targets = this.GetTestFilesMatchingConditions(applicabilityConditions);

VerifyApplicabilityResults(
skimmer,
targets,
useDefaultPolicy,
expectedApplicability,
ruleName,
expectedReasonForNotAnalyzing);
}

private void VerifyApplicability(
BinarySkimmer skimmer,
HashSet<string> applicabilityConditions,
Expand All @@ -261,8 +285,6 @@ private void VerifyApplicability(
testFilesDirectory = Path.Combine(Environment.CurrentDirectory, "FunctionalTestData", testFilesDirectory);
testFilesDirectory = Path.Combine(testFilesDirectory, "NotApplicable");

var context = new BinaryAnalyzerContext();

HashSet<string> targets = this.GetTestFilesMatchingConditions(applicabilityConditions);

if (Directory.Exists(testFilesDirectory))
Expand All @@ -276,6 +298,88 @@ private void VerifyApplicability(
}
}

VerifyApplicabilityResults(
skimmer,
targets,
useDefaultPolicy,
expectedApplicability,
ruleName,
expectedReasonForNotAnalyzing);

//var logger = new TestMessageLogger();
//context.Logger = logger;

//var sb = new StringBuilder();

//foreach (string target in targets)
//{
// string extension = Path.GetExtension(target);

// context = this.CreateContext(logger, null, target);
// if (!context.IsValidAnalysisTarget) { continue; }

// if (useDefaultPolicy)
// {
// context.Policy = new PropertiesDictionary();
// }

// context.Rule = skimmer;

// AnalysisApplicability applicability = skimmer.CanAnalyze(context, out string reasonForNotAnalyzing);

// if (applicability != expectedApplicability)
// {
// // Generates message such as the following:
// // "'BA2025:EnableShadowStack' - 'CanAnalyze' did not correctly indicate target applicability
// // (unexpected return was 'NotApplicableToSpecifiedTarget'): ARM64_CETShadowStack_NotApplicable.exe"
// sb.AppendLine(
// string.Format(
// "'{0}:{1}' - 'CanAnalyze' did not correctly indicate target applicability (unexpected return was '{2}'): {3}",
// skimmer.Id,
// ruleName,
// applicability,
// Path.GetFileName(target)));

// continue;
// }

// if (expectedReasonForNotAnalyzing != null && reasonForNotAnalyzing != expectedReasonForNotAnalyzing)
// {
// // Generates message such as the following:
// // "'BA2025:EnableShadowStack' - 'CanAnalyze' produced expected outcome but unexpected reason identified
// // (unexpected return was 'image is an ARM64 binary' but 'test' was expected): ARM64_CETShadowStack_NotApplicable.exe"
// sb.AppendLine(
// string.Format(
// "'{0}:{1}' - 'CanAnalyze' produced expected outcome but unexpected reason identified (unexpected return was '{2}' but '{3}' was expected): {4}",
// skimmer.Id,
// ruleName,
// reasonForNotAnalyzing,
// expectedReasonForNotAnalyzing,
// Path.GetFileName(target)));

// continue;
// }
//}

//if (sb.Length > 0)
//{
// this.testOutputHelper.WriteLine(sb.ToString());
//}

//Assert.Equal(0, sb.Length);
}

private void VerifyApplicabilityResults(
BinarySkimmer skimmer,
HashSet<string> targets,
bool useDefaultPolicy,
AnalysisApplicability expectedApplicability,
string ruleName,
string expectedReasonForNotAnalyzing)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created method for shared code.


var context = new BinaryAnalyzerContext();
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first line empty can be removed. #Closed


var logger = new TestMessageLogger();
context.Logger = logger;

Expand Down Expand Up @@ -460,6 +564,17 @@ private HashSet<string> GetTestFilesMatchingConditions(HashSet<string> metadataC
result.Add(Path.Combine(testFilesDirectory, "DotnetNative_x86_VS2019_UniversalApp.exe"));
}

if (metadataConditions.Contains(MetadataConditions.ImageIsArmBinary))
{
result.Add(Path.Combine(testFilesDirectory, "ARM_CETShadowStack_NotApplicable.exe"));
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARM_CETShadowStack_NotApplicable.exe

nice, how did you generate this file, please add the step in md file
\docs\FunctionalTestBuildScripts.md #Closed

}

if (metadataConditions.Contains(MetadataConditions.ImageIsArm64BitBinary))
{
result.Add(Path.Combine(testFilesDirectory, "ARM64_CETShadowStack_NotApplicable.exe"));
result.Add(Path.Combine(testFilesDirectory, "ARM64_dotnet_CETShadowStack_NotApplicable.exe"));
}

return result;
}

Expand Down Expand Up @@ -1166,10 +1281,19 @@ public void BA2025_EnableShadowStack_Fail()
[Fact]
public void BA2025_EnableShadowStack_NotApplicable()
{
this.VerifyApplicability(
HashSet<string> notApplicableArm64 = new HashSet<string>() { MetadataConditions.ImageIsArm64BitBinary };

this.VerifyApplicabililtyWithReason(
skimmer: new EnableShadowStack(),
applicabilityConditions: null,
applicabilityConditions: notApplicableArm64,
expectedReasonForNotAnalyzing: MetadataConditions.ImageIsArm64BitBinary);

HashSet<string> notApplicableArm = new HashSet<string>() { MetadataConditions.ImageIsArmBinary };

this.VerifyApplicabililtyWithReason(
skimmer: new EnableShadowStack(),
applicabilityConditions: notApplicableArm64,
expectedReasonForNotAnalyzing: MetadataConditions.ImageIsArmBinary);
}

[Fact]
Expand Down