process_spoofing plugin #1826
process_spoofing plugin #1826
Conversation
|
SolitudePy
force-pushed
the
linux_procspoof
branch
from
68b51e8 to
5e654b1
Compare
There was a problem hiding this comment.
Thanks for your submission and sorry it took so long to find time to review, this plugin looks fun! It's mostly fine, but it smushes the results together into a string, which is longwinded for humans to read and difficult for programs/plugins to parse. Hopefully this should be pretty straight forward to refactor into returning boolean values. It also feels like as number of the methods here may be useful for other plugins, so first check that other plugins don't already implement them and otherwise consider converting them into classmethods so that they can be called externally...
| ) | ||
|
|
||
| except Exception as e: | ||
| vollog.debug(f"Error processing task at {task.vol.offset:#x}: {e}") |
There was a problem hiding this comment.
It might be better to make this a vollog.warning, since otherwise this won't show up for most users? It's also not all that useful to user? It's generally better to catch specific exceptions you're expecting rather than just a catchall "something went wrong" because it makes it very difficult for anyone to diagnose what went wrong (and it prevents volatility's general exception handlers from being able to step in and give the user a nice message, or a developer a full traceback of the exception).
| ), | ||
| ] | ||
|
|
||
| def _get_executable_path( |
There was a problem hiding this comment.
If these functions would be useful externally by other plugins, consider making them public classmethods (using @classmethod and not starting the method name with an _)? It looks like they only really need self to get a context, so if you add the context as the first method parameter (in keeping with all our other plugins/classmethods) then it would make them usable elsewhere.
| if not exe_file or not exe_file.is_readable(): | ||
| return None | ||
|
|
||
| exe_inode = exe_file.dereference().f_path.dentry.d_inode |
There was a problem hiding this comment.
This shouldn't require explicit dereferencing. Requesting a specific member (like f_path) from a pointer will automatically dereference it and return the value from the dereferenced structure. Including it explicitly perpetuates the idea that it's necessary and could lead to misunderstanding of how pointers work in volatility.
| try: | ||
| mm = task.mm | ||
| if not mm or not mm.is_readable(): | ||
| return renderers.NotAvailableValue() |
There was a problem hiding this comment.
The renderer values are part of the UI, and generally not all that useful to plugins wanting to process the data. You return None almost everywhere else apart from here, it would be better to be consistent in your return values unless you're explicitly trying to differentiate something being NotAvailable over some other AbsentValue option. Typically, we'd pass None through, and then just as it comes out of the generator, convert Nones to the responses that makes the most sense for a None for that field.
| return None | ||
|
|
||
| try: | ||
| argv = proc_layer.read(start, size_to_read) |
There was a problem hiding this comment.
I'd check to see if existing argument handling isn't done somewhere else (either in another plugin or a task structure). It seems like it might be more useful exposed that way, just to avoid people rewriting it themselves each time and possibly making slight parsing differences each time.
| else: | ||
| return None | ||
|
|
||
| except (exceptions.InvalidAddressException, AttributeError): |
There was a problem hiding this comment.
There's a lot of generic error catching without any notice to a developer that some possible output got swallowed or why.
| 1 for name in [exe_basename, cmdline_basename, comm] if name | ||
| ) | ||
|
|
||
| is_deleted = exe_basename.endswith(self.deleted) |
There was a problem hiding this comment.
This isn't a good way of identifying this, it should be a separate boolean flag handed around. Stuffing it on the end of a string and then parsing it back out again feels very brittle. I'd make each of your three methods return a tuple (basename, deleted) and then process those, the other two methods can return None but you really should be loading up the basename, and certainly not at the end, because then you can't tell the difference between random file that's been deleted and random file (deleted), within your code the two are indistinguishable.
| notes.append(f"'Potential Process image deletion: exe_file={exe_basename}'") | ||
| exe_basename = exe_basename[: len(self.deleted) * -1] | ||
|
|
||
| if available_sources < 2: |
There was a problem hiding this comment.
Surely you'd want to highlight that the file was only present in one source? Seems like a sure fire sign that something unusual happened?
| return None | ||
|
|
||
| if exe_basename != cmdline_basename: | ||
| notes.append( |
There was a problem hiding this comment.
This combines data fields, so creating a list of strings is not going to be easy for a volatility plugin to process (and, gets collapsed into a single string, which would need parsing again to be used by another program/a different tool). This should probably be returning the three compared values, and then a boolean to say which tests succeeded and which failed. Humans could read that fine (it would be less text to read) and other programs/plugins could parse the data easily.
|
|
||
| exe_basename, cmdline_basename, comm = self._extract_process_names(task) | ||
|
|
||
| notes = self._detect_spoofing(exe_basename, cmdline_basename, comm) |
There was a problem hiding this comment.
As mentioned above, this is a bad design pattern. Keep the tests separate and display them each in their own column as a true/false value.
Hello, just playing with memory & OS internals.
apparently some legitimate processes do these techniques to have enriched information in their cmdline or so. here are some such processes: