Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a Feature Extractor for the Drakvuf Sandbox #2143
Add a Feature Extractor for the Drakvuf Sandbox #2143
Changes from 26 commits
a408629
603d623
90ef348
1e8735a
d2cdccf
840f59f
9e13362
2e408d8
a73d16f
b28e0d0
c05b973
70d03eb
8d4f3c7
bf12ce8
84d68a4
00349d5
53439c7
2663fa6
3bea6e7
15a5efd
0c0c4d0
04ae280
e54f38f
cb7babc
5284ec0
21d50e0
885f216
3b2b022
1e4ed12
b7f4058
0f1750c
4749f24
37f82cb
c45aaa0
aeea39b
9b5dffc
c862f12
cea64d3
718d6ff
32c7a53
7248c0a
de43d1e
3cd5cde
454cd2d
f9d5c4a
6617fc0
8e7bc75
93240f5
c08c5bf
6e0a9eb
2bb7f3c
c0e9150
897e98b
e786552
4cab975
2576aa1
b5047a2
e26072e
d9e3ca1
3e3be41
729679d
3fb0eaf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Again, if I understand the code correctly, and this iterates over arguments from apimon, arg_value won't be a string. Instead, parsed values look like
"0xc6f217efe0:\"ntdll.dll\""
in the JSON. Is that OK?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.
@yelhamer please comment or address
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.
I'm now yielding the
"ntdll.dll"
part of the argument in addition to the entire string (we yield the entire string just in case of unexpected argument formats).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.
@yelhamer can you show some examples from
show-features.py
? I'm not quite following what you mean by this formatting.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.
@williballenthin I meant that for
"0xc6f217efe0:\"ntdll.dll\""
for example it would yieldString("ntdll.dll")
andString("0xc6f217efe0:\"ntdll.dll\"")
, but looking atshow-features.py
it does give misleading results (displays same argument twice):With this in mind I think I might just revert to just yielding
"0xc6f217efe0:\"ntdll.dll\""
as we originally planned, since it would show up inshow-features.py
and might give analysts more insights, and it also wouldn't be misleading like yielding just"ntdll.dll"
, and finally I don't imagine we would be missing any rule matches by yielding"0xc6f217efe0:\"ntdll.dll\""
because the relevant api function would be expecting a memory address so I wouldn't imagine any rules basing any logic on that. Thoughts?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.
should these be blank or contain an indication that this is not available/provided by the sandbox?
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.
Hmm I'm unsure. CAPE's extractor had one of them empty (since it doesn't report it) so I just did the same here.
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.
ok, it's a shame no hash at all is available...
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.
Yeah, unfortunately DRAKVUF is primarily a full VM monitor. In DRAKVUF sandbox it's (ab)used to function as a malware sandbox, but drakmon.log is the output directly from DRAKVUF.
Which is good! It makes this integration more generic (works with DRAKVUF, not just with DRAKVUF sandbox). But that purpose mismatch causes glitches like this.
I think it's possible to send a PR to DRAKVUF that adds logging of sample hashes to the DRAKVUF's
injector
output. If this is valuable I can take a look at this (I can't promise it gets merged, though). But we can't have this in the GSOC timeline, so I hope PR can progress without 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.
loaded DLLs means something else to me than imports - do they mean the same thing here?
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.
My understanding from reading the comments on the relevant drakvuf source code is that the output of this plugin includes imported functions from DLLs loaded by the PE loader, as well as the ones that might be dynamically loaded by a process. I think this because the comments say that they are hooking some windows system calls in order to do this (I believe?), and if this is the case then I feel like this plugin is providing an extensive list of imports which includes static ones as well as dynamic ones that malware might try to load discretely which is why I added this here.
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.
can you please add this documentation to the code?
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.
Hmm, I just noticed that Drakvuf reports the imported functions for each process. Should I extract the imported functions in the process scope instead? this way if a user is analyzing only a specific process then they wouldn't get false results from an import originating from another process.
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.
For the file scope extractors we're only interested in the imports of the target file.
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.
This is an artifact of the static analysis module and likely differs in dynamic analysis and across sandboxes - so maybe we need a new way to handle these?
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.
This thread needs to be resolved.
At the very least, I think we should only yield the imports for the input file.
Optionally, if we can come up with some good motivation and test cases, then we could also extend the sandbox extractor API to cover the recursively imported DLLs/names.
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.
I can confirm that DRAKVUF outputs only execution trace (including loaded DLLs and imported functions) and doesn't concern itself with static analysis.
Can I help with resolving it somehow?
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.
@yelhamer please emit only the import names from the target DLL or none at all. I understand that there's maybe another way to interpret these imports (such as all imports seen in the address space), but this would be inconsistent with other feature extractors, and will be difficult to keep straight and reason about.
I suspect that these import features won't be commonly used, so emitting none at all is usually going to be fine. If we can come up with some specific problematic cases, then we can reassess.
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.
For consistency, as suggested somewhere else
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.
Though actually, this is a bit mixed up (for a lack of a better word). This comment is technically true - DRAKVUF Sandbox (https://github.com/CERT-Polska/drakvuf-sandbox/) only supports x64 windows and PE files.
But - in general - this PR should work for DRAKVUF-the-vm-monitor too. In this case, 32bit windows and ELF files are supported too:
https://github.com/tklengyel/drakvuf/blob/main/README.md?plain=1#L25
In case of DRAKVUF Sandbox (as a maintainer), we don't need Linux or 32bit binary support here. But I'm just pointing it out to Capa maintainers, as a future extension point.
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.
One thing to consider is that capa tries to determine the sample's format and target OS (and architecture):
capa/capa/helpers.py
Line 120 in da6c6cf
For architecture I assume we can look at the addresses (32-bit or 64-bit), but for format and target OS I am not really sure how to do that. That's why I restricted this PR to DRAKVUF sandbox only, but maybe perhaps I should have asked whether there are any suggestions for how to do that (maybe ask for it explicitly via
-f
option)? thoughts?