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

Do not return empty strings in hash if option in datastore is nil #19862

Conversation

sjanusz-r7
Copy link
Contributor

@sjanusz-r7 sjanusz-r7 commented Feb 6, 2025

This is in draft for now as I explore other options to fix this issue in Metasploit Pro.

This PR fixes an issue in Metasploit Pro.

With this PR, the datastore no longer coalesces nil values into empty strings. For example:

datastore['TEST_VAL'] = nil
datastore['USERNAME'] = 'rapid7'

datastore_hash = datastore.to_h
datastore_hash == { 'TEST_VAL' => '', 'USERNAME' => 'rapid7' }

Now, nil values are ignored:

datastore['TEST_VAL'] = nil
datastore['USERNAME'] = 'rapid7'

datastore_hash = datastore.to_h
datastore_hash == { 'USERNAME' => 'rapid7' } # TEST_VAL is not present

The semantics of assigning an empty string are unchanged. E.g.:

datastore['EMPTY'] = ''

datastore_hash = datastore.to_h
datastore_hash == { 'EMPTY' => '' }

This results in the expected results of:

datastore_hash['TEST_VAL'] == nil

Example from the RSpec tests:

[4] > subject
=> #<Msf::ModuleDataStoreWithFallbacks:0x000000011a458538
 @_module=#<InstanceDouble(Msf::Exploit) (anonymous)>,
 @aliases={},
 @defaults={},
 @options={},
 @user_defined={"foo"=>"foo_value", "bar"=>"bar_value", "empty"=>"", "example_nil"=>nil}>
[5] > subject.to_h
=> {"foo"=>"foo_value", "bar"=>"bar_value", "empty"=>""}

Verification

  • Start msfconsole
  • Ensure you can run a module with the correct options
  • Point Pro at this Framework commit
  • Ensure you can replay a session using psexec in Pro
  • Run framework tests using bundle exec rspec spec/lib/msf/core/data_store_with_fallbacks_spec.rb

@adfoster-r7 adfoster-r7 self-assigned this Feb 6, 2025
@sjanusz-r7 sjanusz-r7 marked this pull request as draft February 6, 2025 11:15
@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Feb 6, 2025

I think you're accidentally trying to fix the side effect of this decision:

# Override Hash's to_h method so we can include the original case of each key
# (failing to do this breaks a number of places in framework and pro that use
# serialized datastores)
def to_h
datastore_hash = {}
self.keys.each do |k|
datastore_hash[k.to_s] = self[k].to_s
end
datastore_hash
end

This PR might not be the correct fix for your original problem, maybe worth a double check 👀

@sjanusz-r7 sjanusz-r7 closed this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants