modules/post/windows/gather/checkvm.rb improvement suggestions

[bwatters-r7]

This is related to the PR #18140

While reviewing the PR, we established that the changes were good and we wanted to bring them in, but that the underlying module has had so much bolted on over time that it is a bit kludgy and could be vastly streamlined and improved.

Specifically, it seems like over time, developers have just added return true if condition is met in each instance. While it is good we bail as soon as we find evidence, there are also several times we query the same registry key ot call the same command multiple times. Also, some of the methods defined in the module already appear to exist elsewhere.
For example:

• service_exists? is already in the windows service post library

• HKLM\HARDWARE\DEVICEMAP\Scsi\Scsi Port 0\Scsi Bus 0 is queried repeatedly for different vm fingerprints, each compared to something else.

• I don't see any controlling for registry redirection, though I admit, I don't know if it matters in this case

• It might be nice to know that we're running in a sandbox in a virtual environment, so the current setup that only a sandbox or a vm could be reported seems limiting.

@smcintyre-r7 @jmartin-r7 @cdelafuente-r7

[gardnerapp]

Hello, I have completely refactored the existing module, it is downstream from this pull request which has not yet been pulled into the rapid7/master branch. I will initiate a pull request there, but I am new to open source and am not sure if that is a best practice. Let me know! The basic philosophy behind these improvements is as follows:

To avoid querying multiple processes, services, and registries I stored the data from these initial queries within instance variables that can be referenced by whatever_vm? methods later. The original code was not very dry, there are multiple points where loop blocks are used to enumerate over services, processes and registries. I abstracted these to several methods, for example the services_exist? method takes a list of services that are fingerprints of vm usage and compares them to the services running on the machine. As stated before similar methods were created for processes and registry keys.

Many times the code would make queries and if the query came up empty it would result in a nil object which would result in a no method error when calling include?. This would also result in having to repeatedly check if the desired object is not nil before calling any Array methods on it. To avoid the redundancy, when making a initial query of processes, registries etc. I would do a nil check and set the object to an empty list if so.

Hopefully reducing the redundancy of queries on the host system will make this module stealthier and easier to refactor in the future. In regards to method naming conflicts the methods could either be 1) renamed, 2) made private so that they only exists within this class or 3) create a module/class existing within this class to avoid name spacing conflicts.

One final note, within the run method there is a large if/elsif block containing all of the methods for querying the VM's. For example parallels? is the first method called and it stores to instance variables @system_bios_version and @video_bios_version. Subsequent methods in the calling chain such as hyperv?, virtual_box?, and qemu? all use these instance variables. If for some reason the code changes in the future and one of these methods is called before parallels? contributors will have to be aware that the setting of these various instance variables is dependent on the order in which these methods are called.