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.

Sign up for GitHub

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

adding a new column cracked password in creds command to show cracked… #17689

Merged
merged 10 commits into from
Oct 13, 2023
Merged

adding a new column cracked password in creds command to show cracked… #17689

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cae7f8c
adding a new column cracked password in creds command to show cracked…
manishkumarr1017 Feb 23, 2023
60113f7
fixing spec files for creds command new enhancements
manishkumarr1017 Mar 1, 2023
28a2bcf
Fix calculation of delete_count size
gwillcox-r7 Mar 6, 2023
dc97b33
Merge branch 'rapid7:master' into add_new_column_in_creds
manishkumarr1017 Mar 18, 2023
02608a4
adding extra specs for the new enhancement and optimizing the queries
manishkumarr1017 Mar 18, 2023
4aea945
fixing the failed specs by keeping the exact output necessary
manishkumarr1017 Mar 18, 2023
df4a5b9
Merge branch 'master' of github.com:manishkumarr1017/metasploit-frame…
manishkumarr1017 May 19, 2023
bd9591f
changing nil datatype to string datatype for the column display
manishkumarr1017 May 19, 2023
375a91e
Merge branch 'rapid7:master' into add_new_column_in_creds
manishkumarr1017 Jun 23, 2023
87582ee
PR Review changes
manishkumarr1017 Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions lib/msf/ui/console/command_dispatcher/creds.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ def creds_search(*args)
set_rhosts = false
truncate = true

cred_table_columns = [ 'host', 'origin' , 'service', 'public', 'private', 'realm', 'private_type', 'JtR Format' ]
cred_table_columns = [ 'host', 'origin' , 'service', 'public', 'private', 'realm', 'private_type', 'JtR Format', 'cracked_password' ]
delete_count = 0
search_term = nil

Expand Down Expand Up @@ -432,6 +432,7 @@ def creds_search(*args)
opts[:workspace] = framework.db.workspace
query = framework.db.creds(opts)
matched_cred_ids = []
cracked_cred_ids = []

if output_file&.ends_with?('.hcat')
output_file = ::File.open(output_file, 'wb')
Expand All @@ -444,8 +445,9 @@ def creds_search(*args)
tbl = Rex::Text::Table.new(tbl_opts)
end

filtered_query(query, opts, origin_ranges, host_ranges) do |core, service, origin|
filtered_query(query, opts, origin_ranges, host_ranges) do |core, service, origin, cracked_password_core|
matched_cred_ids << core.id
cracked_cred_ids << cracked_password_core.id if cracked_password_core.present?

if output_file && output_formatter
formatted = output_formatter.call(core)
Expand Down Expand Up @@ -488,7 +490,8 @@ def creds_search(*args)
private_val,
realm_val,
human_val, #private type
jtr_val
jtr_val,
cracked_password_core&.private&.data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this being nil potentially negatively impact anything vs if it was a blank String? Just something to consider. I think the other fields here all default to an empty string if the field doesn't exist, so it might be a good idea to follow this pattern?

Copy link
Contributor Author

@manishkumarr1017 manishkumarr1017 Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure I will change it to empty string. I Apologise that I didn't notice this comment.

]
end
end
Expand All @@ -502,7 +505,7 @@ def creds_search(*args)
end

if mode == :delete
result = framework.db.delete_credentials(ids: matched_cred_ids.uniq)
result = framework.db.delete_credentials(ids: matched_cred_ids.concat(cracked_cred_ids).uniq)
delete_count = result.size
end

Expand Down Expand Up @@ -535,6 +538,13 @@ def cmd_creds_tabs(str, words)

def filtered_query(query, opts, origin_ranges, host_ranges)
query.each do |core|
# we will not show the cracked password in a seperate row instead we will show in seperate column
if core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) && query.find_by(id: core.origin.metasploit_credential_core_id).present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) && query.find_by(id: core.origin.metasploit_credential_core_id).present?
# calling `find_by` against this result is a rails direct interaction that may fail in json database mode
if core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) && query.find_by(id: core.origin.metasploit_credential_core_id).present?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah added the comment but is there anyway we can handle the web json service?

next
elsif core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword)
core = framework.db.creds.find_by(id: core.origin.metasploit_credential_core_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elsif core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword)
core = framework.db.creds.find_by(id: core.origin.metasploit_credential_core_id)
elsif core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword)
# calling `find_by` against this result is a rails direct interaction that may fail in json database mode
original_core = framework.db.creds.find_by(id: core.origin.metasploit_credential_core_id)
if original_core
core = original_core
end

