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

Linux: add member presence checks to linux.checks_afinfo #1038

Merged

Conversation

eve-mem
Copy link
Contributor

@eve-mem eve-mem commented Nov 13, 2023

Hello!

Issue #832 is tracking the fact that newer kernels (from around 2018) won't have the required members for the hooking checks to be performed in this way.

This PR adds some checks around the members of the objects in the _check_afinfo function so that a more informative warning is displayed rather than simply crashing. Does this seem like a good idea?

…o function to stop crashes when none of the required members are present
@Abyss-W4tcher
Copy link
Contributor

Hello, do you think redirecting users to https://github.com/AsafEitani/rootkit_plugins/blob/main/plugins/check_seqops.py, or directly merging both plugins would be a solution ?

They work toward the same goal in the end :)

Regards

@eve-mem
Copy link
Contributor Author

eve-mem commented Nov 14, 2023

@Abyss-W4tcher - I'd imagine that's certainly a possibility if @AsafEitani wanted to make a PR. My understanding is that @atcuno is looking into the way that this issue should be fixed fundamentally, and it might well be by updating it to do similar things to that seqops plugin.

I'm not sure that a framework plugin would every redirect users to a community plugin, but I'd let the core devs make that call. Maybe you and I could help more on the community support side of things and point people to these other resources when it pops up (exactly like you did in the issue comments for #832)

🦊 just a random internet vol user

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.

Looks pretty good to me, just wondering how useful the debugging message is, given it traverse the loop twice every single item its called on, seemingly with not a great value?

yield var_name, hooked_member, hook_address
# check if object has a least one of the members used for analysis by this function
required_members = ["seq_fops", "seq_ops", "seq_show"]
for member in required_members:
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I know it's not a big deal, but this is traversing all the elements do output the debug information, and then does it again (effectively) as part of the any operator. The debug information will probably only be checked if they're all false, so it might not be useful at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with you. Not the best.

I wanted to have some difference between "The plugin run and there was no output, because there was nothing hooked" and "The plugin run and there was no output, because the kernel version isn't supported".

The later might lead to a false sense of security. I'll work up a better way to show that difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tweaked this now, but might be abusing the PluginRequirementException - let me know what you think.

@eve-mem
Copy link
Contributor Author

eve-mem commented Nov 15, 2023

Hello @ikelos

I've tweaked the logic for debug and warning messages. This removes the loops through the same parts twice, and also means that if every check with _check_afinfo fails you'll get a single warning message. Previously this change would have some a warning for every single symbol without the right members - which is quite spamy. It would be 6 warnings with the current plugin.

I may be abusing the PluginRequirementException to do it however... what do you think? I originally used an attribute error, but thought catching those might hide other problems in the future somehow. Maybe there is a better way to do this?

I'm just keen to show the difference between "no results, all is fine" and "no results, because the plugin couldn't check".

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.

Yep, looks good, thanks! Much happier about the logging and checks.. 5:)

@ikelos ikelos merged commit d56297c into volatilityfoundation:develop Nov 15, 2023
14 checks passed
@ikelos
Copy link
Member

ikelos commented Nov 15, 2023

As to the PluginRequirementException, I think it's the right one to use, even if there isn't an explicit requirement exception. Kinda makes me wonder how we'd make an exception like that? I guess we'd need to check that a kernel symbol containing the kernel version was within certain bounds, but that might not be accurate. Defining a requirement on each of the required members would be cumbersome too. I think this works best, I didn't even know we had that exception, but I think it's the right one. If it causes any problems we can swap it out... 5:)

@eve-mem eve-mem deleted the linux_check_afinfo_member_checks branch December 1, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants