Linux netfilter hooks plugin #1080
Conversation
|
Hey @ikelos/@atcuno this is still awaiting a review 🙏 |
|
@ikelos this one has been tested quite a bit and is good from my end. It is a powerful plugin for rootkit detection. |
| ): | ||
| self._context = context | ||
| self._config = config | ||
| symbol_table = self._config["kernel"] |
There was a problem hiding this comment.
symbol_table is a bit of an odd name, but not a show stopper. 5;P
| def run_all( | ||
| cls, | ||
| context: interfaces.context.ContextInterface, | ||
| config: interfaces.configuration.HierarchicalDict, |
There was a problem hiding this comment.
It looks like config['kernel'] is the only time this is ever used? I'd be much happier passing through a module_name than a whole hierarchical dict? Was the expectation this might need extending or is there another good reason to pass config through?
|
|
||
| from typing import Iterator, List, Tuple | ||
| from volatility3.framework import ( | ||
| class_subclasses, |
There was a problem hiding this comment.
This is a function, not a module. It would be better (and is somewhat the coding standard in volatility) to import volatility.framework and then call framework.class_subclasses to avoid people accidentally thinking it's defined here and then importing it from here.
| self.layer_name = self.vmlinux.layer_name | ||
|
|
||
| modules = lsmod.Lsmod.list_modules(context, symbol_table) | ||
| self.handlers = linux.LinuxUtilities.generate_kernel_handler_info( |
There was a problem hiding this comment.
I believe LinuxUtilities is also versioned, so should be one of the plugin's requirements, a VersionRequirement please (just to make sure the API won't change out from under you).
| self._set_data_sizes() | ||
|
|
||
| def _set_data_sizes(self): | ||
| self.ptr_size = self.vmlinux.get_type("pointer").size |
There was a problem hiding this comment.
This isn't a show stopper because this gets called as part of the __init__ but in general all class variables should be defined only and directly in the initializer, to avoid accesses before they've been set.
| vollog.error("Unsupported Netfilter kernel implementation") | ||
|
|
||
| def _run(self) -> Iterator[Tuple[int, str, str, int, int, str, bool]]: | ||
| """Iterates over namespaces and protocols, executing various callbacks that |
There was a problem hiding this comment.
Really good docstrings! Thanks! 5:D
| # the INET protocol in the kernel. AFAIU, this is used as | ||
| # 'NFPROTO_INET = NFPROTO_IPV4 || NFPROTO_IPV6' | ||
| # in other parts of the kernel source code. | ||
| return ("IPV4", "ARP", "BRIDGE", "IPV6", "DECNET") |
There was a problem hiding this comment.
Feels like this should be defined as a constant at the top of the module? Any reason this list is different?
| """Returns the data structure used in a specific kernel implementation to store | ||
| the hooks for a respective namespace and protocol. | ||
|
|
||
| Except for kernels < 4.3, all the implementations use network namespaces. |
There was a problem hiding this comment.
Did I mention how awesome your docstrings are! 5:D
| return self.vmlinux.symbol_table_name + constants.BANG + symbol_basename | ||
|
|
||
| @staticmethod | ||
| def get_member_type( |
There was a problem hiding this comment.
This feels a little black magicy... You're definitely playing around with volatility internals here. My concern is if there's a change and we add another object with a subtype or something. I can see it gets used a lot and looks useful, but I worry about how robust it is. If you're happy with it, I'll go for it, it's just got my spidey sense tingling a little... 5;)
It supports the following protocols: INET, IPv4, IPv6, ARP, NETDEV (ingress and egress hooks), BRIDGE and DECNET.
Supported Netfilter hooks implementations:
Supported NetDev ingress hooks implementation
Supported NetDev egress hooks implementation
Example:
See the full test case suite output:
vol3_linux_netfilter_output.txt