search

volatilityfoundation / volatility3

Volatility 3.0 development
http://volatilityfoundation.org/
Other
2.73k stars 463 forks source link

vad get_protection method may throw an InvalidAddressException #144

Closed ikelos closed 4 years ago

ikelos commented 5 years ago

The get_protection method can throw InvalidAddressException as part of the array lookup , so we need to decide whether we catch the exception or require calling plugins (such as malfind) to catch it? I'm slightly nervous about taking control out of the calling plugins hands, because I don't think ignore it (treating value as 0) properly indicates what's happened. With something like malfind we may want to ignore it (and log that we ignored it), but I don't know if that will always be the case...

iMHLv2 commented 4 years ago

@ikelos Do you have a backtrace for this? Any chance the InvalidAddressException is being raised here instead?

https://github.com/volatilityfoundation/volatility3/blob/master/volatility/framework/plugins/windows/vadinfo.py#L78

ikelos commented 4 years ago

So debugging information we need is the offset of the protect_values array, and the index being accessed...

ikelos commented 4 years ago

So it appears this is because we read the protect_values from the layer of the object in question and not the kernel (so the layer's copy of this table may have been paged out, but the kernel's hasn't).

iMHLv2 commented 4 years ago

@ikelos Let me know if this works better:

https://github.com/volatilityfoundation/volatility3/tree/issue-10-vad-protection-layer

We could derive the layer from the proc object or from the context that's already being passed in, but I added an additional layer_name parameter since most methods that take a symbol table also explicitly take a layer.

ikelos commented 4 years ago

Yep, that looks good. I think it would need to be a parameter, because using the proc object is what caused the problem in the first place (for those reading, we identified that some processes had their copy of the protection table paged out) and we couldn't do it from the context, because we couldn't tell which layer to use (config would be plugin specific and we parameterized these classmethods specifically so we didn't use the config to pull out any values, to allow cross use between plugins).

I feel mebbe that layer_name is a little generic, because in this case it needs not be the proc's layer, it almost should be labelled proc_base_layer_name? Lemme know if you think that'd be too cumbersome? I'm ok on just layer_name (because it fits with convention), but then the docstring definitely needs updating to explain precisely what layer_name means in this situation. But yeah, seems like a good solution, fire it up as a pull request, I can document the requested changes and we get those made then I'm happy to merge it! Thanks! 5:)