Linux: Add error handling to elf extension magic bytes check

[eve-mem]

Hello

This PR adds some error handling to the Linux ELF extension when it checks for the ELF magic bytes.

It was @garanews that spotted the issue #1009. It causes linux.pslist to crash as soon as it hits the first process where the magic bytes are not available.

This doesn't happen with the linux.elfs plugin as it has its own protection here: https://github.com/volatilityfoundation/volatility3/blob/develop/volatility3/framework/plugins/linux/elfs.py#L141-L149

I originally used a padded read in the extension, but thought that a try/expect was better? I could also add the protections to the linux.pslist plugin instead, but I thought that having them in the extension was better?

The original behavior of the extension is to return None and not check any other parts of the ELF if the magic bytes are wrong or unavailable. I've not changed that here, do you think it would be a good idea to allow the option to override this if needed? Likely better left to a different PR.

Thanks!

[ikelos]

A little bit cumbersome. I get why we read the state here, and why we can't just use the ELF layer's checks, but it feels as though we now have three different places where this checking happens. We should probably include in this fix something to remove the specific handling from the linux.Elfs plugin, which you presumably inherit this fix when it's made?

[eve-mem]

Hello @ikelos sorry it took so long to make progress on your comments, I was getting in a twist about how to correctly read _hdr.e_ident when the __init__ function returns quickly if the magic bytes are missing (meaning that _hdr etc don't exist as attributes that can be accessed.)

What do you think of the way I've handled that, adding in a check for those attributes first?

The other option would be not doing the magic bytes checks at all and creating the full object regardless, then adding more logic to the is_valid() function to check if parts of the object are correct/readable etc. I could change it to that way of working if you think that would be best? I sort of prefer that as it means you can make this object wherever you want with no requirements. In the same way you could make a task anywhere, then leave it up to the plugin etc to decide if its valid.

It might mean more work for this PR, or if you'd rather merge this one so that vol stops crashing as in issue #1009 and then I can work on that upgrade after? I'm happy either way.

Thanks again!

[ikelos]

I'm happy to commit this once the has_member change is in place if you'd like, but also happy to delay if there's further work that needs doing on the PR and it makes sense to do it all together? Happy to take your lead on the decision... 5:)

[ikelos]

Just to check does hasattr work on properties that are dynamically created (since Objects generate their members dynamically? I feel as though has_member might be a better/the right way to achieve this?

[eve-mem]

Marking this as draft for a moment, it's not possible to use vols has_member() instead of hasattr as the extension is basically adding these extra attributes, but not via the standard vol route so they don't exist under self.vol for example. Lots of extensions work in this currently, but it still feels like using has_member is the right thing to do.

It'll just take me a little longer to work out what exactly to do here, any and all thoughts/ideas/suggestions are welcome as always.

[ikelos]

So, we typically expect plugins to override members as a property if they're redefining them, or if they're a method to calculate them, then they tend to be get_blah or do_blah, some kind of verb. We could try to jigger has_member to support properties?

[ikelos]

Having said that, a quick search of the codebase shows we use hasattr all over the place, so perhaps that is ok? I guess has member can help you tell what was originally defined in the structure at least, but the point is I think I was being too harsh trying to get rid of hasattr, so I've marked that as resolved. I think that was everything, so... happy to merge this if you're happy?

[eve-mem]

Merging would allow the plugin to work when there are errors, and I'm keen to fix the issue.

Could you merge it to fix that issue? Then longer term we can think about how has member could work with extensions like this or how to do them better.

🙏 Thanks again.