There is no referential integrity guarantee that origin for the crack will be in the database. Either test for nil or just show this credential that in the query result as a standard Password.

Also even if the core does exist this results in a possible edge case that would override the query results by adding in another core that did not meet the original search params.

I am not sure on the best approach here based on the tests injecting the additional core is intentional however this could for some be considered unexpected output.

Another option is simply to skip this core.

Suggested change
elsif core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword)
core = framework.db.creds.find_by(id: core.origin.metasploit_credential_core_id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah Actually I have added this elsif condition to give correct results when creds -t password is used. The actual result before these changes showed both the passwords and the cracked passwords but after my change (incase of keeping the if condition as core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) then it shows only passwords and cracked passwords get skipped so I have kept this if and elsif condition.

So here after checking your changes
for the 1st suggestion I got this output

creds -t password

host  origin  service  public  private           realm  private_type        JtR Format  cracked_password
----  ------  -------  ------  -------           -----  ------------        ----------  ----------------
                       simon   4F8BC1809CB2AF77         Nonreplayable hash  des,oracle  A
                       SYSTEM  9EEDFA0AD26C6D52         Nonreplayable hash  des,oracle  THALES
                       guest   guest password           Password

But in the 2nd suggestion

creds -t password
host  origin  service  public  private         realm  private_type  JtR Format  cracked_password
----  ------  -------  ------  -------         -----  ------------  ----------  ----------------
                       simon   A                      Password
                       SYSTEM  THALES                 Password
                       guest   guest password         Password

I got it like this.
Seems both are logical to me.
But as you have suggested above that the elsif condition might add some creds which doesn't belong there. So seems that the second suggestion looks good. For now committing the 2nd suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah updated the file with the suggested changes.

end

# Exclude non-blank username creds if that's what we're after
if opts[:user] == '' && core.public && !(core.public.username.blank?)
next
Expand All @@ -558,11 +568,13 @@ def filtered_query(query, opts, origin_ranges, host_ranges)
next
end

cracked_password_core = core.public.cores.where(origin_type: "Metasploit::Credential::Origin::CrackedPassword").joins("LEFT JOIN metasploit_credential_origin_cracked_passwords ON metasploit_credential_origin_cracked_passwords.id = metasploit_credential_cores.origin_id").find_by("metasploit_credential_origin_cracked_passwords.metasploit_credential_core_id = (?)", core.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a direct rails interaction that cannot cross the json db service layer, need to adjust to query params
that are passed as opts and filter against query result we received above that is not guarantee to be an ActiveRecordRelation.

Suggested change
cracked_password_core = core.public.cores.where(origin_type: "Metasploit::Credential::Origin::CrackedPassword").joins("LEFT JOIN metasploit_credential_origin_cracked_passwords ON metasploit_credential_origin_cracked_passwords.id = metasploit_credential_cores.origin_id").find_by("metasploit_credential_origin_cracked_passwords.metasploit_credential_core_id = (?)", core.id)
# this is a direct rails interaction that cannot cross the json db service layer, access of origin objects may be problematic
cracked_password_core = nil
if !core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) && core.public.cores.count > 1
core.public.cores.each do |potential_cracked_core|
next unless potential_cracked_core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword)
if potential_cracked_core.origin.originating_core == core
cracked_password_core = potential_cracked_core
end
end
end

Avoids the hardcoded database structure strings but still expects fully linked ActiveRecord objects that will not respond as expected when connected to a web json service instead of postgresql directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here core.public.cores.each does a full record extraction. core.public.cores.where(origin_type: "Metasploit::Credential::Origin::CrackedPassword") can filter most of the unnecessary cores right?
I understood that we are avoiding hardcoded strings but feels that the where query can make it a bit faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah updated the file with the suggested changes.


if core.logins.empty?
service = service_from_origin(core)
next if service.nil? && host_ranges.present? # If we're filtering by login IP and we're here there's no associated login, so skip

yield core, service, origin
yield core, service, origin, cracked_password_core
else
core.logins.each do |login|
service = framework.db.services(id: login.service_id).first
Expand All @@ -574,7 +586,7 @@ def filtered_query(query, opts, origin_ranges, host_ranges)
next
end

yield core, service, origin
yield core, service, origin, cracked_password_core
end
end
end
Expand Down
102 changes: 82 additions & 20 deletions spec/lib/msf/ui/console/command_dispatcher/creds_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------',
' thisuser thispass Password '
'host origin service public private realm private_type JtR Format cracked_password',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these changes are good but none of this seems to be testing the new functionality that you added in to make sure that it works in a variety of scenarios. I'd probably want to see some tests added to check this functionality given this code is going to be heavily utilized by anyone running the creds command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, Sorry I thought I should add specs for this new functionality in the next separate PR. Ok So I will add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the specs which tests the new functionality.

