Add new resident data displaying plugin. Scan in correct layer. Condense code and remove duplication. Fix bugs #1169
Conversation
There was a problem hiding this comment.
Mmmm, this is kinda skirting the rules by making the generator and run dummy functions and then cramming all the differences into the parse_mft_records method. 5:S The generator really shouldn't be returning different sized output depending on input flags (or if it should that should definitely all stay within one plugin).
I think the best way for this set of plugins to work would be to have the enumerate_mft_records central and used from one place (ideally parameterized and made a classmethod, I'm not sure why it stores local state mid run?) and then the run/generator and the appropriate columns should be defined in the plugins that use them (so parse_mft_records gets distributed across the plugins. That even negate the need for inheritance, but I'll let you evaluate that...
|
I am going to rewrite this a bit based on discussion from this weekend and what you said in this ticket, but I did want to point out early that each plugin does have its own |
|
That's fine. This only has a run but calls a generator from a child. The problem is pulling some bits from one plugin but redefining other bits itself, which makes an easily missed dependency between the two. If you're going to make it so that changes in a parent can't easily break the child without the user getting a nice message telling them about the problem, then that would be ideal... |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
ok @ikelos I reworked all of this to match where we have the staticmethods and classmethods and each plugin has its own run and generator.. removing the inheritance lets the flag about which $DATA to display be part of the parameters and also the callbacks have consistent columns in their return data sets. |
|
Still waiting on a couple of minor things then it should be fine to go in... |
|
If you get this past the black formatting, it can be merged now, thanks for the changes... |
fixed now |
There was a problem hiding this comment.
Yep, looking pretty good, much happier about the way this works now. Just one little last gotcha is passing the whole config in rather than just the data you need. The whole config will be tricky for other plugins to match up. Otherwise it looks good to go in! 5:D
| conversion.wintime_to_datetime(attr_data.AccessedTime), | ||
| file_name, | ||
| ) | ||
| for record in attr_callback( |
There was a problem hiding this comment.
Looks like you could probably use yield from here, but honestly this is fine. More just a point of interest...
https://docs.python.org/3/whatsnew/3.3.html#pep-380
| file_name, | ||
| ads_name, | ||
| content, | ||
| ) in MFTScan.enumerate_mft_records( |
There was a problem hiding this comment.
This also looks like it could be done as a yield from?
atcuno
force-pushed
the
mftscan_updates_resident_data_plugin
branch
from
d6772b9
to
685dc97
Compare
|
Feedback addressed @ikelos . I added yield from anywhere feasible plus the primary layer name passing. |
| attribute_object, | ||
| offset=offset + attr_base_offset, | ||
| layer_name=layer.name, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def parse_mft_records(record_map, mft_record, attr, symbol_table): |
There was a problem hiding this comment.
This could do with some typing information, but not a show stopper...
| context: interfaces.context.ContextInterface, | ||
| config_path: str, | ||
| primary_layer_name: str, | ||
| attr_callback, |
There was a problem hiding this comment.
Could we type this as a Callable at least, if not providing the full function signature please?
| # past the first $DATA record, attempt to get the ADS name | ||
| # NotAvailableValue = > 1st Data, but name was not parsable | ||
| ads_name = attr.get_resident_filename() | ||
| if not ads_name: |
There was a problem hiding this comment.
Feels like this could be ... = attr.get_resident_filename() or renderers... but I'm fine if you want to keep it this way.
| record_map: Dict[int, Tuple[str, int, int]], | ||
| mft_record: interfaces.objects.ObjectInterface, | ||
| attr: interfaces.objects.ObjectInterface, | ||
| symbol_table, |
There was a problem hiding this comment.
Just for clarity, could we please rename this to symbol_table_name indicating it's not the actual symbol table object, and give it type information too?
This MR performs a number of tasks related to the mftscan file and MFT-related functionatlity:
Fixes bugs, such as incorrect instantiation of absent values
Creates a unified and inheritable code flow for plugins that want to parse MFT records and their attributes.
Previously, ADSscan and MFTScan scanned in the kernel virtual address space, which is incorrect and caused in many samples over half the entries to be missed. These scans should occur in the physical address space.
Stops printing a disassembly of ADS data, which makes no sense anyway and made the output strange.
Adds a new plugin, ResidentData, in the file that hexdumps all of the resident data of a file (the first $DATA attribute). This inherits and uses a combined implementation of $DATA attribute access from the ADS plugin, which displays the second (and third and fourth ..., if present) $DATA attribute. Volatility 2 displayed resident data inline with the regular MFTscan type output, but this was confusing and would break grep results.