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.

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

FEATURE: Add BA3031.EnableClangSafeStack #663

Merged
merged 6 commits into from
Jul 12, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
22 changes: 22 additions & 0 deletions docs/BinSkimRules.md
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,28 @@ No checked functions are present/used when compiling '{0}', and it was compiled

---

## Rule `BA3031.EnableSafeStackWithClang`
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

EnableSafeStackWithClang

EnableClangSafeStack?

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


### Description

The SafeStack instrumentation pass protects programs by implementing two separate program stacks, one for return addresses and local variables, and the other for everything else. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines.

### Messages

#### `Pass`: Pass

'{0}' was compiled using Clang and with the SafeStack instrumentation pass, which mitigates the risk of stack-based buffer overflows.

#### `Error`: Error

'{0}' was compiled using Clang but without the SafeStack instrumentation pass, which should be used to mitigate the risk of stack-based buffer overflows. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines.

#### `InvalidMetadata`: NotApplicable

'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}.

---

## Rule `BA5001.EnablePositionIndependentExecutableMachO`

### Description
Expand Down
83 changes: 46 additions & 37 deletions docs/FunctionalTestBuildScripts.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,52 @@ This file records scripts used to compile the test files, in alphabetical order.
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
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Jul 7, 2022

Choose a reason for hiding this comment

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

this is just a re-order alphabetically #Closed


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

```json
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Jul 7, 2022

Choose a reason for hiding this comment

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

json

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

{
"name": "arm64-release",
"displayName": "ARM64 Release",
"inherits": "windows-base",
"architecture": {
"value": "arm64",
"strategy": "external"
},
"cacheVariables": {
"CMAKE_BUILD_TYPE": "RelWithDebInfo"
}
},
```

## ARM_CETShadowStack_NotApplicable.exe

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
{
"name": "arm-release",
"displayName": "ARM Release",
"inherits": "windows-base",
"architecture": {
"value": "arm",
"strategy": "external"
},
"cacheVariables": {
"CMAKE_BUILD_TYPE": "RelWithDebInfo"
}
},
```

## clang.[version].elf.[c,cpp].[no_]safe_stack

A simple hello world C/C++ program, compiled with different `clang [version]` that generates a executable file. Script to reproduce:
`clang++-14 -Wall hellocpp.cpp -O2 -g -gdwarf-5 -o clang.14.elf.cpp.no_safe_stack`
`clang-7 -Wall helloc.c -O2 -g -gdwarf-5 -o clang.7.elf.c.safe_stack -fsanitize=safe-stack`

## clang.object_file.dwarf.3.o

A simple hello world program, compiled with `clang 10.0.0` that generates a .o object file. Script to reproduce:
Expand Down Expand Up @@ -47,40 +93,3 @@ Also create two user functions `userfn_use_safebuffers_1()` and `userfn_use_safe
A simple `Windows Kernel Mode Driver` program, created with `Visual Studio 2019` that generates a .exe and associated .pdb file. Code to reproduce:
Use `NTSTATUS GsDriverEntry(_In_ PDRIVER_OBJECT DriverObject, _In_ PUNICODE_STRING RegistryPath)` as entry point and do not decorated with `__declspec(safebuffers)`.
No user functions decorated with `__declspec(safebuffers)`.

## ARM64_CETShadowStack_NotApplicable.exe
A simple C++ hellow world program, cross compiled using CMake using the `cl.exe` compiler and `Ninja` generator.
`CMakePresets.json` should be configured with a `configurePresets` as below:
```
{
"name": "arm64-release",
"displayName": "ARM64 Release",
"inherits": "windows-base",
"architecture": {
"value": "arm64",
"strategy": "external"
},
"cacheVariables": {
"CMAKE_BUILD_TYPE": "RelWithDebInfo"
}
},
```

## ARM_CETShadowStack_NotApplicable.exe

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:
```
{
"name": "arm-release",
"displayName": "ARM Release",
"inherits": "windows-base",
"architecture": {
"value": "arm",
"strategy": "external"
},
"cacheVariables": {
"CMAKE_BUILD_TYPE": "RelWithDebInfo"
}
},
```
90 changes: 90 additions & 0 deletions src/BinSkim.Rules/ElfRules/BA3031.EnableSafeStackWithClang.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;
using System.Composition;
using System.Linq;

using ELFSharp.ELF;
using ELFSharp.ELF.Sections;