'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------',
' thisuser thispass Password '
])
end

Expand All @@ -83,8 +83,8 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------',
'host origin service public private realm private_type JtR Format cracked_password',
'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------',
' thisuser thispass Password '
])
end
Expand All @@ -96,9 +96,9 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------',
' nonblank_pass Password '
'host origin service public private realm private_type JtR Format cracked_password',
'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------',
' nonblank_pass Password '
])
end
end
Expand All @@ -109,9 +109,9 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------',
' nonblank_user Password '
'host origin service public private realm private_type JtR Format cracked_password',
'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------',
' nonblank_user Password '
])
end
end
Expand All @@ -125,8 +125,8 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------'
'host origin service public private realm private_type JtR Format cracked_password',
'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------'
])
end
end
Expand All @@ -137,8 +137,45 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------'
'host origin service public private realm private_type JtR Format cracked_password',
'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------'
])
end
end
context 'showing new column of cracked_password for all the cracked passwords' do
it 'should show the cracked password in the new column named cracked_passwords' do
common_public = FactoryBot.create(:metasploit_credential_username, username: "this_username")
core = FactoryBot.create(:metasploit_credential_core,
origin: FactoryBot.create(:metasploit_credential_origin_import),
private: FactoryBot.create(:metasploit_credential_nonreplayable_hash, data: "some_hash"),
public: common_public,
realm: nil,
workspace: framework.db.workspace)
cracked_core = FactoryBot.create(:metasploit_credential_core,
origin: Metasploit::Credential::Origin::CrackedPassword.create!(metasploit_credential_core_id: core.id),
private: FactoryBot.create(:metasploit_credential_password, data: "this_cracked_password"),
public: common_public,
realm: nil,
workspace: framework.db.workspace)
creds.cmd_creds('-u', 'this_username')
expect(@output).to eq([
"Credentials",
"===========",
"",
"host origin service public private realm private_type JtR Format cracked_password",
"---- ------ ------- ------ ------- ----- ------------ ---------- ----------------",
" this_username some_hash Nonreplayable hash this_cracked_password"
])
end
it "should show the user given passwords on private column instead of cracked_password column" do
creds.cmd_creds('-u', 'thisuser')
expect(@output).to eq([
"Credentials",
"===========",
"",
"host origin service public private realm private_type JtR Format cracked_password",
"---- ------ ------- ------ ------- ----- ------------ ---------- ----------------",
" thisuser thispass Password "
])
end
end
Expand Down Expand Up @@ -206,11 +243,36 @@
'Credentials',
'===========',
'',
'host origin service public private realm private_type JtR Format',
'---- ------ ------- ------ ------- ----- ------------ ----------',
' thisuser thispass Password '
'host origin service public private realm private_type JtR Format cracked_password',
'---- ------ ------- ------ ------- ----- ------------ ---------- ----------------',
' thisuser thispass Password '
])
end
it 'should show all the cores whose private is either password or the private is cracked password' do
common_public = FactoryBot.create(:metasploit_credential_username, username: "this_username")
core = FactoryBot.create(:metasploit_credential_core,
origin: FactoryBot.create(:metasploit_credential_origin_import),
private: FactoryBot.create(:metasploit_credential_nonreplayable_hash, data: "some_hash"),
public: common_public,
realm: nil,
workspace: framework.db.workspace)
cracked_core = FactoryBot.create(:metasploit_credential_core,
origin: Metasploit::Credential::Origin::CrackedPassword.create!(metasploit_credential_core_id: core.id),
private: FactoryBot.create(:metasploit_credential_password, data: "this_cracked_password"),
public: common_public,
realm: nil,
workspace: framework.db.workspace)
creds.cmd_creds('-t', 'password')
expect(@output).to eq([
"Credentials",
"===========",
"",
"host origin service public private realm private_type JtR Format cracked_password",
"---- ------ ------- ------ ------- ----- ------------ ---------- ----------------",
" thisuser thispass Password ",
" this_username some_hash Nonreplayable hash this_cracked_password"
])
end
end

context 'ntlm' do
Expand All @@ -223,8 +285,8 @@
'Credentials',
'===========',
'',
'host service public private realm private_type JtR Format',
'---- ------- ------ ------- ----- ------------ ----------',
'host service public private realm private_type JtR Format cracked_password',
'---- ------- ------ ------- ----- ------------ ---------- ----------------',
" thisuser #{ntlm_hash} NTLM hash"
]
end
Expand Down