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

Jump to bottom

FEATURE: Add BA3031.EnableClangSafeStack #663

Merged

michaelcfanning merged 6 commits into main from users/shaopeng-gh/safe_stack

Jul 12, 2022

Collaborator

shaopeng-gh commented Jul 7, 2022 •

edited

Loading

This new rule applies to Clang only since GCC does not support it.
More info:
https://clang.llvm.org/docs/SafeStack.html
#634

Also fixed:

rename BA3030.UseCheckedFunctionsWithGcc to BA3030.UseGccCheckedFunctions
rename MetadataConditions.ElfIsCoreNoneOrObject to MetadataConditions.ElfIsCoreNoneOrRelocatable

shaopeng-gh added 2 commits

July 6, 2022 23:02


          * FEATURE: Add BA3031.EnableSafeStackWithClang.

abd0cd5


          Fix spacing.

4bb866a

shaopeng-gh requested review from michaelcfanning and marmegh as code owners

July 7, 2022 06:11

shaopeng-gh requested a review from yongyan-gh

July 7, 2022 06:11

shaopeng-gh commented

View reviewed changes

....BinSkim.Driver/BaselineTestData/Expected/Native_x64_RustC_Rust_debuginfo2_v1.58.1.exe.sarif

@@ @@ -951,7 +951,7 @@ @@
                           },
                           {
                             "id": "BA2026",
-                            "name": "EnableAdditionalSdlSecurityChecks",
+                            "name": "EnableMicrosoftCompilerSdlSwitch",

Collaborator Author

shaopeng-gh Jul 7, 2022 •

edited by marmegh

Loading

EnableMicrosoftCompilerSdlSwitch

This is not related to the current change, fix by the way. #Closed

Contributor

marmegh Jul 7, 2022

Nice. Thanks for fixing this.

shaopeng-gh commented

View reviewed changes

src/Test.FunctionalTests.BinSkim.Rules/RuleTests.cs Outdated

                           this.VerifyApplicability(new UseCheckedFunctionsWithGcc(), new HashSet<string>());
                       }
+                      [Fact]
+                      public void BA3031_EnableSafeStackWithClang_Pass()

Collaborator Author

shaopeng-gh Jul 7, 2022 •

edited

Loading

public void BA3031_EnableSafeStackWithClang_Pass()

the combination of the test files are:

version, from latest 14 down to 7
language, C and C++

(By these tests we found that lower version has different symbol name) #Closed

Contributor

marmegh Jul 7, 2022

Awesome, do we have build script examples to include for these?

Collaborator Author

shaopeng-gh Jul 7, 2022

yes! added.


          Add build script

a76da09

shaopeng-gh commented

View reviewed changes