using Microsoft.CodeAnalysis.BinaryParsers;
using Microsoft.CodeAnalysis.IL.Sdk;
using Microsoft.CodeAnalysis.Sarif;
using Microsoft.CodeAnalysis.Sarif.Driver;

namespace Microsoft.CodeAnalysis.IL.Rules
{
[Export(typeof(Skimmer<BinaryAnalyzerContext>)), Export(typeof(ReportingDescriptor))]
public class EnableSafeStackWithClang : ElfBinarySkimmer
{
// Symbol is the same for both C and C++, despite the "cpp" or "cc" in file name.
// Clang V7 - V9: "safestack.cc.o"
// Clang V10 - V14: "safestack.cpp.o"
private static readonly string[] symbolSafeStack = new string[] { "safestack.cpp.o", "safestack.cc.o" };

/// <summary>
/// BA3031
/// </summary>
public override string Id => RuleIds.EnableSafeStackWithClang;

/// <summary>
/// The SafeStack instrumentation pass protects programs by implementing two separate program stacks,
/// one for return addresses and local variables, and the other for everything else.
/// To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines.
/// </summary>
public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.BA3031_EnableSafeStackWithClang_Description };

protected override IEnumerable<string> MessageResourceNames => new string[] {
nameof(RuleResources.BA3031_Pass),
nameof(RuleResources.BA3031_Error),
nameof(RuleResources.NotApplicable_InvalidMetadata)
};

public override AnalysisApplicability CanAnalyzeElf(ElfBinary target, Sarif.PropertiesDictionary policy, out string reasonForNotAnalyzing)
{
IELF elf = target.ELF;

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

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed to ElfIsCoreNoneOrRelocatable

return AnalysisApplicability.NotApplicableToSpecifiedTarget;
}

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

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

ElfNotBuiltWithClangV7OrLater

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

return AnalysisApplicability.NotApplicableToSpecifiedTarget;
}
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

Remove this. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


reasonForNotAnalyzing = null;
return AnalysisApplicability.ApplicableToSpecifiedTarget;
}

/// <summary>
/// Checks if SafeStack is enabled by Symbols.
/// </summary>
public override void Analyze(BinaryAnalyzerContext context)
{
IELF elf = context.ElfBinary().ELF;

IEnumerable<ISymbolEntry> symbols =
ElfUtility.GetAllSymbols(elf).Where(sym => sym.Type == SymbolType.File);

if (symbols.Any(s => symbolSafeStack.Contains(s.Name)))
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

{
context.Logger.Log(this,
RuleUtilities.BuildResult(ResultKind.Pass, context, null,
nameof(RuleResources.BA3031_Pass),
context.TargetUri.GetFileName()));
}
else
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree with this approach, fixed.

{
context.Logger.Log(this,
RuleUtilities.BuildResult(FailureLevel.Error, context, null,
nameof(RuleResources.BA3031_Error),
context.TargetUri.GetFileName()));
}
}
}
}
1 change: 1 addition & 0 deletions src/BinSkim.Rules/RuleIds.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

@michaelcfanning michaelcfanning Jul 11, 2022

Choose a reason for hiding this comment

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

UseCheckedFunctionsWithGcc

UseGccCheckedFunctions #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

public const string EnableSafeStackWithClang = "BA3031";

// Reporting checks
public const string ReportPortableExecutableCompilerData = "BA4001";
Expand Down
27 changes: 27 additions & 0 deletions src/BinSkim.Rules/RuleResources.Designer.cs

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

