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

[RULE REQUEST] skip "debug only" ELF files #405

Closed
chipitsine opened this issue Jul 8, 2021 · 22 comments
Closed

[RULE REQUEST] skip "debug only" ELF files #405

chipitsine opened this issue Jul 8, 2021 · 22 comments

Comments

@chipitsine
Copy link

let us consider the following source file

#include <stdio.h>

void main(void){
        double buf[66];
        printf("hello, world!\n");
}

and we build binary and debug symbols

gcc -ggdb -fPIC -fstack-protector-strong --param ssp-buffer-size=4 -fstack-clash-protection -o hello hello.c
objcopy --only-keep-debug hello hello.dbg

file hello.dbg is ELF, but is not executable. I suggest skip such files from analyzing

ilia@localhostr:~/hello$ file hello
hello: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=f4d9d5f5305d875d8442d5d4b82c4e7a2eca2980, for GNU/Linux 3.2.0, with debug_info, not stripped
ilia@localhost:~/hello$ file hello.dbg
hello.dbg: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter *empty*, BuildID[sha1]=f4d9d5f5305d875d8442d5d4b82c4e7a2eca2980, for GNU/Linux 3.2.0, with debug_info, not stripped
ilia@localhost:~/hello$

currently, BinSkim complains on hello.dbg

C:\i\binskim-1.7.5>BinSkim.exe analyze c:\i\hello\*.* --recurse                                                                                               Analyzing...
Analyzing 'hello'...
Analyzing 'hello.dbg'...
c:\i\hello\hello.dbg: error BA3003: The stack protector was not found in 'hello.dbg'.  This may be because the binary has no stack-based arrays, or because '--stack-protector-strong' was not used.
Analysis completed successfully.

indeed, hello.dbg is ELF, but it is not executable.

@eddynaka
Copy link
Contributor

eddynaka commented Jul 8, 2021

Hi @chipitsine ,

You should only analyze the hello file and, if needed, Binskim would try to load the .dbg

@chipitsine
Copy link
Author

I analyze output folder recursively using Guargian

@chipitsine
Copy link
Author

It would be really nice if BinSkim will skip dbg by itself

@eddynaka
Copy link
Contributor

eddynaka commented Jul 8, 2021

It would be really nice if BinSkim will skip dbg by itself

It's complex because Binskim cannot know what extension did you use. For example, in the second command, you could use .abc instead of .dbg.

Since binskim tries to analyze everything that you pass in the pattern, you should try to:

  1. move the dbg to another folder
  2. check if you can improve the glob pattern in the guardian task.

@chipitsine
Copy link
Author

There's "empty" interpreter. Why not to skip such files

@chipitsine
Copy link
Author

I do not mean using "dbg" file extension as whitelist marker))

@eddynaka
Copy link
Contributor

eddynaka commented Jul 8, 2021

There's "empty" interpreter. Why not to skip such files

I'm not following...

@eddynaka
Copy link
Contributor

eddynaka commented Jul 8, 2021

@chipitsine ,

What is the difference between objcopy --only-keep-debug hello hello.dbg and using -gsplit-dwarf?
Looking at the hello after the objcopy, looks like it is still keeping the debugging information.

@chipitsine
Copy link
Author

Sure, we strip debug later. Only "dbg" has debig

@chipitsine
Copy link
Author

There's "empty" interpreter. Why not to skip such files

I'm not following...

Internally "dbg" differ from "hello", please see " file" output above

@chipitsine
Copy link
Author

I'll have a look at split-dwarf, thank for the idea

@shaopeng-gh
Copy link
Collaborator

Update:
looking at both, please wait will update in this week

  1. if can support using objcopy --only-keep-debug and strip
  2. if can skip debug only file

@shaopeng-gh
Copy link
Collaborator

shaopeng-gh commented Jul 14, 2021

Update:
have a new code checked in,
with new code it should be able to:

  1. recognize and treat debug only file as not applicable and skip
  2. able to analyze debug separated files generated by this kind of method:

objcopy --only-keep-debug gcc.objcopy.stripall.addgnudebuglink gcc.objcopy.stripall.addgnudebuglink.dbg // create debug only file from original full bin

objcopy --strip-all gcc.objcopy.stripall.addgnudebuglink // remove debug from original full bin and make it bin only file

objcopy --add-gnu-debuglink=gcc.objcopy.stripall.addgnudebuglink.dbg gcc.objcopy.stripall.addgnudebuglink // link bin only file and debug only file

@chipitsine
Copy link
Author

yes, we already use --add-gnu-debuglink
I will test today, thank you!

@chipitsine
Copy link
Author

@shaopeng-gh , mostly it works, however few binaries provide strange errors:

dynamically linked (only one file):

D:\i\xxx.dbg : error ERR997.ExceptionLoadingAnalysisTarget : Could not load analysis target 'xxx.dbg'.

statically linked file

D:\i\xxx: error BA3001: PIE disabled on executable 'xxx'.  This means the code section will always be loaded to the same address, even if ASLR is enabled in the Linux kernel.  To address this, ensure you are compiling with '-fpie' when using clang/gcc.
D:\i\xxx: error BA3003: The stack protector was not found in 'xxx'.  This may be because the binary has no stack-based arrays, or because '--stack-protector-strong' was not used.
D:\i\xxx: error BA3010: The GNU_RELRO segment is missing from this binary, so relocation sections in 'xxx' will not be marked as read only after the binary is loaded.  An attacker can overwrite these to redirect control flow.  Ensure you are compiling with the compiler flags '-Wl,z,relro' to address this.
D:\i\xxx : error ERR998.ExceptionInAnalyze : BA3011 : An exception of type 'KeyNotFoundException' was raised analyzing 'xxx' for check 'EnableBindNow' (which has been disabled). The exception may have resulted from a problem related to parsing the analysis target, and not specific to the rule, however.
KeyNotFoundException: Given section .dynamic could not be found in the file.
   at ELFSharp.ELF.ELF`1.GetSection(String name)
   at ELFSharp.ELF.ELF`1.ELFSharp.ELF.IELF.GetSection(String name)
   at Microsoft.CodeAnalysis.IL.Rules.EnableBindNow.HasBindNowFlag(IELF elf) in D:\I\binskim-master\src\BinSkim.Rules\ElfRules\BA3011.EnableBindNow.cs:line 87
   at Microsoft.CodeAnalysis.IL.Rules.EnableBindNow.Analyze(BinaryAnalyzerContext context) in D:\I\binskim-master\src\BinSkim.Rules\ElfRules\BA3011.EnableBindNow.cs:line 60
   at Microsoft.CodeAnalysis.Sarif.Driver.AnalyzeCommandBase`2.AnalyzeTarget(IEnumerable`1 skimmers, TContext context, ISet`1 disabledSkimmers) in D:\I\binskim-master\src\sarif-sdk\src\Sarif.Driver\Sdk\AnalyzeCommandBase.cs:line 770

please let me know how to share those binaries privately

@toshipiazza
Copy link
Member

That error seems like an issue in the BindNow rule, merged in #363. I thought I added protections to gracefully handle a missing .dynamic section, but it seems that GetSection(".dynamic") is throwing an unhandled KeyNotFoundException now.

@toshipiazza
Copy link
Member

This patch fixes the static issue, I'll send a PR this week

diff --git a/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs b/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs
index d1205be..fc7e9f0 100644
--- a/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs
+++ b/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs
@@ -104,6 +104,10 @@ namespace Microsoft.CodeAnalysis.IL.Rules
             {
                 return false;
             }
+            catch (KeyNotFoundException)
+            {
+                return false;
+            }

             return false;
         }

@shaopeng-gh
Copy link
Collaborator

@chipitsine thanks for the feed back.
please let me know how to share those binaries privately ---- sent you an email to your profile email address.

@eddynaka
Copy link
Contributor

This patch fixes the static issue, I'll send a PR this week

diff --git a/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs b/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs
index d1205be..fc7e9f0 100644
--- a/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs
+++ b/src/BinSkim.Rules/ElfRules/BA3011.EnableBindNow.cs
@@ -104,6 +104,10 @@ namespace Microsoft.CodeAnalysis.IL.Rules
             {
                 return false;
             }
+            catch (KeyNotFoundException)
+            {
+                return false;
+            }

             return false;
         }

Thanks @toshipiazza

@eddynaka
Copy link
Contributor

@shaopeng-gh , if u take a look at the snippet from @toshipiazza , u will be able to fix the issue.

@eddynaka
Copy link
Contributor

I just opened a PR that would fix that: #412

@eddynaka
Copy link
Contributor

We just released a new prerelease version with the fix for this issue.

If you face any new issues, pls, feel free to open a new issue!
thank you!

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

No branches or pull requests

4 participants