Add APIs and initial plugins for GUI support and fit Windows major APIs to current design flow #1640
Conversation
There was a problem hiding this comment.
First off, whaaay too big a PR. This had additional plugins, it had sweeping changes across the entire base (all of which ended up needing tweaking), there was a small bugfix snuck in. I'm gonna start rejecting these if they're not cut up into their appropriate components. As it is, I can't guarantee I've caught everything, and you guys now have 84 comments to go through...
Technically all of the touched plugins should have a PATCH version bump, but I'm happy to overlook that.
There's a lot of places where keyword arguments were swapped out in favour of positional arguments? I don't think there's any benefit of that, it just provides less information for people trying to read the source code as to what's going on. It would make changing names easier in the future, but I'd actually prefer we think through and choose the right ones first time, rather than removing them so that we could mess with them in the future...
This also goes to a lot of effort to replace the more generic newer VersionRequirement, with the older overly specific alias of PluginRequirement. We should be going in the other direction please? Happy to discuss it if you feel otherwise, but PluginRequirement is literallly a facade over VersionRequirement (which can be used for more than plugins), so you'll need a good argument. (See issue #1644).
Because this was such a sweeping change, there's at least two plugins that will still need a further pass (timers and KPCRs) because there's some daft stuff that got in the tree somehow where some methods still take a separate layer name and symbol table without clear reason for why (and I think there's one that takes a kernel_module and a layer_name and a kernel_symbol_table*).
So some definite changes here. On the whole I approve of the changes being made and the effort to smarten up the codebase. Big changes like this are always going to be hard to make across so many existing plugins (and noting that will affect other plugins that make use of them), but that's why this PR should probably have been several PRs:
- Only Changing parameters
- Adding new plugins
- Bugfix
Please do feel free to submit example PRs (in draft if you want to differentiate them) that can break tests, but at least show what you're intending to do once, before applying it hundreds of times across the codebase, only to need all those times tweaking. I'd far sooner we checked (and allow tests to fail during development) than create a monstrosity like this where bugs will slip through simply because there's too much to check in one go...
|
Thanks for the feedback. Even though its 80 comments most are easy fixes like the function parameter explicit names in calls. Check slack links after reading this. I left comments on everything that did not require code changes as I have to leave for airport. Overall, I agree with these:
My two pushback points.
|
|
Yep, all looks good, but could you please go back and everywhere you commented with "it'll be fixed in a future PR" could you file an issue explaining exactly what the PR will cover, and add it to the comment? That'll help make sure that things which need doing don't get lost, you'll be able to write out how you want to solve it and what the solution should be and when the time comes it can be auto-closed by a PR tagged against it. Just useful housekeeping. Yeah, I think I was grumpy because the PR was big and killed several hours (started between 12 and 2am and then finished it between 9 and 10) just reviewing the same kinds of changes with a few random extras thrown in that meant I had to pay attention! 5;P There was a bit where you added a whole new function but with a signature that was similar to the old one, and it seemed odd to push new code that will definitely need revisiting and fixing up when it could've been added here in a fixed state, but otherwise I just want to make sure that our future desires get documented so they don't get lost or forgotten. I think what I meant was, split up the PRs into smaller bits so that a huge change doesn't get replicated and then need fixing everywhere? A draft PR can be used to demonstrate the intended fix without needing to pass all the (now extensive and greatly appreciated) test suite. I just want to make sure that we don't now all-or-nothing PRs because we're afraid of it failing tests. It likely will never get committed if it fails, but it's still a good medium to review intended code changes, and I don't want us to be afraid of that. It's much easier to have review and have a conversation in a PR than randomly on branch code. I just... you've just gotta give me smaller/more consistent PRs to work on please! 5:P Beasts like that are just too much for me in one go... |
|
Agreed! We don't want PRs like this again either :| I just went through and made new tickets for all the new/extra efforts discussed here and then resolved the comment thread with the link. David is working on updates now to knock out the bulk of these at once (like adding the parameters), which should get us pretty close to merge ready. |
This adds a new classmethod, `list_processes_from_kernel`, updates the `list_processes` method signature to use only the kernel module name and the context instead of splitting information about the kernel between the layer_name and symbol_table_name paramters, and does a major version number increase on the plugin. Also updates the documentation to reflect pslist method signature change. Co-authored-by: Andrew Case <andrew@dfir.org>
This cleans up the APIs for some methods in the modscan/modules plugins that currently take separate symbol_table_name and layer_name parameters, when it really makes more sense to just pass in the context and the kernel module name. It also updates the pslist plugin requirement version, and uses the updated method signatures. Co-authored-by: Andrew Case <andrew@dfir.org>
This simplifies the `build_module_collection` signature to take a single `kernel_module_name` parameter instead of `layer_name` and `symbol_table_name` parameters, both of which would only ever belong to the kernel anyway. This will prevent future confusion for consumers of this method. Co-authored-by: Andrew Case <andrew@dfir.org>
Updates all plugins that use the `ssdt` module with correct parameters and updated version requirement values.
Co-authored-by: Andrew Case <andrew@dfir.org>
This updates all plugins that depend on windows.hivelist.HiveList to use the updated method signature, and bumps their dependency version accordingly. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates the Windows Amcache plugin to use the latest changes in Windows HiveList. It requires a major version bump of its own due to a breaking method signature change, and is therefore in its own commit. Co-authored-by: Andrew Case <andrew@dfir.org>
dgmcdona
force-pushed
the
plugin/windows_gui
branch
from
ce27159 to
083d35e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dgmcdona
force-pushed
the
plugin/windows_gui
branch
8 times, most recently
from
2aedcbf to
e5ab4aa
Compare
This updates the ssdt, truecrypt, drivermodule, driverirp, and verinfo plugins to use the new method signatures from the Modules plugin, and updates the requirement version numbers as well. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates the PESymbols plugin to use the latest changes from pslist and modules. This required breaking interface changes of its own (altered method signatures) and so receives its own major version bump. Dependents of PESymbols that _don't_ require any breaking changes have their method calls and version requirement numbers updated here as well. Co-authored-by: Andrew Case <andrew@dfir.org>
This bumps the requirement version number for pe_symbols, and uses the latest method signature. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates the windows.threads plugin with a method signature change: `module_name` is now `kernel_module_name` for clarity. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates the debugregisters and suspended threads plugins' threads requirement with the latest major version bump. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates the windows.consoles.Consoles plugin to use the updated hivelist method signature, changing one of its own method signatures as required and doing a major version bump of its own. Plugins that depend on consoles also have their method calls changed, and their dependency versions bumped. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates all dependents on windows.pslist.PsList that could be updated without breaking interface changes of their own to use the latest windows.pslist.PsList plugin version with a simplified method signature Co-authored-by: Andrew Case <andrew@dfir.org>
This updates PEDump to use the latest version of PsList, updates the dependency version number, changes one of it's own method signatures to facilitate the pslist change, and does a major bump of its own version number. Co-authored-by: Andrew Case <andrew@dfir.org>
This updates all plugins that both depend on PEDump and won't require breaking changes of their own. They now call the updated method signature, and have their pedump requirement version bumped appropriately. Co-authored-by: Andrew Case <andrew@dfir.org>
This fixes an `AttributeError` that can crop up when the actionset's context is `None`.
This updates the Windows pool extension with a new get_name() method, and changes the method signature for another. We'll need to figure out if there is a good way to version extension classes. Deskscan requires this in order to use get_name with an alternate (non-kernel) symbol table.
This adds the windowstations, desktops, and deskscan plugins, and removes the gui.py plugin file that was stubbed out in the introductory work for the effort. It also adds a new method to the poolscanner class, but does not bump the poolscanner version number since there is already a major version number bump going into this PR. Co-authored-by: Andrew Case <andrew@dfir.org>
code review fix
Change to self.current_kernel_name so people can change this value.
Also adds some keywords in method call args, and fixes an incorrect type-hint.
Makes it explicit that these are symbol table names, not actual symbol tables.
This updates a whole host of method calls to pass keyword arguments instead of positional arguments.
Code Review: PluginRequirement -> Version Requirement
Also underscores an unused unpacked tuple value
There are a lot of things going into this so we're doing a minor framework version bump.
dgmcdona
force-pushed
the
plugin/windows_gui
branch
from
ded055a to
a553ee3
Compare
|
@ikelos I addressed all of the comments and fixed up all of the arguments where they should be passed as keyword arguments, so I think this is good to go |
bumping this just in case it got lost in all of the force-pushed commits |
This PR accomplishes the final major API additions and overhauls needed before we can get to the parity release. This was a collaboration with myself and @dgmcdona. With these changes, only the new GUI-related windows, messagehooks, and rawinputdevicemonitors plugins are remaining before parity. These will be short and build off the APIs added in this PR.
From a high level, this PR covers:
Changing the major Windows APIs (listing processes, modules, and registry hives) to fit our current design flow (sending in the kernel module name instead of different pieces), which also matches Linux and fixes a painful amount of bugs that we found while getting the GUI API developed.
Adding the GUI API, which required adding the ability to pool scan across different layers and types, as well as the GUI-specific APIs and building blocks. This also led to fixing many bugs along the way while getting it all working.
Adding the initial GUI plugin (windowstations) and then the next logical GUI step (desktops of Windows) through both re-using the workstation scanning (desktops plugin) as well as direct desktop scanning (deskscan). Deskscan shows how the remaining GUI plugins will re-use the GUI-specific scanning infrastructure. Desktops shows how the new APIs can be integrated easily into future GUI work outside the core team.
All these changes were tested through multiple runs against the full Windows sample set (411 samples covering XP-11 and 32/64 bit).
The deeper details:
The three core Windows APIs (list_processes, list_modules, hivelist) all used the flawed/old way of sending parameters to reference the kernel symbol table and layer. This led to all the follow on plugins also following this pattern, which led to a significant amount of bugs as plugins and APIs would 1) confuse layers and tables between the kernel and processes, and 2) these issues was extremely hard to debug as the code used crappy names like
layer_name, which is ambiguous in code paths dealing with multiple layers, and names likesymbol_tablewhich were actually the name of the table and not the table itself. Allsymbol_tablenamed variables in the affected code paths were changed tosymbol_table_name. With these API changes, code must be 100% explicit when it wants to reference the kernel, and being able to reference kernel.layer_name is much harder to break then accidently sending in the wrong (badly named) variable.To remedy this overall situation with the least amount of changes for one PR, David (@dgmcdona) and I changed the three core APIs to take in only the kernel module name, which then led to the downstream using APIs doing the same. We only touched the APIs directly needed to make the GUI code work, not other APIs buried in different plugins. Given that this encompassed the core 3 APIs (processes, modules, hives), we are quite certain we will prevent these bad patterns from showing up in future plugins. These changes makes, like Linux, to where only functions that need the kernel object construct it, instead of it being passed around everywhere. This immediately called out several bugs in existing APIs.
The effects of these changes were pretty minimal in most plugins as they would call list_processes or list_modules from
runor_generator. Only a few plugins needed major changes to accommodate this, which we fixed.We then went through and changed all needed requirement dependencies versions as well as bumped versions in plugins that changed their API signatures. With these changes in place, all the new GUI APIs follow the convention correctly from the start to avoid future overhauls.
As mentioned before, the branch in its current state (and a few previous ones) were tested against all 411 Windows samples in our internal set. In the final run, the only backtraces triggered were unrelated to our changes, and I made separate tickets for them with the parity label added.
Fixes #1504 #1636