-
Notifications
You must be signed in to change notification settings - Fork 457
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
New linux plugin: modxview #1330
base: develop
Are you sure you want to change the base?
New linux plugin: modxview #1330
Conversation
@Abyss-W4tcher it looks good to me.
|
Alright, I thought about a Anyway, the code can indeed be shared somewhere (maybe LinuxUtilities), while keeping the convenient and self-contained APIs inside the Putting the taint flags in other plugins outputs should be en easy task, maybe in a 3-for-1 PR if that's ok for ikelos. |
@@ -307,6 +378,17 @@ def section_strtab(self): | |||
return self.strtab | |||
raise AttributeError("module -> strtab: Unable to get strtab") | |||
|
|||
@property | |||
def taints_value(self) -> int: | |||
return self.taints |
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.
Not sure about having a property like this if the member was consistently named taints. Is there a particular reason for 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.
Mostly type hinting, which allows to clearly differentiates properties, to separate between the taints value/taints string/taints parsed.
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 double-checked this and taints was added to struct module in 2.6.19 here -> torvalds/linux@2bc2d61 .. so we are good to go.
Would you mind adding a comment with this information please?
# kernels >=2.6.19 2bc2d61a9638dab670d8361e928d1a5a291173ef
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 not sure polluting the namespace to get better typing information is useful? It now offers people two options for the samething. Either taints
shouldn't be public (_taints
) or this property shouldn't exist (untill it does something different than returning just a plain member).
Sure, this is ultimately up to @ikelos, but I personally lean toward the Unix philosophy: small, specialized tools, each dedicated to a single purpose. This approach doesn't mean each plugin must detect malicious behavior on its own. Then, we can have let's say 'macro' plugins, like the one in this PR, that aggregates various indicators to help identify anomalies. This way, the small plugins can be reused in other "macro" plugins. The kernel tainted flags plugin code will be minimal since we will reuse the code you have already written for modules, but the insights it could provide would be highly valuable and unique. For instance:
The output could be just in a single line, or we could opt for a more detailed, explanatory format such as:
|
Yeah, this is one is for @ikelos. IMO if it's just one function it could be in LinuxUtilities. Alternatively, if there are more related functions, I think it could be more convenient to have a separated class containing that subsystem API .. like https://github.com/volatilityfoundation/volatility3/pull/1332/files#diff-8456da6d20fc84f0d63dedc5bc816ffde418ff41c83b1828da7c614252bda150R840 with its own version, but let's see if ikelos agrees with this idea, since that PR is still pending review. On a related note, I think it would be a good idea to reorganize LinuxUtilities in the future, grouping functions by subsystems such as mount, modules, etc. and only keep in LinuxUtilities the general or framework helpers like
Correct, and call the LinuxUtilities or modules API function with |
Alright, let's wait for ikelos to give its point of view on all these comments, then I'll unify the API depending on where we want to put it. The Globally, it sounds really good to me :) |
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.
Generally looks really nice, next to no comments (which is great work, you're making it harder and harder for me to nit-pick!) 5:P Sadly, the ugly need for version numbers raises its head, so just get that bumped appropriately and sort out the other couple of bits and it should be good to go... 5:)
|
||
return taints_string | ||
|
||
def get_taints_as_plain_string(self) -> str: |
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.
These are externally visible additions to the API, which means a MINOR version number somewhere, needs to go up. I suspect it may be the framework itself because I don't think anything further down is versioned?
- module_flags_taint kernel function | ||
""" | ||
comprehensive_taints = [] | ||
for c in self.get_taints_as_plain_string(): |
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 don't use single letters in loops. You only have to type it once, but character
will just make it a little easier for readers of the code for years to come...
@@ -307,6 +378,17 @@ def section_strtab(self): | |||
return self.strtab | |||
raise AttributeError("module -> strtab: Unable to get strtab") | |||
|
|||
@property | |||
def taints_value(self) -> int: | |||
return self.taints |
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 not sure polluting the namespace to get better typing information is useful? It now offers people two options for the samething. Either taints
shouldn't be public (_taints
) or this property shouldn't exist (untill it does something different than returning just a plain member).
Hello,
This new plugin centralizes the module detection capabilities, and allows to quickly spot and list all the modules. This way, it is easier to detect copycat malicious modules, trying to mimic kernel modules names (ex: https://attack.mitre.org/techniques/T1036/006/). To keep the name familiar for long-term users, it follows the
psxview
naming convention.In addition, this PR introduces the capability to parse taint flags, which embeds additional data to help debug a kernel or a module (as it is in the end, quite similar).
Here are some sample (stripped) outputs :
"/proc/modules" hidden module :
"/proc/modules" hidden module (plain taints string) :
Hidden module :
Note: kovid cleans out the taints attributes, but the UNSIGNED_MODULE one will be there nonetheless. This is still a great piece of information to spot LKM modules.
Happy to read your reviews about this !