Add error handing to rdp_web_login.py to handle malformed NetNTLM messages

[NeffIsBack]

As mentioned in #15720 with some NTLM responses the modules/auxiliary/scanner/http/rdp_web_login.py module crashes, unable to enumerate the domain. To prevent this behavior a try and except block was added, to inform the user about the crash.

Debug output from above mentioned crash:

[09/30/2021 13:02:40] [e(0)] core: Unexpected output running /usr/share/metasploit-framework/modules/auxiliary/scanner/http/rdp_web_login.py:
Traceback (most recent call last):
  File "/usr/share/metasploit-framework/modules/auxiliary/scanner/http/rdp_web_login.py", line 197, in <module>
    module.run(metadata, run)
  File "/usr/share/metasploit-framework/lib/msf/core/modules/external/python/metasploit/module.py", line 122, in run
    ret = callback(args)
  File "/usr/share/metasploit-framework/modules/auxiliary/scanner/http/rdp_web_login.py", line 171, in run
    domain = get_ad_domain(args['rhost'], args['rport'], user_agent)
  File "/usr/share/metasploit-framework/modules/auxiliary/scanner/http/rdp_web_login.py", line 98, in get_ad_domain
    domain = base64.b64decode(bytes(domain_hash, 'utf-8')).replace(b'\x00',b'').split(b'\n')[1]
IndexError: list index out of range

[gwillcox-r7]

This looks good but my one concern is the fact that this is going to silently handle the exception without informing the user that an error occurred. This may lead to confusion if the user expected something to work and then all of a sudden now it doesn't work but they are provided no information as to why their attempt might have failed.

[gwillcox-r7]

Changing PR title again as this PR presently doesn't add in any additional error messages, and mainly serves to just provide additional error handling.

[gwillcox-r7]

Looking at this closer I'm wondering if there shouldn't just be more validation done on the actual values. The error you posted suggests that an index went out of range and looking at the code it seems to assume that quite a few values won't be empty or that it will be formatted correctly.

Rather than adding input validation I think it may be better to address the root issue here. I'll see if I can cook something up.

[NeffIsBack]

Changing PR title again as this PR presently doesn't add in any additional error messages, and mainly serves to just provide additional error handling.

Correct me if I'm wrong but now the user should be confronted with an error message if an error occurs during the NTML hash handling.
Instead of silently crashing, the error message

module.log('Failed to find Domain', level='error')
return None

should be presented, resulting in stopping the script.

Currently i got no testing environment, which would provide me additional NTML hashes, besides the one that crashes. If you got more NTML hashes could you perhaps give me functioning hashes, in order to track down the problem and maybe add a fix to the crash?

[NeffIsBack]

Looking at this closer I'm wondering if there shouldn't just be more validation done on the actual values. The error you posted suggests that an index went out of range and looking at the code it seems to assume that quite a few values won't be empty or that it will be formatted correctly.

Rather than adding input validation I think it may be better to address the root issue here. I'll see if I can cook something up.

Yes i am pretty sure it doesn't find a \n in my hash and therefore crashes at the last split, as mentioned in #15717. If you can provide me with more hashes i can try to track down the problem and add a fix.

[NeffIsBack]

As a couldn't fix the problem with my given resources i at least tried to provide an error message in order to inform the user about the (until now) silent crash.

[gwillcox-r7]

Changing PR title again as this PR presently doesn't add in any additional error messages, and mainly serves to just provide additional error handling.

Correct me if I'm wrong but now the user should be confronted with an error message if an error occurs during the NTML hash handling. Instead of silently crashing, the error message

module.log('Failed to find Domain', level='error')
return None

should be presented, resulting in stopping the script.

Currently i got no testing environment, which would provide me additional NTML hashes, besides the one that crashes. If you got more NTML hashes could you perhaps give me functioning hashes, in order to track down the problem and maybe add a fix to the crash?

That isn't a great message imho. Keep in mind your within a loop so you wouldn't stop, you would just go on to the next execution of that loop. There are multiple URLs that you are processing. How do you know which one caused the error? Unfortunately I don't have additional hashes that would trigger this error but I don't think this is needed in this case; its a simple enough piece of code that it can likely be fixed by reading the source code and fixing some edge cases that seem to be apparent.

[gwillcox-r7]

Looking at this closer I'm wondering if there shouldn't just be more validation done on the actual values. The error you posted suggests that an index went out of range and looking at the code it seems to assume that quite a few values won't be empty or that it will be formatted correctly.
Rather than adding input validation I think it may be better to address the root issue here. I'll see if I can cook something up.

Yes i am pretty sure it doesn't find a \n in my hash and therefore crashes at the last split, as mentioned in #15717. If you can provide me with more hashes i can try to track down the problem and add a fix.

Yeah I'll see if I can add some code to fix this up. My guess is that the code should be checking certain elements aren't NULL or will at the very least contain the right format of data before proceeding and right now it just assumes responses are formatted correctly. What should be happening is that we have a series of steps for validating the data and then print out more specific error messages if one of those steps fails.

[k0pak4]

@NeffIsBack I actually got the original NTLM hash parsing idea from the auxiliary owa_login.rb module. It might have a bit more protection than mine given the NTLM parsing it uses, but if you have an environment that produces this crash, it could be helpful to check if that module crashes on the enum_domain or not also 🙂

[smcintyre-r7]

I have not tested this, but what about using a method that will parse the NTLM type 1 message. Something along the lines of:

import struct

def parse_ntlm_t1(message):
    if not message.startswith(b'NTLMSSP\x00\x01\x00\x00\x00'):
        return None
    struct_fmt = '<8xIIHHIHHI'
    size = struct.calcsize(struct_fmt)
    if len(message) < size:
        return None
    fields = struct.unpack(struct_fmt, message[:size])
    
    properties = {'type': fields[0], 'flags': fields[1]}
    
    length, _, offset = fields[2:5]
    if len(message) < offset + length:
        return None
    properties['domain'] = message[offset:offset+length].decode('utf-8')
    
    length, _, offset = fields[5:8]
    if len(message) < offset + length:
        return None
    properties['workstation'] = message[offset:offset+length].decode('utf-8')
    return properties

Then

Suggested change

	try:
	ntlm_header_array = request.headers['WWW-Authenticate'].split('NTLM ')
	if len(ntlm_header_array) < 2:
	module.log("NTLM authenticate header was not in the expected format for %s" % url)
	continue
	domain_hash = ntlm_header_array[1].split(',')[0]
	domain = base64.b64decode(bytes(domain_hash, 'utf-8')).replace(b'\x00',b'').split(b'\n')
	if len(domain) < 2:
	module.log("Domain is not in the right format, skipping processing %s" % url)
	continue
	domain = domain[1]
	start = domain.find(b'\x0f')
	end = domain.find(b'\x02')
	if (start == -1) or (end == -1):
	module.log("Couldn't find byte 0xf or 0x2 in the request, skipping processing %s", url)
	continue
	domain = domain[start + 1:end].decode('utf-8')
	module.log('Found Domain: {}'.format(domain), level='good')
	return domain
	except:
	module.log("An unhandled exception occured when processing %s. Please file a bug report." %url)
	continue
	type1_msg = request.headers['WWW-Authenticate'].split('NTLM ')[1].split(',')[0]
	type1_msg = parse_ntlm_t1(base64.b64decode(type1_msg))
	if type1_msg is None:
	module.log("NTLM authenticate header was not in the expected format for %s" % url)
	continue
	return type1_msg['domain']

[NeffIsBack]

@NeffIsBack I actually got the original NTLM hash parsing idea from the auxiliary owa_login.rb module. It might have a bit more protection than mine given the NTLM parsing it uses, but if you have an environment that produces this crash, it could be helpful to check if that module crashes on the enum_domain or not also 🙂

I will have a look at it and test out the module when i get back to work, but can't promise anything. If we could get any working NTLM hashes i could further track down the problem. :)

[smcintyre-r7]

If we could get any working NTLM hashes i could further track down the problem. :)

You can generate a Net-NTLM type 1 message with Metasploit, that's what I did. Start msfconsole and drop into the Pry debug console using the pry command. Here's two examples:

Generate one without a domain or workstation:

Rex::Text.encode_base64(Net::NTLM::Message::Type1.new.tap { |msg| msg.domain = ''; msg.workstation = '' }.serialize)

Generate one with a domain and workstation:

Rex::Text.encode_base64(Net::NTLM::Message::Type1.new.tap { |msg| msg.domain = 'LOCALDOMAIN'; msg.workstation = 'LOCALHOST' }.serialize)

[bwatters-r7]

Release Notes

This PR adds more robust NTLM message parsing with better error handling and messaging when pulling out the NTLM hashes.