-
-
Notifications
You must be signed in to change notification settings - Fork 505
Floss Capa Refactor #2933
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
base: develop
Are you sure you want to change the base?
Floss Capa Refactor #2933
Conversation
Couple of points to note with this PR.
|
The code looks good to me, however I tried to replicate your analysis (main.out sample) but I had some problems with the updates. Trying from the plugin pages I got an error message. In the logs there are these:
I tried to replicate the wget command from my command line and seems to work, am I missing something ? |
9400e1c
to
ca4d856
Compare
Hi @drosetti I've made some fixes. It should work now. |
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.
Nice work!
|
||
for signature in signatures_list: | ||
try: | ||
subprocess.run( |
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.
Just to be extra sure, I'd like to make shlex
handle splitting and quoting of parameters before passing them into run()
@spoiicy now I can download correctly the rules, but when I run the analysis I get an error: |
capture_output=True, | ||
) | ||
|
||
except subprocess.CalledProcessError as e: |
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.
If possible I would like to prefer avoid using subprocess for a tool (wget) that performs HTTPS connections that can be handled with classic HTTP libraries. Can you change this?
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.
Please also update migration number since we merged the other PR. Then we should be good 😃
767c04d
to
918965d
Compare
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.
good job! I asked to do additional work regarding the management of the rules which is critical in terms of performance and actual effectiveness of this complex analyzer. Please let me know if you have any doubts about this.
AnalyzerConfig = apps.get_model("analyzers_manager", "AnalyzerConfig") | ||
|
||
pm = PythonModule.objects.get( | ||
module="capa_info.CapaInfo", |
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.
if I remember correctly there is also a param called "docker_based" that should be set to False in the migration to both the analyzers
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.
Done
try: | ||
|
||
response = requests.get(file_url, stream=True) | ||
logger.info(f"Started downloading rules from {file_url}") |
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.
log the version
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.
Done
logger.error(f"Failed to download rules with error: {e}") | ||
raise AnalyzerRunException("Failed to download rules") | ||
|
||
logger.info(f"Rules have been successfully downloaded at {RULES_LOCATION}") |
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.
log the version
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.
Done
|
||
@classmethod | ||
def _download_signatures(cls) -> None: | ||
logger.info(f"Downloading signatures at {SIGNATURE_LOCATION} now") |
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.
the "signatures" are less important than the rules ones. These are almost never updated while the "rules" are are updated often. Plus, most of the time, we don't want these signatures to execute either cause it would slow the Capa execution. The rules are always necessary because they are the core part of the tool while these one could not be necessary. Because of that, I would not re-update them once they are here, like you already do. But we need another additional parameter for the user to enable them explicitly otherwise it would be better if these signatures would be disabled by default.
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.
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.
Plus, most of the time, we don't want these signatures to execute either cause it would slow the Capa execution.
Regarding your point, Actually I've tried executing the flare-capa without the signatures but it threw an error, when only executed with rules. So, I feel the signatures are necessary for it's execution.
Though, I can definitely make changes in the code that the signatures are only downloaded once or updated on-demand by the user.
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.
made the changes such that signatures are only downloaded the first or whenever force_pull_signatures
is set to True.
…oading of signatures
751159c
to
c57d234
Compare
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.
Looks good. Worth considering though. View full project report here.
|
||
|
||
class AnalyzerRulesFileVersion(models.Model): | ||
last_downloaded_version = models.CharField(max_length=50, blank=True, null=True) |
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.
last_downloaded_version = models.CharField(max_length=50, blank=True, null=True) | |
last_downloaded_version = models.CharField(max_length=50, blank=True, default="") |
null=True
on a string field causes inconsistent data types because the value can be either str
or None
. This adds complexity and maybe bugs, but can be solved by replacing null=True
with default=""
. More info.
|
||
class AnalyzerRulesFileVersion(models.Model): | ||
last_downloaded_version = models.CharField(max_length=50, blank=True, null=True) | ||
download_url = models.URLField(max_length=200, blank=True, null=True) |
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.
download_url = models.URLField(max_length=200, blank=True, null=True) | |
download_url = models.URLField(max_length=200, blank=True, default="") |
Similarly, consider replacing null=True
with default=""
(and blank=True
to pass validation checks).
ddeecc4
to
045bc30
Compare
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.
Greatly improved! I'll let @mlodic take a look at this to see if it's how he expected to be 😃
p1 = Parameter( | ||
name="timeout", | ||
type="float", | ||
description="Duration in seconds for which intelowl waits for capa to return results. Default is set to 15 seconds.", |
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.
Maybe I am missing something, but have you set the default of the new parameter?
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.
You are right, I've not set the default for this one. No worries I'll update and add it. :)
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.
Perfect, thank you!
Closes #2738
Description
This PR aims to update the Flare Capa and Flare Floss analyzers, by refactoring the exisiting implementation and utilizing the pip packages for better support.
Type of change
Please delete options that are not relevant.
Checklist
develop
dumpplugin
command and added it in the project as a data migration. ("How to share a plugin with the community")test_files.zip
and you added the default tests for that mimetype in test_classes.py.FREE_TO_USE_ANALYZERS
playbook by following this guide.url
that contains this information. This is required for Health Checks (HEAD HTTP requests)._monkeypatch()
was used in its class to apply the necessary decorators.MockUpResponse
of the_monkeypatch()
method. This serves us to provide a valid sample for testing.DataModel
for the new analyzer following the documentation# This file is a part of IntelOwl https://github.com/intelowlproject/IntelOwl # See the file 'LICENSE' for copying permission.
Black
,Flake
,Isort
) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.tests
folder). All the tests (new and old ones) gave 0 errors.DeepSource
,Django Doctors
or other third-party linters have triggered any alerts during the CI checks, I have solved those alerts.Important Rules