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

Change default behavior of required OptString to permit empty strings #11061

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@asoto-r7
Contributor

asoto-r7 commented Dec 4, 2018

In an attempt to address #10166 and #10879, @wvu, @bcook, @wchen-r7 and I have joined forces.

The problem

Presently, a required string option is not allowed to be set to false. So a user trying to throw an exploit using an empty password is unable to run the exploit, because the valid? method fails. For example, trying to use an empty password by using two single-quotes:

msf5 exploit(windows/mysql/mysql_mof) > set PASSWORD ''
PASSWORD => 
msf5 exploit(windows/mysql/mysql_mof) > run

[-] 127.0.0.1:3306 - Exploit failed: The following options failed to validate: PASSWORD.
[*] Exploit completed, but no session was created.

Also with double quotes:

msf5 exploit(windows/mysql/mysql_mof) > set PASSWORD ""
PASSWORD => 
msf5 exploit(windows/mysql/mysql_mof) > run

[-] 127.0.0.1:3306 - Exploit failed: The following options failed to validate: PASSWORD.
[*] Exploit completed, but no session was created.

The proposed fix

By default, an empty string that is required fails the validation check. This PR updates the valid? method for only OptString to allow an empty string to pass validation. This change will allow users to use an empty password, as in the case of the mysql_mof attempt above.

Before:

https://github.com/asoto-r7/metasploit-framework/blob/fe1b24e666a52170e3f6ada6d9ede30d32612acb/lib/msf/core/opt_string.rb#L31-L35

After:

https://github.com/asoto-r7/metasploit-framework/blob/117d8ad986dec42d9c0cc02ed2598771fab1ff81/lib/msf/core/opt_string.rb#L31-L35

Verification

List the steps needed to make sure this thing works

  • Start msfconsole
  • use exploit/windows/mysql/mysql_mof
  • set RHOST 127.0.0.1
  • set USERNAME "user"
  • set PASSWORD ""
  • run
  • Confirm that the module begins to run. Feel free to ignore the ConnectionRefused exception, unless you go through the trouble of setting up a MySQL server:
msf5 exploit(windows/mysql/mysql_mof) > run

[*] 127.0.0.1:3306 - Attempting to login as 'user:'
[-] 127.0.0.1:3306 - Exploit failed [unreachable]: Rex::ConnectionRefused The connection was refused by the remote host (127.0.0.1:3306).
[*] Exploit completed, but no session was created.

Alternatively, if you did set up a pre-Vista MySQL server (tested with v5.5.21-winx64):

msf5 exploit(windows/mysql/mysql_mof) > run

[*] Started reverse TCP handler on 192.168.199.137:4444 
[*] 192.168.220.128:3306 - Attempting to login as 'root:'
[*] 192.168.220.128:3306 - Uploading to 'C:/windows/system32/BcSGf.exe'
[*] 192.168.220.128:3306 - Uploading to 'C:/windows/system32/wbem/mof/OHFDd.mof'
[!] 192.168.220.128:3306 - This exploit may require manual cleanup of 'BcSGf.exe' on the target
[!] 192.168.220.128:3306 - This exploit may require manual cleanup of 'wbem\mof\good\OHFDd.mof' on the target

Additional tests would be helpful, especially in a case where a module is not expecting a user to set an empty string. Per the discussion with @bcook-r7 and @wvu-r7, we agree that if a user chooses to set VAR_NAME "", they have intentionally accepted the risk of that action.

As always, feedback is welcome. :-)

@asoto-r7 asoto-r7 added the bug label Dec 4, 2018

@asoto-r7

This comment has been minimized.

Contributor

asoto-r7 commented Dec 4, 2018

Addendum: I've confirmed these code changes do not impact ms17_010_eternalblue and struts2_namespace_ognl, so I'm reasonably confident we're not going to break anything.

Again, feedback welcome. :-)

@@ -31,7 +31,7 @@ def normalize(value)
def valid?(value=self.value, check_empty: true)
value = normalize(value)
return false if check_empty && empty_required_value?(value)
return super
return super(check_empty: false)

This comment has been minimized.

@acammack-r7

acammack-r7 Dec 4, 2018

Contributor

This would be best fit in a subclass of OptString, as there a number of modules that really shouldn't have empty strings for required arguments. Options that default to the empty string could automatically be converted under the covers to keep code-touch low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment