-
Notifications
You must be signed in to change notification settings - Fork 484
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: linux.tracing.ftrace #1564
base: develop
Are you sure you want to change the base?
New Linux plugin: linux.tracing.ftrace #1564
Conversation
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.
Looking pretty reasonable. Just a couple possible improvements and if we're adding in a new modules utility, it might be good to start shifting associated functionality out of LinuxUtilities
if possible. I haven't figured out how to move stuff out of the module class, so that still needs some thinking about, but maybe just duplicating functionality in a versionable module, and then a deprecation warning in the module method? That can be for another time...
I made all the changes in a single commit, as it should be easier for you to review the requested changes. There were a few tweaks too, like an I'll definitely look into writing unit tests for the modules plugins once the infrastructure is up, to prevent missing these tricky errors in the future. |
Thanks for making all the requested changes. I'm still a little concerned about the Module standing out with just that one function. I know you said you'd include them in a different PR, but I'd almost like to get that PR in first so I can see what it'll look like and if it's the best way of doing it. I think the class name and the module name should probably match, because it'll be a real pain to include different classes in the same file (the file will import, but an exception will get thrown when the non-existant class is accessed as part of the requirement, so if anything it'll break less cleanly). I'm also not sure the main project version bump is needed because plugins that can't import the file will just fail during the import phase, and they'll do that before they can check the main project version number anyway, so depending on the higher version number doesn't gain anything. |
This is ready for a review after the merge of #1567. |
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, a small optimization, and also I'd like to consider whether using a volatility enumeration would be better/easier? If there isn't support for multiple flags (I don't remember) it's the kind of thing we can add because it would be useful, but given we've got a big and powerful parser that integrates with everything else, I'm keen to make use of it rather than rolling our own in python...
Based on https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/ftrace.h#L255. | ||
""" | ||
|
||
FTRACE_OPS_FL_ENABLED = auto() |
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.
Would this not be better as a volatility enum, then the values could be stored in JSON as data rather than in code? It would mean adding an Ftrace symbol table, but then it should just be possible to cast ftrace_ops.flags as the enum? That might also then get around the python <= 3.10 issue you were seeing?
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 against the idea, both should work well. However IntEnum
provides really useful APIs out of the box, and I don't have to do a manual list comprehension and mask the value. The little string manipulation I did was to prettify the output because we already use "|" as a column separator.
I think JSON is more interesting when we have a lot of constants or nested structs to declare.
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.
Well, I'd like to improve our enum support to the point it becomes as easy to use as IntEnum? If you could tell me what APIs it's missing, we could add them and give people more reason to use 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.
The use of an IntEnum
does not require to specifically make a JSON dict with hardcoded powers of 2, and I think the readability offered by auto()
is quite nice.
Nevertheless, I still looked on how we could add an easy int masking API (or leverage the one in https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/symbols/wrappers.py) to enums.
I noticed that objects returned by my_symbol_space.get_enumeration("XX")
aren't of type Enumeration
, but volatility3.framework.objects.templates.ObjectTemplate
I think I was able to understand the calling tree:
-
get_enumeration
instantiates anObjectTemplate
:
volatility3/volatility3/framework/symbols/intermed.py
Lines 543 to 548 in ad90804
return objects.templates.ObjectTemplate( type_name=self.name + constants.BANG + enum_name, object_class=objects.Enumeration, base_type=base_type, choices=curdict["constants"], ) -
No inherited properties from
Enumeration
, but itsVolTemplateProxy
:
volatility3/volatility3/framework/objects/templates.py
Lines 25 to 45 in ad90804
def __init__( | |
self, | |
object_class: Type[interfaces.objects.ObjectInterface], | |
type_name: str, | |
**arguments, | |
) -> None: | |
arguments["object_class"] = object_class | |
super().__init__(type_name=type_name, **arguments) | |
proxy_cls = self.vol.object_class.VolTemplateProxy | |
for method_name in proxy_cls._methods: | |
setattr( | |
self, | |
method_name, | |
functools.partial(getattr(proxy_cls, method_name), self), | |
) | |
@property | |
def size(self) -> int: | |
"""Returns the children of the templated object (see :class:`~volatilit | |
y.framework.interfaces.objects.ObjectInterface.VolTemplateProxy`)""" |
volatility3/volatility3/framework/objects/__init__.py
Lines 640 to 642 in ad90804
class VolTemplateProxy(interfaces.objects.ObjectInterface.VolTemplateProxy): | |
_methods = ["lookup"] | |
>>> dir(kernel.get_enumeration("XX"))
['__call__', '__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattr__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_vol', 'child_template', 'children', 'clone', 'has_member', 'lookup', 'relative_child_offset', 'replace_child', 'size', 'update_vol', 'vol']
No description
, is_valid_choice
etc. available, which also makes it difficult to add new methods. As opposed to a symbol for example, which value can change depending on the memory sample, isn't the enum a simple static definition from the ISF ? Why is the constructed enum limited ?
I may have missed something obvious, but right now I am not sure if this is intended or if there is another way ?
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.
So get_enumeration
would be the equivalent of get_type
. If you wanted to apply it to some data, you'd use context.Object
to create, just as you would a type. Then you'll get a bunch of functions that should let you interrogate it. If there's mirror methods you think we can expose on the template because they don't require the data, I'm happy to add those in too. The point is, the enumeration type won't improve unless we have cases where we try to use it, so even if we do end up going with a normal IntEnum, I'd like to improve the enumeration API so that it's similarly easy to use to do the things we want to be able to do with it.
Auto()
actually scares me, because an accidental addition (or a change of the upstream code) and all the autos fall out of place. That's one of the reasons I'd prefer having the actual data encoded into it, because then you can see exactly what you're getting, and other people can use the data themselves. The fact that it numbers them for you helps for precisely one time, so not as important as the thousands of time the enum is going to be used by everybody 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.
It looks as though I have mirrored lookup, which I think allows you to pass in a value and get the identifier for it back...
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.
Enumeration
objects are designed to operate in a context
, technically we could do ftrace_ops.flags.cast("ftrace_symbol_table!ftrace_flags")
. Still, for this use case I'm not sure it is worth the overhead of a custom ISF loading.
I think we'll have to do live enumeration parsing for later tainting
plugins, I'll work on a way to parse the flags more intelligibly, if that is ok for you ? It should be easier to design APIs with a concrete use case.
Happy to change auto()
values to explicit 1 << n
values, thanks for raising this concern !
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, fair enough. I am interested in getting our enum implementation up to the same ease-of-use as the built-in ones (or better!), but it might be a bit more overhead for this example now.
I would prefer the auto()
values be changed to explicit values though, thanks.
Hopefully, all issues have been addressed now. 👍 |
Hello, I think it is ready to go in ? No hurries of course if this is not on your schedule. |
Can you prod me again next week please? I think we've done all the jiggery-pokery we'd intended, but I just want to get to next week to let things settle if that's ok... |
Hi :),
This new plugin adds detection of malicious kernel functions hooking (see #1286), by inspecting the
Ftrace
(Function Tracer) infrastructure. Attackers can leverage its capabilities to hook critical low level calls, and perform man-in-the middle or output manipulation.It also relies on the recent
linux.hidden_modules
andlinux.modxview
to extend the uncovering of callbacks parents/modules.Documentation:
Superseeds https://github.com/Abyss-W4tcher/volatility-scripts/blob/master/Volatility_contest_2023/plugins/check_ftrace.py
Hopefully, this does not interfere with the parity release ?
Versioning
Minor framework bumps:
- add framework.symbols.linux.utilities.modules.Modules API