Set options as a required option when a default value is set

[cdelafuente-r7]

This is not really a bug but more coding practice, but some modules like this one have options with a default value set and the required flag set to false. It makes more sense to have options set as required in this case. This not a big deal, but fixing this will avoid copy/pasting this over and over (e.g. here).

[errorxyz]

Grepping for the pattern:

└─$ grep -rn "\[[ ]*false,[ ]*\".*\",.*\]" modules/
modules/post/multi/gather/chrome_cookies.rb:24:        OptString.new('CHROME_BINARY_PATH', [false, "The path to the user's Chrome binary (leave blank to use the default for the OS)", '']),
modules/encoders/x86/opt_sub.rb:52:        OptBool.new( 'OverwriteProtect', [ false, "Indicate if the encoded payload requires protection against being overwritten", false])
modules/exploits/bsdi/softcart/mercantec_softcart.rb:59:        OptString.new('URI', [ false, "The target CGI URI", '/cgi-bin/SoftCart.exe' ])
modules/exploits/multi/svn/svnserve_date.rb:81:        OptInt.new('RetLength', [ false, "Length of rets after payload", 100 ]),
modules/exploits/multi/svn/svnserve_date.rb:82:        OptBool.new('IgnoreErrors', [ false, "Ignore errors", false ])
modules/exploits/multi/misc/wireshark_lwres_getaddrbyname_loop.rb:143:      OptBool.new("ExitOnSession", [ false, "Return from the exploit after a session has been created", true ])
modules/exploits/multi/http/phpmyadmin_null_termination_exec.rb:54:        OptString.new('PASSWORD', [ false, "Password to authenticate with", '']),
modules/exploits/multi/http/vtiger_php_exec.rb:49:        OptString.new('PASSWORD', [ false, "Password to authenticate with", 'admin'])
modules/exploits/multi/http/ispconfig_php_exec.rb:48:        OptString.new('PASSWORD', [ false, "Password to authenticate with", 'admin']),
modules/exploits/multi/http/cmsms_showtime2_rce.rb:52:        OptString.new('PASSWORD', [false, "Password to authenticate with", ''])
modules/exploits/multi/http/phpmyadmin_lfi_rce.rb:51:        OptString.new('PASSWORD', [ false, "Password to authenticate with", ''])
modules/exploits/multi/http/openmediavault_cmd_exec.rb:49:      OptString.new('PASSWORD', [ false, "Password to authenticate with", 'openmediavault'])
...

└─$ grep -rn "\[[ ]*false,[ ]*\".*\",.*\]" modules/ | wc -l
277

[errorxyz]

Can we run a quick one liner for this or do we need to check each of them individually?

[cdelafuente-r7]

I wasn't expecting this much options set like this :/. I'm afraid this might require individual review, or at least for some category of changes.

A couple of examples that come in mind right now:

OptBool.new( 'OverwriteProtect', [ false, "Indicate if the encoded payload requires protection against being overwritten", false])

For this one, changing the required flag to true is fine since a default value is set.

OptString.new('PASSWORD', [ false, "Password to authenticate with", '']),

This one is a bit different, since the default value is an empty string, I would be inclined to remove the default value and keep the required flag to false. That said, it would require to check the code if guards are in place since it would be nil by default. Note that these guards should be there already since the option is not required and can be nil anyway, but who knows...

[github-actions[bot]]

Hi!

This issue has been left open with no activity for a while now.

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 30 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request.

[dwelch-r7]

Just to drop my thoughts here, it would be super nice to have this for options that are required and have a default value, but I do think there's definitely going to be cases where the option has a default value but isn't required, thinking maybe a password has a very common value which would be nice to have as a default but it may also possible to login without a password as an example

In other words, I don't think there's much we can do beyond check them one by one 😅