Add namespace to identify.rb

[k0pak4]

This change addresses Issue 16515 to namespace the identify.rb file

There are a lot of small changes, mostly to include the new module after requiring it. I thought about changing it at a higher level for some (e.g. the Wordpress helper since like 10+ wordpress modules use it) but wasn't sure if the team had a specific location that might make more sense. It can also just be the way I changed it here, or any other suggested locations. There also were inconsistencies on where to put the require line (in the class, above it, top of file, etc.) I am happy to change it to what the current standard is, just let me know!

Verification

Load and attempt to run any of the modified modules. I searched for the old require line in order to find all affected modules. I absolutely would appreciate a second check on that, in case one was missed. There are tests that verify the module usage already, since they were failing when I first did not include it correctly.

[jmartin-tech]

Namespace wise I think this may read better to move the file and set the namespace as Metasploit::Framework::Hashes.

Also as an optional but preferred patten, instead of including the modules I would prefer to see the methods and constants accessed directly:

Metasploit::Framework::Hashes::JTR_NTLMV1
Metasploit::Framework::Hashes::JTR_NTLMV2

Metasploit::Framework::Hashes.identify_hash(hash_value)

I think the quantity of file changes would still be similar, so more a preference than required guidance.

[k0pak4]

Namespace wise I think this may read better to move the file and set the namespace as Metasploit::Framework::Hashes.

Also as an optional but preferred patten, instead of including the modules I would prefer to see the methods and constants accessed directly:

Metasploit::Framework::Hashes::JTR_NTLMV1
Metasploit::Framework::Hashes::JTR_NTLMV2

Metasploit::Framework::Hashes.identify_hash(hash_value)

I think the quantity of file changes would still be similar, so more a preference than required guidance.

This makes a lot of sense to me, it seems more resilient to be able to add more utility to the file if needed at a later date. I can definitely move it up one level and rename it hashes.rb instead of identity.rb. I'll take a look at referencing the method directly also instead of the module level include

[k0pak4]

@jmartin-r7 I've made the requested changes. I searched for the JTR constants being used but didn't find any, and this should contain all the files that were previously using identify_hash. Anywhere calling identify_hash should be accessed directly now rather than an include

[jmartin-tech]

Thanks for adjusting.

I see a few usages of the constants in the following files:

metasploit-framework/lib/msf/core/exploit/remote/smb/server/hash_capture.rb

Lines 47 to 59 in 49f3e0d

	jtr_format = JTR_NTLMV1
	when :ntlmv2
	hash_type = 'NTLMv2-SSP'
	client_hash = "#{bin_to_hex(ntlm_message.ntlm_response[0...16])}:#{bin_to_hex(ntlm_message.ntlm_response[16..-1])}"
	combined_hash << ":#{bin_to_hex(challenge)}"
	combined_hash << ":#{client_hash}"
	jtr_format = JTR_NTLMV2
	end
	return if hash_type.nil?
	jtr_format = ntlm_message.ntlm_version == :ntlmv1 ? JTR_NTLMV1 : JTR_NTLMV2

metasploit-framework/modules/auxiliary/server/capture/mssql.rb

Lines 234 to 251 in 49f3e0d

	smb_db_type_hash = JTR_NTLMV1
	capturelogmessage =
	"#{capturedtime}\nNTLMv1 Response Captured from #{host} \n" +
	"DOMAIN: #{domain} USER: #{user} \n" +
	"LMHASH:#{lm_hash_message ? lm_hash_message : "<NULL>"} \nNTHASH:#{nt_hash ? nt_hash : "<NULL>"}\n"
	when NTLM_CONST::NTLM_V2_RESPONSE
	smb_db_type_hash = JTR_NTLMV2
	capturelogmessage =
	"#{capturedtime}\nNTLMv2 Response Captured from #{host} \n" +
	"DOMAIN: #{domain} USER: #{user} \n" +
	"LMHASH:#{lm_hash_message ? lm_hash_message : "<NULL>"} " +
	"LM_CLIENT_CHALLENGE:#{lm_chall_message ? lm_chall_message : "<NULL>"}\n" +
	"NTHASH:#{nt_hash ? nt_hash : "<NULL>"} " +
	"NT_CLIENT_CHALLENGE:#{nt_cli_challenge ? nt_cli_challenge : "<NULL>"}\n"
	when NTLM_CONST::NTLM_2_SESSION_RESPONSE
	#we can consider those as netv1 has they have the same size and i cracked the same way by cain/jtr
	#also 'real' netv1 is almost never seen nowadays except with smbmount or msf server capture
	smb_db_type_hash = JTR_NTLMV1

[jmartin-tech]

This looks good to me although I would prefer to see this squashed to a smaller number of commits.

@cdelafuente-r7 you logged the original issue, any thoughts to add?

[k0pak4]

@jmartin-r7 I squashed everything post review to a single commit. I wasn't sure if metasploit-framework uses squash commits on PR merges, but this at least makes it a bit more easy to read

[jmartin-tech]

This is better and based on the number of files changed seems reasonable. We merge in as no-ff when landing so the commit history for high count PRs can be noisy at times.

[k0pak4]

@jmartin-r7 thanks for the help and the details on how it's merged. I'll keep that in mind for future PRs to squash first 🤙

[cdelafuente-r7]

I found a couple of instances that still need to be updated:

metasploit-framework/modules/post/android/gather/hashdump.rb

Line 147 in d348601

jtr_format: identify_hash(md5),

metasploit-framework/modules/auxiliary/gather/suite_crm_export_sqli.rb

Line 179 in d348601

jtr_format: identify_hash(hash),

Also, @jmartin-r7 , I'm wondering if this mixin should be autoloaded when MSF starts? I noticed that any classes in lib/msf/core/auxiliary/ get autoloaded anyway and some of them already load metasploit/framework/hashes.

[cdelafuente-r7]

Just a heads up: this class does not require metasploit/framework/hashes and I believe it does not break because this module is already autoloaded in one of the lib/msf/core/auxiliary/ classes.

[jmartin-tech]

@cdelafuente-r7 lib/metasploit/framework is not yet ready for zeitwerk autoloading I tested this idea when I first started reviewing this PR, requires will have to do for now. We could consider shifting to an early require since this is light weight and loaded so often, given that there are already lib/msf/core classes requiring this we could consider finding a common location that makes sense until the lib/metasploit/framework paths are ready for full autoloading.

[cdelafuente-r7]

@jmartin-r7 , thanks for the explanation. In this case, I don't think it is worth blocking this PR for this. I'm good to land it as it is right now.

[cdelafuente-r7]

Thanks for your contribution @k0pak4. Everything looks good to me. I tested a couple of modules and make sure #identify_hash still works as expected. I'll go ahead and land it.

[cdelafuente-r7]

Release Notes

This adds proper namespace to the hash identification library to avoid any potential collision with the constants defined previously.