9 changes: 9 additions & 0 deletions src/BinSkim.Rules/RuleResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -573,4 +573,13 @@ Modules did not meet the criteria: {1}</value>
<data name="ERR997_ExceptionLoadingPdbGeneric" xml:space="preserve">
<value>'{0}' was not evaluated because its PDB could not be loaded ({1}).</value>
</data>
<data name="BA3031_EnableSafeStackWithClang_Description" xml:space="preserve">
<value>The SafeStack instrumentation pass protects programs by implementing two separate program stacks, one for return addresses and local variables, and the other for everything else. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines.</value>
</data>
<data name="BA3031_Error" xml:space="preserve">
<value>'{0}' was compiled using Clang but without the SafeStack instrumentation pass, which should be used to mitigate the risk of stack-based buffer overflows. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines.</value>
</data>
<data name="BA3031_Pass" xml:space="preserve">
<value>'{0}' was compiled using Clang and with the SafeStack instrumentation pass, which mitigates the risk of stack-based buffer overflows.</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/BinSkim.Sdk/MetadataConditions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public static class MetadataConditions
public static readonly string ImageIsResourceOnlyAssembly = SdkResources.MetadataCondition_ImageIsResourceOnlyAssembly;
public static readonly string ElfNotBuiltWithGccV8OrLater = SdkResources.MetadataCondition_ElfNotBuiltWithGccV8OrLater;
public static readonly string ImageIsKernelModeAndNot64Bit = SdkResources.MetadataCondition_ImageIsKernelModeAndNot64Bit;
public static readonly string ElfNotBuiltWithClangV7OrLater = SdkResources.MetadataCondition_ElfNotBuiltWithClangV7OrLater;
public static readonly string ImageIsDotNetCoreBootstrapExe = SdkResources.MetadataCondition_ImageIsDotNetCoreBootstrapExe;
public static readonly string ImageLikelyLoadsAs32BitProcess = SdkResources.MetadataCondition_ImageLikelyLoads32BitProcess;
public static readonly string ElfNotBuiltWithDwarfDebugging = SdkResources.MetadataCondition_ElfNotBuiltWithDwarfDebugging;
Expand Down
9 changes: 9 additions & 0 deletions src/BinSkim.Sdk/SdkResources.Designer.cs

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
Original file line number Diff line number Diff line change
Expand Up @@ -246,4 +246,7 @@
<data name="MetadataCondition_ImageIsArmBinary" xml:space="preserve">
<value>image is an ARM binary</value>
</data>
<data name="MetadataCondition_ElfNotBuiltWithClangV7OrLater" xml:space="preserve">
<value>not compiled with Clang v7 or later</value>
</data>
</root>
4 changes: 4 additions & 0 deletions src/ReleaseHistory.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# BinSkim Release History

## Unreleased

* FEATURE: Add BA3031.EnableSafeStackWithClang. [#663](https://github.com/microsoft/binskim/pull/663)

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

* Bump ELFSharp from 2.13.2 to 2.14.0. [#628](https://github.com/microsoft/binskim/pull/628)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,7 @@
},
{
"id": "BA2026",
"name": "EnableAdditionalSdlSecurityChecks",
"name": "EnableMicrosoftCompilerSdlSwitch",
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Jul 7, 2022

Choose a reason for hiding this comment

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

EnableMicrosoftCompilerSdlSwitch

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Thanks for fixing this.

"fullDescription": {
"text": "/sdl enables a superset of the baseline security checks provided by /GS and overrides /GS-. By default, /sdl is off. /sdl- disables the additional security checks."
},
Expand All @@ -969,7 +969,7 @@
"text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}."
}
},
"helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2026EnableAdditionalSdlSecurityChecks"
"helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2026EnableMicrosoftCompilerSdlSwitch"
}
],
"properties": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,27 @@
}
}
]
},
{
"ruleId": "BA3031",
"ruleIndex": 6,
"level": "error",
"message": {
"id": "Error",
"arguments": [
"clang.elf.objectivec.dwarf"
]
},
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/clang.elf.objectivec.dwarf",
"index": 0
}
}
}
]
}
],
"tool": {
Expand Down Expand Up @@ -277,6 +298,28 @@
}
},
"helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA3011EnableBindNow"
},
{
"id": "BA3031",
"name": "EnableSafeStackWithClang",
"fullDescription": {
"text": "The SafeStack instrumentation pass protects programs by implementing two separate program stacks, one for return addresses and local variables, and the other for everything else. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines."
},
"help": {
"text": "The SafeStack instrumentation pass protects programs by implementing two separate program stacks, one for return addresses and local variables, and the other for everything else. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines."
},
"messageStrings": {
"Pass": {
"text": "'{0}' was compiled using Clang and with the SafeStack instrumentation pass, which mitigates the risk of stack-based buffer overflows."
},
"Error": {
"text": "'{0}' was compiled using Clang but without the SafeStack instrumentation pass, which should be used to mitigate the risk of stack-based buffer overflows. To enable SafeStack, pass '-fsanitize=safe-stack' flag to both compile and link command lines."
},
"NotApplicable_InvalidMetadata": {
"text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}."
}
},
"helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA3031EnableSafeStackWithClang"
}
],
"properties": {
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading