Feat: Migrate windows idt plug-in to volatility 3 #976
Conversation
There was a problem hiding this comment.
Thanks very much for your submission!
This is a good first attempt, but there's a couple of minors points and a major shift in the way it operates which I'd strongly recommend. Where you've constructed your own objects (such as KPCR, etc) please consider instead defining a JSON ISF file, and defining class_types to override the standard struct class for any calculations/convenience methods that the objects should have.
You can find more information at:
https://volatility3.readthedocs.io/en/stable/complex-plugin.html#writing-using-intermediate-symbol-format-files
https://volatility3.readthedocs.io/en/stable/complex-plugin.html#writing-new-templates-and-objects
Or please ask on the slack channel #vol3-dev for help if you need it. 5:)
|
|
||
| # Get kpcr object | ||
| kpcr_offset = ntkrnlmp.get_type("_KPCR").relative_child_offset("PrcbData") | ||
| kpcr = ntkrnlmp.object( |
There was a problem hiding this comment.
It seems like you're trying to wrap the KPCR data into a custom object, rather than using a volatility object? Can I please strongly recommend that you consider defining a JSON file with the appropriate types for a _KPCR if the standard ones aren't suitable? You can even define an override class if there are specific methods or calculations you require (such as idt_entries or gdt_entries). This should reduce the complexity of the code and turn store more information, which should be data, as data rather than embedding it into code.
I don't recall whether it was the KPCR or the KD_DEBUGGER_DATA that started getting encrypted by Microsoft, but if that is the case for the KPCR, we may want a more generic/general means to handle it, so all the code can live in a central location in the core.
There was a problem hiding this comment.
It was KD_DEBUGGER_DATA that gets encrypted now. AFAIK the _KPCR types are included in the IST files from ntoskrnl, so they're already available.
|
|
||
| for cpu_index in range(cpu_count): | ||
| # Calculate the address of KiProcessorBlock | ||
| KiProcessorBlock_addr = ntkrnlmp.get_symbol("KiProcessorBlock").address + cpu_index * 4 |
There was a problem hiding this comment.
This seems to be manually iterating an array of pointers, but it would probably be better to just instantiate an object on top of the KiProcessorBlock offset, which knows that it contains an array of pointers to... whatever type of structure the pointers point to (I don't recall off the top of my head). It also doesn't look like the target attribute is set, so these are likely just pointers to... something. meaning when they're accessed, they're unlikely to work as expected?
There was a problem hiding this comment.
I just looked at context.object_from_symbol and we current expect the symbol to contain type information to use that, but I might add a parameter, allowing plugins to construct an object from a symbol name and a type, just so we can reuse most of the existing machinery. For now you'd need to get the symbol and then instantiate it at the symbol's address (somewhat as you're doing here).
| from volatility3.framework.configuration import requirements | ||
| from volatility3.plugins.windows import modules | ||
|
|
||
| GDT_DESCRIPTORS = dict(enumerate([ |
There was a problem hiding this comment.
The can all be stored in data in the JSON ISF file, under the enums section. An example is, so the type and size must be defined, and then each of the possible names and associated values should be listed.
"ETW_COMPRESSION_RESUMPTION_MODE": {
"base": "int",
"constants": {
"EtwCompressionModeNoDisable": 1,
"EtwCompressionModeNoRestart": 2,
"EtwCompressionModeRestart": 0
},
"size": 4
},
| kernel = self.context.modules[self.config["kernel"]] | ||
| layer_name = kernel.layer_name | ||
| symbol_table = kernel.symbol_table_name | ||
| kvo = self.context.layers[layer_name].config["kernel_virtual_offset"] |
There was a problem hiding this comment.
This should also be accessible as kernel.offset, I think.
| layer_name = kernel.layer_name | ||
| symbol_table = kernel.symbol_table_name | ||
| kvo = self.context.layers[layer_name].config["kernel_virtual_offset"] | ||
| ntkrnlmp = self.context.module(symbol_table, layer_name=layer_name, offset=kvo) |
There was a problem hiding this comment.
This should be the same as kernel?
| sect_name = self.get_section_name(ntkrnlmp, layer_name, module, addr) | ||
| else: | ||
| module_name = "UNKNOWN" | ||
| sect_name = '' |
There was a problem hiding this comment.
Please use a class derived from BaseAbsentValue when indicating that data is unavailable or not relevant. This allows user interface designers to know more about how to display the information for the medium they're displaying the output.
| return TreeGrid( | ||
| [ | ||
| ('CPU', int), | ||
| ('Index', str), |
There was a problem hiding this comment.
it isn't clear why Index is a str here, when it's actually already a number and then being converted into a string and have the 0x removed and set to uppercase? If it should be displayed as a number, please put is at an int, if it should be a hex value, please put hex. Again, it allows the user interface designers to know what kind of data is, how to align it, and will allow programs ingesting the data to use the actual value (for example if exported to CSV).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| try: | ||
| yield idt_index, _KIDT(idt_struct) | ||
| except: |
Check notice
Code scanning / CodeQL
Empty except Note
|
|
||
| try: | ||
| yield gdt_index, _KGDT(gdt_struct) | ||
| except: |
Check notice
Code scanning / CodeQL
Empty except Note
| for mod in mods: | ||
| if mod.DllBase + mod.SizeOfImage >= offset and mod.DllBase <= offset: | ||
| return mod | ||
| except: |
Check notice
Code scanning / CodeQL
Empty except Note
There was a problem hiding this comment.
Please figure out exactly which type of exception you're intending to handle, and catch only that one. This allows for other errors that might not have been envisaged to bubble up through the code and potentially be handled more appropriately by something higher up. This goes for all try/except blocks in this code.
| ) | ||
| try: | ||
| yield idt_index, _KIDT(idt_struct) | ||
| except: |
Check notice
Code scanning / CodeQL
Except block handles 'BaseException' Note
|
|
||
| try: | ||
| yield gdt_index, _KGDT(gdt_struct) | ||
| except: |
Check notice
Code scanning / CodeQL
Except block handles 'BaseException' Note
| for mod in mods: | ||
| if mod.DllBase + mod.SizeOfImage >= offset and mod.DllBase <= offset: | ||
| return mod | ||
| except: |
Check notice
Code scanning / CodeQL
Except block handles 'BaseException' Note
Thanks for your reply! I have been busy with my work recently. I will make corrections according to the questions you raised when I am free. |
| ) | ||
|
|
||
|
|
||
| class _KIDT: |
There was a problem hiding this comment.
@Ma1icious I'd recommend using "extensions" instead of this style of class representing _KIDT, _KGDT, and _KPCR. There are examples here:
The advantage is the classes will then be accessible from all plugins, not just the IDT plugin. Also, it will eliminate all the lines like self.Offset = idt_struct.Offset as those will just be set naturally. The properties and methods can mostly stay the same, as they work on extensions as well.
| self.symbol_table = symbol_table | ||
|
|
||
| def idt_entries(self): | ||
| base_idt = self.kpcr.IDT |
There was a problem hiding this comment.
I believe this may only work for x86. On x64, the _KPCR finds its IDT through a member named IdtBase instead. That may be OK if you only plan to support 32-bit systems, however the plugins' list of architectures include Intel64.
| base_idt = self.kpcr.IDT | ||
| idt_index = 0 | ||
| for idt_index in range(256): | ||
| idt_offset = base_idt + 8 * idt_index |
There was a problem hiding this comment.
A good shortcut here would be to use something similar to the code below (from the Windows handles.py file):
ptrs = ntkrnlmp.object(
object_type="array",
offset=table_addr,
subtype=ntkrnlmp.get_type("pointer"),
count=100,
)
You could just create an array of 256 _KIDTENTRY entries and it would result in a list of objects. In that case, you wouldn't have to create 256 objects manually, and you could also remove the hard-coded 8 on line 102, because that would get calculated from the size of the _KIDTENTRY.
|
|
||
| # Since the real GDT size is read from a register, we'll just assume | ||
| # that there are 128 entries (which is normal for most OS) | ||
| for gdt_index in range(128): |
There was a problem hiding this comment.
Same comment as above regarding the use of array. The code here isn't wrong, it could just be simplified a bit and remove some hard coded values.
|
|
||
| # Get kpcr object | ||
| kpcr_offset = ntkrnlmp.get_type("_KPCR").relative_child_offset("PrcbData") | ||
| kpcr = ntkrnlmp.object( |
There was a problem hiding this comment.
It was KD_DEBUGGER_DATA that gets encrypted now. AFAIK the _KPCR types are included in the IST files from ntoskrnl, so they're already available.
|
@Ma1icious there are a number of outstanding changes and comments made from our reviews. Could you please address them so that we can get the code merged? |
|
No response from the author so converting this to a draft. |
I moved volatility2 windows idt plug-in to volatility 3 and is currently being tested somewhat, although the code is not very elegant.
Relevant Issue: #974