Skip to content
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

Feat: Migrate windows idt plug-in to volatility 3 #976

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Ma1icious
Copy link

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

Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now created a branch to try to resolve this, #978 should allow an object_type parameter to be passed to module.object_from_symbol, with the same form as to module.object. Please give this a test and let me know on #978 if it functions as intended... 5:)

from volatility3.framework.configuration import requirements
from volatility3.plugins.windows import modules

GDT_DESCRIPTORS = dict(enumerate([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the same as kernel?

sect_name = self.get_section_name(ntkrnlmp, layer_name, module, addr)
else:
module_name = "UNKNOWN"
sect_name = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

)
try:
yield idt_index, _KIDT(idt_struct)
except:

Check notice

Code scanning / CodeQL

Empty except

'except' clause does nothing but pass and there is no explanatory comment.

try:
yield gdt_index, _KGDT(gdt_struct)
except:

Check notice

Code scanning / CodeQL

Empty except

'except' clause does nothing but pass and there is no explanatory comment.
for mod in mods:
if mod.DllBase + mod.SizeOfImage >= offset and mod.DllBase <= offset:
return mod
except:

Check notice

Code scanning / CodeQL

Empty except

'except' clause does nothing but pass and there is no explanatory comment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Except block directly handles BaseException.

try:
yield gdt_index, _KGDT(gdt_struct)
except:

Check notice

Code scanning / CodeQL

Except block handles 'BaseException'

Except block directly handles BaseException.
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'

Except block directly handles BaseException.
@Ma1icious
Copy link
Author

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:)

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.

@digitalisx digitalisx mentioned this pull request Aug 13, 2023
5 tasks
)


class _KIDT:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ma1icious I'd recommend using "extensions" instead of this style of class representing _KIDT, _KGDT, and _KPCR. There are examples here:

https://github.com/volatilityfoundation/volatility3/tree/develop/volatility3/framework/symbols/windows/extensions

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ikelos
Copy link
Member

ikelos commented Sep 28, 2023

@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?

@ikelos
Copy link
Member

ikelos commented Feb 1, 2024

No response from the author so converting this to a draft.

@ikelos ikelos marked this pull request as draft February 1, 2024 11:05
)

# Get kpcr object
kpcr_offset = ntkrnlmp.get_type("_KPCR").relative_child_offset("PrcbData")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1280 suggest that PrcbData may not always be available in all versions, this should probably either test for it, or be wrapped in a protective try/except to avoid errors making their way back to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants