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

Make check_vm.rb DRY and Stealthy #18218

Merged
merged 36 commits into from
Nov 3, 2023
Merged

Conversation

gardnerapp
Copy link
Contributor

According to #18149 the check_vm .rb module is very disorganized and makes repeated calls on the host system to retrieve registry keys, services and processes to determine if the machine is running inside of a virtual environment. Constantly calling enumerating registries, services and processes hinders stealth and is not a good programming practice because you're writing the same code over and over again.

To solve this issue I organized the module so that whenever a process, registry key or service is initially checked by the module on the host system that data would be stored in an instance variable. When another method needs to see if a given process, registry or service exists it does not have to repeat calls to the enumerate these services on the machine, it can just check the data in the instance variable.

Another issue was repeated looping over VM fingerprints and checking if these are included on the active machine. For example each of the different VM methods would loop over a given set of finger print processes and compare those to the running processes. I abstracted the enumeration of active vs fingerprints into the processes_exist? method which takes a list of fingerprint processes and checks to see if they are included within in the list stored in the @processes variable.

The services_exist? and key_present? methods were also created under a similar philosophy. For example the services_exist? method enumerates over the fingerprint services to see if they are included in the @services variable, which is an array containing the active services on the machine. The @keys instance variable is of type hash with the registry key as the key and it's corresponding value unironically assuming it's value.

Additionally, the module will make calls to retrieve other keys like video bios, system bios or scsi bus values. These were more distinct than the other keys stored in @keys and were given their own instance variables, for example @system_bios_version.

Lastly, if the module goes to gather some data, be it a service, process or whatever and nothing is available then it will return a nil object. This is problematic because whenever include? is called a no-method error would arise and the previous developers wrote in a series of nil checks before calling include. To avoid these repetitive nil checks I would simply set the data to an empty array if it happened to be nil upon retrieval.

@adfoster-r7
Copy link
Contributor

@gardnerapp I believe the base code has been merged in now, and this can be rebased against the latest code in master 👍

jheysel-r7 and others added 27 commits September 17, 2023 18:05
@gardnerapp
Copy link
Contributor Author

All set on the rebase !

@bwatters-r7
Copy link
Contributor

@jheysel-r7 do you still have an environment for testing this?

@jheysel-r7 jheysel-r7 self-assigned this Oct 5, 2023
Copy link
Contributor

@jheysel-r7 jheysel-r7 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @gardnerapp! Love the idea of using instance variables to reduce the number of requests sent to the machine. Found a couple of minor issues outlined below.

modules/post/windows/gather/checkvm.rb Show resolved Hide resolved
modules/post/windows/gather/checkvm.rb Outdated Show resolved Hide resolved
modules/post/windows/gather/checkvm.rb Outdated Show resolved Hide resolved

hyperv_services = %w[vmicexchange vmicheartbeat vmicshutdown vmicvss]

return true if services_exist?(hyperv_services)
Copy link
Contributor

Choose a reason for hiding this comment

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

The instance variable @services never gets populated before services_exist? gets called.

Probably want to define @services and @processes at the top of the run method. We could define them as they are needed to potentially reduce the amount of requests sent to the target although I know we've switched around the order in which each hyper-visor is checked to remediate false positives before. Seems like a better practice to have them defined at the beginning of run.

Copy link
Contributor Author

@gardnerapp gardnerapp left a comment

Choose a reason for hiding this comment

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

OK, I integrated the changes some I did by hand. This is my first time doing this let me know if I missed anything. P.S. thanks for the instance variable compliment.

gardnerapp and others added 4 commits October 9, 2023 15:23
correct spelling

Co-authored-by: jheysel-r7 <Jack_Heysel@rapid7.com>
Co-authored-by: jheysel-r7 <Jack_Heysel@rapid7.com>
Copy link
Contributor

@jheysel-r7 jheysel-r7 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! VMware is still being reported as Hyper-V. Everything else seems to be working as expected.

Parallels:

msf6 post(windows/gather/checkvm) > run

[*] Checking if the target is a Virtual Machine ...
[+] This is a Parallels Virtual Machine
[*] Post module execution completed

VMware:

msf6 post(windows/gather/checkvm) > run

[*] Checking if the target is a Virtual Machine ...
[+] This is a Hyper-V Virtual Machine
[*] Post module execution completed

Virtual Box

msf6 post(windows/gather/checkvm) > run

[*] Checking if the target is a Virtual Machine ...
[+] This is a VirtualBox Virtual Machine
[*] Post module execution completed

Qemu:

msf6 post(windows/gather/checkvm) > rexploit
[*] Reloading module...

[*] Checking if the target is a Virtual Machine ...
[+] This is a Qemu/KVM Virtual Machine
[*] Post module execution completed

modules/post/windows/gather/checkvm.rb Outdated Show resolved Hide resolved
modules/post/windows/gather/checkvm.rb Show resolved Hide resolved
@jheysel-r7
Copy link
Contributor

All hypervisor are now being correctly reported 👍 Thanks for the PR @gardnerapp!

@jheysel-r7 jheysel-r7 merged commit ce5188a into rapid7:master Nov 3, 2023
34 checks passed
@jheysel-r7 jheysel-r7 added rn-enhancement release notes enhancement enhancement labels Nov 3, 2023
@jheysel-r7
Copy link
Contributor

Release Notes

This PR reduces the number of requests the Windows checkvm post module sends to the host when attempting to determine what hypervisor the session is running in by saving the initial responses in instance variables for later use in the module. The PR also includes many other general code improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement rn-enhancement release notes enhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants