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

Windows: Added '--refined' option to windows malfind plugin #1071

Merged

Conversation

hsarkey
Copy link
Contributor

@hsarkey hsarkey commented Dec 21, 2023

The windows malfind plugin lists process memory ranges that potentially contain injected code. In the original volatility framework, the windows malfind plugin offered a "W" command line option that refined the results to only high confidence matches. The criteria for a high confidence match was either memory regions with an MZ header or that started with a well known opcode combination, ie. PUSH EBP.

This PR adds support for a "--refined" option for volatility3 to match the functionality of the "W" option from the original framework. For this PR, the possible criteria for a high confidence match was updated to include the common opcodes for 32 and 64bit functions ("\x55\x48" and "\x55\x89"), in addition to the original criteria of the MZ header or the PUSH EBP opcode ("\x55\x8B") from volatility. I have tested against various memory samples with injected code to validate the functionality of this option.

Also updated malfind to include "\x55\x48" and "\x55\x89" as part of the refined_criteria list.
@ikelos
Copy link
Member

ikelos commented Jan 1, 2024

Thanks, this looks good I'm just wondering if refined is descriptive enough? I'm not sure what would be better, --high-suspicion or --likely or something along those lines? Something a bit more descriptive about what the flag actually does...

@ikelos
Copy link
Member

ikelos commented Jan 10, 2024

Ok, so we had a bit of a think about this and in general I'm not keen on flags that alter the output of the plugin because that's technically making the plugin decide what's really a user interface decision (do I display this data or not). So in this instance, it might be better to add a permanent column that either lists attributes (like "Common opcodes" or "MZ header" or "PUSH ebp") or a score indicating a change of malware (probably a numeric value to allow for future expansion and tweaking, even if at the moment it only ever returns 0 and 100). We don't yet have filtering capabilities for the CLI, but this may also encourage people to build a better UI for processing the results of volatility. Returning a list of attributes might get messy on the CLI and be difficult to parse for another program, but might be easier to see from a human readable perspective. So either of those work, whichever you think works better?

…s a command line option and instead

added an additional column called "Notes" to provide info on common headers.
…if a process

meets a "refined criteria", meaning it has a common header type (MZ, PE, or a
function prologue).
…cess meets

"refined" criteria, which includes common headers like MZ,PE or function
prologues. Fixed black formatting issue.
@hsarkey
Copy link
Contributor Author

hsarkey commented Jan 18, 2024

After thinking about this, I updated to no longer have the "--refined" command line option and instead added a permanent "Notes" column. This column will display "None" unless a process has one of the common header types discussed above, in which case the "Notes" column will display "MZ header", "PE header", or "Function prologue" depending on what criteria are met.

Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

Hiya, I'm much happier with this change, but I think there's a couple bits that could still be tweaked. Please let me know what you think and once you're happy with any changes you make and we can get it merged. 5:)

volatility3/framework/plugins/windows/malfind.py Outdated Show resolved Hide resolved
process_name = utility.array_to_string(proc.ImageFileName)

for vad, data in self.list_injections(
self.context, kernel.layer_name, kernel.symbol_table_name, proc
):
# Check for unique headers and update "Notes" column if criteria is met
if data[0:2] in refined_criteria:
Copy link
Member

Choose a reason for hiding this comment

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

This list needs keeping in sync with refined_criteria, it might be better to make refined_criteria a dictionary that listen an initial byte pattern with a value for notes? It would make this code a little more generic and it would be easier to add future classifiers if needed...

Copy link
Member

Choose a reason for hiding this comment

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

It might be worth using startswith or something, rather than limiting all future things to exactly 2 bytes, but that's something we can do fairly easily if we decide to extend the dictionary in the future, so not a show stopper.

@hsarkey
Copy link
Contributor Author

hsarkey commented Jan 31, 2024

I took your advice and updated the "refined_criteria" list to be a dictionary, so in case it grows larger in the future, it will easier to maintain. Also, instead of "None", I updated to use the "NotApplicableValue" from the renderers package, which has the BaseAbsentValue you mentioned above.

Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

Yep, looks good now. Just thinking about the specific data[0:2] bit, but not enough to block this. Just let me know if you want to change it before I merge or if you're happier leaving it as is.

process_name = utility.array_to_string(proc.ImageFileName)

for vad, data in self.list_injections(
self.context, kernel.layer_name, kernel.symbol_table_name, proc
):
# Check for unique headers and update "Notes" column if criteria is met
if data[0:2] in refined_criteria:
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth using startswith or something, rather than limiting all future things to exactly 2 bytes, but that's something we can do fairly easily if we decide to extend the dictionary in the future, so not a show stopper.

@hsarkey
Copy link
Contributor Author

hsarkey commented Feb 1, 2024

Hi, I am happy with keeping the data[0:2] for now. If significant changes are made to the dictionary, this can be easily modified, but I'm not sure when/if that would even happen. Thanks for your help on this!

@ikelos ikelos merged commit 9b281c4 into volatilityfoundation:develop Feb 1, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants