adding a new column cracked password in creds command to show cracked… #17689
Conversation
|
Do I have to fix the spec files too? From the action I can see some the specs got failed due to the changes. |
Yes if you break the specs you will likely need to update those files as well if they failed. Usually other tests get canceled if one fails as running extra versions on the same test would waste computation cycles for GitHub Actions and we pay for the number of actions we run if I understand correctly. Even if we didn't pay, its a public resource at the end of the day so we don't want to clog it up with extra jobs that won't help us if can help it. |
|
Completed fixing the spec files. |
|
Going to hold off on #17667 till this lands, so that its easier to test and such. |
| delete_count = matched_cred_ids.size | ||
| result = framework.db.delete_credentials(ids: matched_cred_ids.concat(cracked_cred_ids).uniq) |
There was a problem hiding this comment.
Bit concerned r.e this. You seem to be changing the delete_count value from the confirmed number of deleted entries to the expected number of deleted entries.
If you look later on in the code we go ahead and print out the number of entries we actually deleted.
I'd probably rework this code so we actually use the result.size number to confirm how many creds we actually deleted. This will also help catch errors where the expected number of creds deleted doesn't match the actual number of creds deleted.
There was a problem hiding this comment.
So Is it fine to show them 6 rows of creds when creds command is used and show them 12 creds got deleted while creds -d doesn't it impact the user experience? doesn't the user gets confused that there are only 6 rows but we are 12 rows got deleted?
There was a problem hiding this comment.
@manishkumarr1017 I'm confused what your thinking happens here. I was under the impression that framework.db.delete_credentials may not necessarily delete all the credentials that you pass to it. Therefore you should get the result of this, aka result.size and report that as the number of successfully deleted entries.
There was a problem hiding this comment.
Ok. I understood your concern. but we are showing the number of deleted creds with the end users right? So let's take one sample output.
Credentials
===========
host origin service public private realm private_type JtR Format cracked_password
---- ------ ------- ------ ------- ----- ------------ ---------- ----------------
oracle12c_epsilon H:DC9894A01797D91D92ECA1DA66242209;T:E3243B98974159CC24FD2C9A8B30BA62E0E83B6CA2FC7C5517 (TRUNCATED) Nonreplayable hash pbkdf2,oracle12c epsilon
SYSTEM 9EEDFA0AD26C6D52 Nonreplayable hash des,oracle THALES
example md5be86a79bf2043622d58d5453c47d4860 Postgres md5 raw-md5,postgres password
simon 4F8BC1809CB2AF77 Nonreplayable hash des,oracle A
DEMO S:8F2D65FB5547B71C8DA3760F10960428CD307B1C6271691FC55C1F56554A;H:DC9894A01797D91D92ECA1 (TRUNCATED) Nonreplayable hash raw-sha1,oracle epsilon
oracle11_epsilon S:8F2D65FB5547B71C8DA3760F10960428CD307B1C6271691FC55C1F56554A;H:DC9894A01797D91D92ECA1 (TRUNCATED) Nonreplayable hash raw-sha1,oracle epsilon
[*] Deleted 12 creds
So as you can see there are only 6 rows but it shows the 12 creds are deleted. Does it not confuse the end users? I understood that this might be helpful for us to debug but I feel it certainly confuses the end users.
| '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', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, Sorry I thought I should add specs for this new functionality in the next separate PR. Ok So I will add them.
There was a problem hiding this comment.
I added the specs which tests the new functionality.
|
Alright so tried to do some light testing but getting errors about no applicable hashes in database to crack: Also as for readding in the hash it seems to work for normal non-replayable hashes but not for the NTLM hashes despite it being removed from the database. I can't see this touching any existing changes at the moment though so this may be a separate bug: |
| cracked_hash_origin = Metasploit::Credential::Origin::CrackedPassword.find_by(metasploit_credential_core_id: core.id) | ||
| cracked_password_core = query.find_by(origin_id: cracked_hash_origin.id, origin_type: "Metasploit::Credential::Origin::CrackedPassword") if cracked_hash_origin.present? | ||
| cracked_cred_ids << cracked_password_core.id if cracked_password_core.present? | ||
| cracked_password_val = cracked_password_core.present? ? cracked_password_core.private.data : '' |
There was a problem hiding this comment.
This is very expensive way to determine any matching cracked origins, this runs a new database query for each credential.
Consider reworking this to have the result of filtered_query also return additional origins.
There was a problem hiding this comment.
Ok So you want me to return the cracked passwords from the filtered_query method? Can you please suggest me any way to optimize this? I too thought about this but unable to do at that time. I will try it once again.
There was a problem hiding this comment.
One idea might be to enhance the query to group the related cores/origins together and then yield results only when when finished processing all related cores/origins.
I can look into this in more detail, however it will be a couple days. Please take pass at the idea and see what you come up with.
There was a problem hiding this comment.
I have tried to optimize it using the joins and I have returned the cracked passwords from the filtered_query method.
|
@gwillcox-r7 when adding a hash you need to specify the jtr_type, or else none of the cracker modules know what that hash is and that they should try to crack it. https://docs.metasploit.com/docs/using-metasploit/intermediate/hashes-and-password-cracking.html#example-hashes has a command to add a bunch of hashes in the right format. |
Thanks appreciated, I'll wait until Jeffrey's comment here is resolved to retest as I think that will entail a redesign of how this operates, but I'll definitely use this to test once the changes are applied. |
|
@manishkumarr1017 any updates on this? Just wanted to check in on it as it will make testing of my PR MUCH easier. |
I apologise I was bit busy due to some personal work. I will update this PR today. I apologise for the delay. I updated the PR. |
|
Any updates on when this can be reviewed? Looks like its been re-ready for 3 weeks now, and its holding up my rewrite of the password crackers from february |
|
@jmartin-r7 I'm happy to review this once you have reviewed the implementation on your point. Just let me know when your ready and I can give this a further run though. @h00die See above, sorry this has been delayed for a while. I'm trying to resolve what issues are related to comments I have left but I imagine Jeffrey will need to do a further checking before this gets landed since this might impact Metasploit Pro. |
|
@gwillcox-r7 I need to look at the query implementation here, based on a quick review I think the current query code needs some improvement. I should get to this in the next couple days. |
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah sure I will change it to empty string. I Apologise that I didn't notice this comment.
…work into add_new_column_in_creds
|
please let me know if it requires any more changes. |
There was a problem hiding this comment.
This is still a great pass at getting a more clear idea of passwords that have been cracked however this introduces code that does not account for the various ways the console may interact with a data service.
Use of the Metasploit Data Service while not highly adopted is a standard and it is still supported. Breaking a core command like creds should be avoided.
This example is a but convoluted to avoid actually cracking a credential but shows how the error would occur:
msf6 > db_status
[*] Connected to remote_data_service: (https://localhost:5443). Connection type: http. Connection name: local-https-data-service.
msf6 > creds add user:postgres postgres:md5be86a79bf2043622d58d5453c47d4860
msf6 > creds add user:other hash:d19c32489b870735b5f587d76b934283 jtr:md5
msf6 > creds add user:postgres postgres:md55f4dcc3b5aa765d61d8327deb882cf99
msf6 > irb
[*] Starting IRB shell...
[*] You are in the "framework" object
irb: warn: can't alias jobs from irb_jobs.
>> my_core = Metasploit::Credential::Core.all.first
>> db.create_cracked_credential( username: my_core.public.username, password: 'password', core_id: my_core.id )
=>
#<Metasploit::Credential::Core:0x00005581c67a4908
id: 4,
origin_type: "Metasploit::Credential::Origin::CrackedPassword",
origin_id: 1,
private_id: 4,
public_id: 1,
realm_id: nil,
workspace_id: 1,
created_at: 2023-06-05 16:04:58.33 UTC,
updated_at: 2023-06-05 16:04:58.33 UTC,
logins_count: 0>
>> exit
msf6 > creds
[-] Error while running command creds: undefined method `find_by' for [#<Metasploit::Credential::Core id: 4, origin_type: "Metasploit::Credential::Origin::CrackedPassword", origin_id: 1, private_id: 4, public_id: 1, realm_id: nil, workspace_id: 1, created_at: "2023-06-05 16:04:58.330000000 +0000", updated_at: "2023-06-05 16:04:58.330000000 +0000", logins_count: 0>, #<Metasploit::Credential::Core id: 3, origin_type: "Metasploit::Credential::Origin::Import", origin_id: 1, private_id: 3, public_id: 1, realm_id: nil, workspace_id: 1, created_at: "2023-06-05 16:04:46.018000000 +0000", updated_at: "2023-06-05 16:04:46.018000000 +0000", logins_count: 0>, #<Metasploit::Credential::Core id: 1, origin_type: "Metasploit::Credential::Origin::Import", origin_id: 1, private_id: 1, public_id: 1, realm_id: nil, workspace_id: 1, created_at: "2023-06-05 16:04:45.902000000 +0000", updated_at: "2023-06-05 16:04:45.902000000 +0000", logins_count: 0>, #<Metasploit::Credential::Core id: 2, origin_type: "Metasploit::Credential::Origin::Import", origin_id: 1, private_id: 2, public_id: 2, realm_id: nil, workspace_id: 1, created_at: "2023-06-05 16:04:45.959000000 +0000", updated_at: "2023-06-05 16:04:45.959000000 +0000", logins_count: 0>]:Array
Did you mean? min_by
Call stack:
/vagrant/lib/msf/ui/console/command_dispatcher/creds.rb:556:in `block in filtered_query'
/vagrant/lib/msf/ui/console/command_dispatcher/creds.rb:554:in `each'
/vagrant/lib/msf/ui/console/command_dispatcher/creds.rb:554:in `filtered_query'
/vagrant/lib/msf/ui/console/command_dispatcher/creds.rb:462:in `creds_search'
/vagrant/lib/msf/ui/console/command_dispatcher/creds.rb:82:in `cmd_creds'
/vagrant/lib/rex/ui/text/dispatcher_shell.rb:581:in `run_command'
/vagrant/lib/rex/ui/text/dispatcher_shell.rb:530:in `block in run_single'
/vagrant/lib/rex/ui/text/dispatcher_shell.rb:524:in `each'
/vagrant/lib/rex/ui/text/dispatcher_shell.rb:524:in `run_single'
/vagrant/lib/rex/ui/text/shell.rb:168:in `run'
/vagrant/lib/metasploit/framework/command/console.rb:48:in `start'
/vagrant/lib/metasploit/framework/command/base.rb:82:in `start'
./msfconsole:23:in `<main>'
msf6 >
Note that even with the suggestions I have added in this review the notes about find_by usage and calls to second layer rails objects in my examples still to not resolve this concern.
| elsif core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) | ||
| core = framework.db.creds.find_by(id: core.origin.metasploit_credential_core_id) |
There was a problem hiding this comment.
| 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.
| elsif core.origin.kind_of?(Metasploit::Credential::Origin::CrackedPassword) | |
| core = framework.db.creds.find_by(id: core.origin.metasploit_credential_core_id) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
yeah updated the file with the suggested changes.
| @@ -572,11 +582,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) | |||
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah updated the file with the suggested changes.
| @@ -549,6 +552,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? | |||
There was a problem hiding this comment.
| 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? |
There was a problem hiding this comment.
yeah added the comment but is there anyway we can handle the web json service?
manishkumarr1017
force-pushed
the
add_new_column_in_creds
branch
from
2e5718d
to
87582ee
Compare
|
Updated the PR. Is there anyway handle json web service? |
|
Unfortunately the json web service does not yet provide endpoints that would enable this code. The connection between different origins to match up Further enhancement of the proxy layer is needed to make this code full compatible with different modes of database connection. One lower effort way forward would be to make this change a conditional coded path that only enhances the table when using a direct database connection, however that creates a new user experience problem when the same command can have differing output based on the how the database is being accessed and increases the maintenance burden on the short term. If made conditional I would suggest we may want to have rspec that documents the different expected behaviors. |
|
Any updates on this? We're nearing the 6 month mark |
|
Let me know if you need any more changes for this. |
|
Checking in on this again since its been a month |
|
Will try to pick this up in the next sprint 👍 |
|
As noted here there are issues related to interactions if not connected directly to postgres for the database services. This needs to be accounted for either by enhancing the proxy layer or allowing a different experience in the console UI depending on the type of database service connected. |
|
Thanks for the PR @manishkumarr1017! 👍 I'll create a follow up PR to get this working with the remote database 👍 |
Release NotesAdds an additional column to the |
… passwords
Fixes #17639 where the ordering of the creds command was not proper. So as suggested by @h00die I have added an another column known as
cracked_passwordto show all the cracked passwords.Verification
List the steps needed to make sure this thing works
msfconsoleuse auxiliary/analyze/crack_databasesAdd some creds using creds add commandcrack some credscredsto display all the credsHere is the sample of the current order when I added the same creds present in the issue.
After adding the column and repeating the same steps as above.
msfconsoleuse auxiliary/analyze/crack_databasesAdd some creds using creds add commandcrack some credscredsto display all the credsHere is the sample output for above data
But there is a problem. When we use
creds -dfor deleting the creds. It shows the number of records that got deleted. But now as we reduced the number of rows which we are showing, so we have to show the right amount of records that got deleted from UI rather than showing all the records got deleted. So the deleted count which we show to UI will get changed.Here is the Sample. Previously
msfconsoleuse auxiliary/analyze/crack_databasesAdd some creds using creds add commandcrack some credscredsto display all the credscreds -dCurrently.
msfconsoleuse auxiliary/analyze/crack_databasesAdd some creds using creds add commandcrack some credscredsto display all the credscreds -dIt just shows 6 records got deleted but it deletes all the 12 records present.