docs/FunctionalTestBuildScripts.md Outdated

               Base scenario is a simple hello world program built with different parameters for testing purpose.
               Test files are located in [BaselineTestData](https://github.com/microsoft/binskim/tree/main/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData) and [FunctionalTestData](https://github.com/microsoft/binskim/tree/main/src/Test.FunctionalTests.BinSkim.Rules/FunctionalTestData).
+              ## ARM64_CETShadowStack_NotApplicable.exe

Collaborator Author

shaopeng-gh Jul 7, 2022 •

edited

Loading

this is just a re-order alphabetically #Closed


          Adjust Order

8cfab55

shaopeng-gh commented

View reviewed changes

docs/FunctionalTestBuildScripts.md

+              A simple C++ hellow world program, cross compiled using CMake with the `cl.exe` compiler and `Ninja` generator.
+              `CMakePresets.json` should be configured with a `configurePresets` as below:
+              ```json

Collaborator Author

shaopeng-gh Jul 7, 2022 •

edited

Loading

json

I added the language for ``` per linter request. and empty lines. #Closed

michaelcfanning reviewed

View reviewed changes

docs/BinSkimRules.md Outdated

		@@ -226,6 +226,28 @@ No checked functions are present/used when compiling '{0}', and it was compiled

		---

		## Rule `BA3031.EnableSafeStackWithClang`

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

EnableSafeStackWithClang

EnableClangSafeStack?

I will think about the name and help refine this help text a bit further. #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

fixed

michaelcfanning reviewed

View reviewed changes

src/BinSkim.Rules/RuleIds.cs Outdated

@@ @@ -61,6 +61,7 @@ internal static class RuleIds @@
                       // BA3012-3029 -- saved for future non-compiler/language specific checks.
                       // Compiler/Language specific checks follow.
                       public const string UseCheckedFunctionsWithGcc = "BA3030";

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

UseCheckedFunctionsWithGcc

UseGccCheckedFunctions #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

fixed

michaelcfanning reviewed

View reviewed changes

src/BinSkim.Rules/ElfRules/BA3031.EnableSafeStackWithClang.cs Outdated

+                          if (elf.Type == FileType.Core || elf.Type == FileType.None || elf.Type == FileType.Relocatable)
+                          {
+                              reasonForNotAnalyzing = MetadataConditions.ElfIsCoreNoneOrObject;

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

ElfIsCoreNoneOrObject

I would tend to name this 'relocatable' as it is, I think, the most precise *nix term for what we're looking at. #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

fixed to ElfIsCoreNoneOrRelocatable

michaelcfanning reviewed

View reviewed changes

src/BinSkim.Rules/ElfRules/BA3031.EnableSafeStackWithClang.cs Outdated

+                          if (!target.Compilers.Any(c => c.Compiler == ElfCompilerType.Clang && c.Version.Major >= 7))
+                          {
+                              reasonForNotAnalyzing = MetadataConditions.ElfNotBuiltWithClangV7OrLater;

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

ElfNotBuiltWithClangV7OrLater

Use a generic message, BinaryCompiledWithOutdatedTools or just use the existing one ImageCompiledWithOutdatedTools? #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

the comment is replaced by later discussions to fire a special error that says you should enable SafeStack, you might need to update your version of Clang to do this.

michaelcfanning reviewed

View reviewed changes

src/BinSkim.Rules/ElfRules/BA3031.EnableSafeStackWithClang.cs Outdated

+                          IEnumerable<ISymbolEntry> symbols =
+                              ElfUtility.GetAllSymbols(elf).Where(sym => sym.Type == SymbolType.File);
+                          if (symbols.Any(s => symbolSafeStack.Contains(s.Name)))

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

Foreach over the symbols, to avoid a 2x traversal in the pathological case. Fire a pass result and exit if you meet the first condition. Otherwise fall out and fire the error result. #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

Changed to not use Linq and just use foreach.
(It may not result in 2x traversal though)

michaelcfanning reviewed

View reviewed changes

src/BinSkim.Rules/ElfRules/BA3031.EnableSafeStackWithClang.cs Outdated

+                          {
+                              reasonForNotAnalyzing = MetadataConditions.ElfNotBuiltWithClangV7OrLater;
+                              return AnalysisApplicability.NotApplicableToSpecifiedTarget;
+                          }

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

Remove this. #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

Removed the version check only, we still need to Clang check, if people just use Gcc we will skip this image as NotApplicable.

michaelcfanning reviewed

View reviewed changes

src/BinSkim.Rules/ElfRules/BA3031.EnableSafeStackWithClang.cs Outdated

+                                         nameof(RuleResources.BA3031_Pass),
+                                         context.TargetUri.GetFileName()));
+                          }
+                          else

Member

michaelcfanning Jul 11, 2022 •

edited by shaopeng-gh

Loading

else

When you exit the function, you could do a version check, and if it's v3.8 or whatever minimal version we know about, just fire an error.

Otherwise fire a special error that says you should enable SafeStack, you might need to update your version of Clang to do this. #Resolved

Collaborator Author

shaopeng-gh Jul 12, 2022

agree with this approach, fixed.

michaelcfanning approved these changes

View reviewed changes

Member

michaelcfanning left a comment


          Fix comments from Michael

6dcc01f

shaopeng-gh changed the title ~~FEATURE: Add BA3031.EnableSafeStackWithClang~~ FEATURE: Add BA3031.EnableClangSafeStack

github-advanced-security bot found potential problems

View reviewed changes

src/BinSkim.Rules/ElfRules/BA3031.EnableClangSafeStack.cs

Comment on lines +74 to +84

+                          foreach (ISymbolEntry symbol in symbols)
+                          {
+                              if (symbol.Type == SymbolType.File && symbolSafeStack.Contains(symbol.Name))
+                              {
+                                  context.Logger.Log(this,
+                                     RuleUtilities.BuildResult(ResultKind.Pass, context, null,
+                                         nameof(RuleResources.BA3031_Pass),
+                                         context.TargetUri.GetFileName()));
+                                  return;
+                              }
+                          }

Check notice

Code scanning / CodeQL

Missed opportunity to use Where

This foreach loop implicitly filters its target sequence [here](1) - consider filtering the sequence explicitly using '.Where(...)'.

shaopeng-gh commented

View reviewed changes

....FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/BinSkim.win-x64.ni.dll.sarif.temp2

		@@ -1,133 +0,0 @@
		{

Collaborator Author

shaopeng-gh Jul 12, 2022 •

edited

Loading

this is just to remove not needed checked in temp file.


          fix minor issue

2f7651e

michaelcfanning merged commit 45ec5ac into main

michaelcfanning deleted the users/shaopeng-gh/safe_stack branch

July 12, 2022 14:44

marmegh mentioned this pull request

Preparing to release BinSkim v1.10.0 #701

Closed

marmegh mentioned this pull request

[RULE REQUEST] Add SafeStack Identification for ELF and Mach-O Binaries #634

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet