-
Notifications
You must be signed in to change notification settings - Fork 627
Add initial elf files support #700
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
Conversation
| for va, _, _, tinfo in vw.getImports(): | ||
| # vivisect source: tinfo = "%s.%s" % (libname, impname) | ||
| modname, impname = tinfo.split(".") | ||
| modname, impname = tinfo.split(".", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| modname, impname = tinfo.split(".", 1) | |
| modname, _, impname = tinfo.partition(".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for this PR @Adir-Shemesh @TcM1911 and https://github.com/intezer!
I am pleasantly surprised how few changes are needed here to enable ELF support. I think we had always hoped that it would be easy to enable and I'm glad to see its the case. More importantly, the initial rule set in mandiant/capa-rules#442 makes this a valuable addition.
I don't have any issues with the code here. I think we should add a handful of regression tests to demonstrate the ELF handling works as expected. Also, there are probably a few updates needed to the documentation/readme. These things can be done in follow up commits rather than blocking this PR.
Did you notice any areas that should be further enhanced to better support ELF files?
|
Hi @williballenthin, I'll let @Adir-Shemesh share feedback with regards to the code part. As for the rules part, we opted to build on the already existing rules and add the needed A thing we have identified that would need some TLC is C++ methods. When malware use for example the C++ STD or Boost, the APIs you need to write are really ugly... The "low level" rules still detects as the methods in the end call a libc function but rules to detect reading specific files don't work as the string is in another function higher up in the call stack. As for tests, I have submitted a test file here mandiant/capa-testfiles#104. |
|
Very cool. Willi and I just chatted about this and will open up an discussion on how to handle the rules to avoid cross-file matching and address some other concerns. Getting the higher level rule matches for free is a great benefit of your approach! It's really great that you provide this update back to the community to kickstart a robust handling of ELF files. Big thanks! |
|
We've begun a discussion on rule organization here: #701 and we would very much appreciate your insight. Some of these ideas we've had for a while but didn't really address because we only supported one OS. Now, we need to figure out what we want to do. |
|
My current thinking is to merge this PR (#700) and the testfile PR mandiant/capa-testfiles#104 very soon/immediately. Thoughts @mr-tz @Ana06 @mike-hunhoff? For the rules PR mandiant/capa-rules#442 we'd like to address two things:
(2) is not anything you're necessarily responsible for; though, this PR now makes it important for us to figure out. So, if you have the patience, we'd like to discuss and (probably) decide before merging. It'll be easier to make changes to the pending patches rather than go back and fixup after a merge. However, we realize that this may feel slower to you and we don't want to discourage further contributions. So, please let us know if you understand and/or what you'd like to see! |
|
I don't have any issues with holding off the merge of the rules until the re-organization has been decided. |
|
#723 adds support for features specifying the os/format/arch. if you have the time, @TcM1911 and @Adir-Shemesh would you take a peek and raise any concerns, etc.? if the PR passes review and gets merged, then we can tweak the ELF rules as appropriate and get the word out. hopefully by early next week or sooner (@mr-tz is on vacation at the moment). incidentally, we're introducing breaking changes so we'll release a capa v3.0 for the ELF support. while we don't necessarily consider major releases as actually "major" its a good time for blog/tweets/etc. Would you all like to put together a blog post on the new ELF support? we'd be happy to share it around and re-post from our accounts - its important to us to recognize how awesome it was to receive these contributions from you! |
|
@williballenthin, sounds good. I'll reach out to you privately. |
Checklist
closes #699