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

Jump to bottom

BA3005.EnableStackClashProtection #379

Merged
merged 4 commits into from
May 21, 2021
Merged

BA3005.EnableStackClashProtection #379

merged 4 commits into from
May 21, 2021

Conversation

shaopeng-gh
Copy link
Collaborator

BA3005.EnableStackClashProtection

eddynaka
eddynaka reviewed May 20, 2021
View reviewed changes
src/BinSkim.Rules/ELFRules/BA3005.EnableStackClashProtection.cs Outdated
/// An attacker can exploit this confusion to overwrite the stack with the other memory region, or the other way around.
/// Use the compiler flags '-fstack-clash-protection' to enable this.
/// </summary>
public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.BA3005_EnableStackClashProtection_Description };
Copy link
Contributor

@eddynaka eddynaka May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Text

add this in the next line #Closed

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

eddynaka
eddynaka reviewed May 20, 2021
View reviewed changes
src/BinSkim.Rules/ELFRules/BA3005.EnableStackClashProtection.cs Outdated
}
else
{
dwarfCompilerCommand = target.GetDwarfCompilerCommand();
Copy link
Contributor

@eddynaka eddynaka May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

dwarfCompilerCommand

don't save this, since you can retrieve in the analyze.
do this only if you could not retrieve during analysis #Closed

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

eddynaka
eddynaka reviewed May 20, 2021
View reviewed changes
src/BinSkim.Rules/ELFRules/BA3005.EnableStackClashProtection.cs Outdated

public override void Analyze(BinaryAnalyzerContext context)
{
if (!dwarfCompilerCommand.ToLower().Contains("-fstack-clash-protection")
Copy link
Contributor

@eddynaka eddynaka May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

dwarfCompilerCommand.ToLower().Contains

replace with Contains("string", stringcomparison.OrdinalgnoreCase) #Closed

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

eddynaka
eddynaka reviewed May 20, 2021
View reviewed changes
src/BinSkim.Sdk/MetadataConditions.cs Outdated
@@ -15,6 +15,8 @@ public static class MetadataConditions
public static readonly string ImageIsBootBinary = SdkResources.MetadataCondition_ImageIsBootBinary;
public static readonly string ImageIs64BitBinary = SdkResources.MetadataCondition_ImageIs64BitBinary;
public static readonly string ElfNotBuiltWithGcc = SdkResources.MetadataCondition_ElfNotBuiltWithGCC;
public static readonly string ElfNotBuiltWithGccV8OrLater = SdkResources.MetadataCondition_ElfNotBuiltWithGccV8OrLater;
Copy link
Contributor

@eddynaka eddynaka May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ElfNotBuiltWithGccV8OrLater

move near the end to make the length in crescent order :) #Closed

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. This style is cool :)

eddynaka
eddynaka reviewed May 21, 2021
View reviewed changes
src/Test.UnitTests.BinaryParsers/ELF/ELFBinaryTests.cs Outdated
@@ -88,5 +88,45 @@ public void ValidateDwarfV5_WithO2_Split_DebugFileMissing()
binary.GetDwarfVersion().Should().Be(5);
binary.GetLanguage().Should().Be(DwarfLanguage.Unknown); //missing dwo file should not cause exception
}

[Fact]
public void ValidateDwarfV5_WithO2_No_Stack_Clash_Protection()
Copy link
Contributor

@eddynaka eddynaka May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ValidateDwarfV5_WithO2_No_Stack_Clash_Protection

you don't need to add this. The tests above were used to test the ELFBinary, and since we know it works, this isn't needed since you are already testing in the functionaltests. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good, removed.

eddynaka
eddynaka approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@eddynaka eddynaka left a comment

Choose a reason for hiding this comment

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

:shipit:

@eddynaka eddynaka merged commit 5e929c0 into main May 21, 2021
@eddynaka eddynaka deleted the users/shaopeng-gh/stackclashprotectionrule branch May 21, 2021 20:26
@eddynaka eddynaka linked an issue May 24, 2021 that may be closed by this pull request
[RULE REQUEST] Enable Stack Clash Protection #369
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddynaka eddynaka eddynaka approved these changes

@michaelcfanning michaelcfanning Awaiting requested review from michaelcfanning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RULE REQUEST] Enable Stack Clash Protection
2 participants
@shaopeng-gh @eddynaka