Skip to content

Conversation

@SolitudePy
Copy link
Contributor

@SolitudePy SolitudePy commented Jun 7, 2025

Categorize windows.unhooked_system_calls as windows.malware.unhooked_system_calls

  • no dep fixes applied
  • class name was changed to be different from module name

@SolitudePy
Copy link
Contributor Author

btw why is this called unhooked_system_calls? these ntdll.dll stub functions are basically user-mode wrapper for the actual syscall which code is executed in kernel (hence its not hooking SSDT, or ntoskrnl syscall implementations), also unhooked refers to this plugin show the total implementations? (hence unhooked and not hooked) ?

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.

There's a couple of changes snuck in there that just need to be checked first.


class unhooked_system_calls(interfaces.plugins.PluginInterface):
"""Looks for signs of Skeleton Key malware"""
class UnhookedSystemCalls(
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't have the same name, meaning people trying to run unhooked_system_calls.unhooked_system_calls will fail. Since the point of this is to provide continuity, please don't change the plugin name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ikelos I get the following error when I do that:

Issue in module volatility3.plugins.windows.unhooked_system_calls: 
line 6, col 0: Direct import of unhooked_system_calls (<class 'abc.ABCMeta'>) 
from module volatility3.plugins.windows.malware - 
change to 'from volatility3.plugins.windows import malware 
and using malware.unhooked_system_calls
Found 1 issues

Also, by continuity you mean for 3rd party tools that may use the framework right? because in the framework none is using it this way.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean other people using the library and us not pulling the rug out from under them by changing things without telling them. It's a small point, next to no one uses the plugin class names, but still, we definitely can't change what the old one was called, because then we're just removing it with no warning...

vollog = logging.getLogger(__name__)


class UnhookedSystemCalls(interfaces.plugins.PluginInterface):
Copy link
Member

Choose a reason for hiding this comment

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

This is a change in plugin name, I'll ask @atcuno if he's happy with the name change. I don't mind either way (and mostly I don't expect people to type it, given it contains capital letters, and volatility will let you get away with the module name) but I want to check before making a change like this.

I understand python classes typically start with a capital letter, and I'm not sure how we diverged from that, but it seems we have in places and I agree that if we want to change it, doing so during a plugin rename is probably the right time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I did it since it couldnt tell the difference between the module name and class name when I tried to call import it and then use it

@ikelos ikelos requested a review from atcuno June 7, 2025 17:17
@atcuno
Copy link
Contributor

atcuno commented Jun 9, 2025

btw why is this called unhooked_system_calls? these ntdll.dll stub functions are basically user-mode wrapper for the actual syscall which code is executed in kernel (hence its not hooking SSDT, or ntoskrnl syscall implementations), also unhooked refers to this plugin show the total implementations? (hence unhooked and not hooked) ?

The hooking is done by EDRs to monitor the flow. See my talk at DEF CON last year:

https://www.youtube.com/watch?v=PmqvBe1LSZc&t=7s

@atcuno
Copy link
Contributor

atcuno commented Jun 9, 2025

Related to this .malware. push, @SolitudePy if I create a ticket with the list of other "malware" plugins will you complete the set? For example on Windows:

  • debugregisters.py
  • direct_system_calls.py
  • driverirp.py
  • drivermodule.py
  • etwpatch.py
  • hollowprocesses.py
  • ldrmodules.py
  • orphan_kernel_threads.py
  • processghosting.py
  • suspended_threads.py
  • suspicious_threads.py

There aren't that many plugins that aren't capable of showing malware in some way, so if we are going to categorize this way then we need to move them all.

@SolitudePy
Copy link
Contributor Author

Related to this .malware. push, @SolitudePy if I create a ticket with the list of other "malware" plugins will you complete the set? For example on Windows:

  • debugregisters.py
  • direct_system_calls.py
  • driverirp.py
  • drivermodule.py
  • etwpatch.py
  • hollowprocesses.py
  • ldrmodules.py
  • orphan_kernel_threads.py
  • processghosting.py
  • suspended_threads.py
  • suspicious_threads.py

There aren't that many plugins that aren't capable of showing malware in some way, so if we are going to categorize this way then we need to move them all.

well I have already have many other open PRs for this (including linux ones), havent finished yet, some plugins I saw less of a malware plugin since its not direct like suspicious_, malfind. E.g im not sure to categorize suspended_threads since its not a certain logic but like an indicator, btw about etwpatch if you could check my PR on it #1818

@ikelos
Copy link
Member

ikelos commented Jun 10, 2025

There aren't that many plugins that aren't capable of showing malware in some way, so if we are going to categorize this way then we need to move them all.

The idea is not to move all the plugins cluttering the os namespaces into their own malware section just because they can be used to detect malware. That's just shifting the problem from one bin to another. Ideally the category would be for plugins whose only or main purpose is for spotting malware. Anything that just provides information about a sample (that happens to be used for malware or could be used for malware) should stay part of the main group. For example, the IRP stuff seems like it's just about drivers, rather than malware in particular. Same for debugregisters. Hollowprocesses seems like a facet specifically used for/malware primarily.

@SolitudePy I am going to get through the linux plugins too, it's just a matter of time I'm afraid.

@ikelos
Copy link
Member

ikelos commented Jun 10, 2025

@atcuno If you could draw up a list of things that are specifically or primarily only used for malware, that would help us narrow some stuff down. If we've already moved some over, I'm happy reverting the changes, really just want to get the main os category dumping grounds for plugins a little better ordered, so people can find things they're interested in.

@atcuno
Copy link
Contributor

atcuno commented Jun 10, 2025

I can make a list, but:

  1. suspicious_threads only prints out threads that are suspicious, otherwise the output is empty.. its 100% for malware

  2. suspend_threads are only interesting in an analysis context for looking for malware and was written as part of the DEF CON talk for detecting EDR evading malware

@ikelos
Copy link
Member

ikelos commented Jun 10, 2025

That's fine, put them in the malware list, just don't want everything ending up in there. Technically pslist can be used to find malware, but that's not it's primary purpose. Happy with whatever you choose, as long as the kitchen sink's not in there... 5;)

@SolitudePy
Copy link
Contributor Author

What is the status of this PR, do I need to change anything?

@ikelos
Copy link
Member

ikelos commented Jun 10, 2025

Nope, it looks like you resolved the comments I put in place, I just need to recheck it.

@ikelos ikelos self-requested a review June 10, 2025 19:54
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.

Looks good now, thanks 5:)

@ikelos ikelos merged commit f2f83f5 into volatilityfoundation:develop Jun 10, 2025
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.

3 participants