Updates to the AutoCheck mixin

[zeroSteiner]

This updates the AutoCheck mixin to allow the module author to rely on some guarantees regarding it's execution. It adds a new AutoCheckLevel module info option which the module author can use to reliably ensure that the check method at least runs (LEVEL_BYPASSABLE) or runs and returns a result implying that the target is vulnerable (LEVEL_REQUIRED). This makes it a lot easier to rely on module instance state set within the check method. If the connection is established in the check method, and required in the exploit, it might make sense to use LEVEL_BYPASSABLE, which guarantees that at least the check method ran and presumably connected. If the exploit's check method maybe gathers a sensitive token that's then used in the exploit and determines the vulnerable state based on whether it could leak the token, then it makes sense for the check method to be a hard requirement and abort execution if the required token couldn't be leaked.

This fixes the stack trace in #19123 by preventing the user from bypassing the check method which as failing due to a connection error. The check method in printnight makes sense to be a requirement. There's a fair bit of setup to access the DCERPC interface and the response to the method can be used to reliably fingerprint whether or not the target is vulnerable.

Verification

• `Use the new printnightmare exploit
• See that the check method can no longer be disabled (via AutoCheck) or bypassed (via ForceExploit)

[adfoster-r7]

I feel like this PR might be trying to solve a mixture of user error, and the module in question not being runnable without the check method having first been run 🤔

i.e. if the user chooses to ignore the check method entirely, that's on them IMO - but I don't think we should disable this functionality from the user, as sometimes our check methods are indeed wrong and do need bypassed

In this case - it feels like the module itself should be updated to gracefully handle the check method not having been run IMO, likewise for the other modules that expect the check method to always have been run first

[zeroSteiner]

I think there are cases where it is reasonable to require the check method be run to, for example, initiate a connection. These changes make it easier for the module author to express the guarantees they expect of the state the module instance to be in at the point the main method is run. What you're proposing would require the module to either duplicate a fair bit of code to initiate the connection or to place it into a method. What complicates placing it in a new method called from both the check and run methods is the desire to not raise unhandled exceptions from check methods.

If it'd be helpful, I can dig out another module I'd previously written that would have benefited from this feature that inspired the idea.

[adfoster-r7]

I believe the check logic and exploit logic should be separately runnable; the Msf::Exploit::Remote::AutoCheck mixin just attempts to improve the UX of running both methods sequentially

What complicates placing it in a new method called from both the check and run methods is the desire to not raise unhandled exceptions from check methods.
[...]
If it'd be helpful, I can dig out another module I'd previously written that would have benefited from this feature that inspired the idea.

That'd be good to share to understand the scenario a bit better 💯

[zeroSteiner]

Alright, here's more examples of where this would come in handy:

vmware_workspace_one_access_vmsa_2022_0011_chain.rb

In this one, the vulnerability is leveraged to leak the authentication token as part of a bypass. You can see in the #check and #exploit methods, that the module author had to go in and force the code path to ensure that the token is always leaked. The check method in this case is highly accurate, because without the leaked authentication token, the exploit can't run.

atlassian_confluence_rce_cve_2023_22527.rb

This one uses the vulnerability to leak the target platform so the command injection can be properly formatted. Again, the exploit author needs to ensure that the target platform can be identified, and work around it with a cached instance variable. A second instance of this pattern is present in another Confluence OGNL injection exploit, atlassian_confluence_namespace_ognl_injection.rb.

cve_2020_1472_zerologon.rb

Zerologon would be another excellent example if it weren't for the fact that there's another action that doesn't require leveraging the vulnerability (action_restore_passwrod). You can see in Zerologon, it actually forces a call to the check method, because if the vulnerability can't be leveraged then it doesn't make sense to continue.

Based on these examples, the pattern is that this would simplify things for the module author in cases where the vulnerability can be safely leveraged in the #check method (it returns Vulnerable and doesn't perform any significant state changes) and checking for the vulnerability performs the same actions necessary to exploit the vulnerability such as:

• authenticating to the target service (zerologon)
• leaking some kind of token or key
• creating a new account

[zeroSteiner]

Here's another example of a module that's effectively inlining this functionality. The apache_normalize_path_rce is basically using AUTO_CHECK_LEVEL_BYPASSABLE here.

metasploit-framework/modules/exploits/multi/http/apache_normalize_path_rce.rb

Line 131 in 2dce738

if (!check.eql? Exploit::CheckCode::Vulnerable) && !datastore['ForceExploit']

[cdelafuente-r7]

I have the same feeling about this. The check method will have a new meaning now, if set as mandatory to setup some requirements for the exploit. I believe the AutoCheck mixin is here to glue together the #exploit (or #run) method with the #check easily. I'm not sure if #check should now be used as a required pre-exploit setup.

I think using an instance variable is still good to me. This state-machine-like logic seems to already be used in many modules, including the VMware module you linked.

def setup_connection
  return if @connected
  # setup things
  @connected = true
end

def leak_token
  return @leak_token if @leak_token
  @leak_token = ... # do something to leak the token
end

def check
  setup_connection
  leak_token
rescue CustomException::Error => e
  return Exploit::CheckCode::Unknown
rescue CustomException::TokenNotLeaked => e
  return Exploit::CheckCode::Safe
end

def exploit
  begin
    setup_connection unless @connected
    leak_token unless @leak_token
  rescue CustomException::Error => e
    fail_with(...)
  rescue CustomException::TokenNotLeaked => e
    fail_with(...)
  end
...
end

[cdelafuente-r7]

It looks like this method is not used and I cannot find a reference to the info hash.

[adfoster-r7]

I'm in favor of Christophe's mental model too; Potentially we're missing verification steps in our testing approach to ensure that the exploit method works in isolation, i.e. verify:

1. check
2. check + exploit
3. exploit