Handle unknown NTSTATUS in SessionError

[ghost]

When a SessionError is raised during SMB session negotiation, currently unexpected NTSTATUS error codes are not handled properly. If the SessionError exception is catched and logged, a KeyError is thrown when retrieving the string representation of the exception in:

https://github.com/SecureAuthCorp/impacket/blob/d509775976ba37f4eaea630cc511e2fc3b65aba3/impacket/smb.py#L581

The error occurred when receiving an unexpected error code during an SMB login scan using CrackMapExec.

This pull request adds a generic error message when the error code is not contained in the ERROR_MESSAGES dictionary.

[gabrielg5]

Hi @rtpt-lucasvater

Thanks for your PR and sorry late response on this one

I've been checking your changes and the only thing that kept me thinking is that perhaps we may also have that same issue in other parts of the lib

Looking at all references to nt_errors.ERROR_MESSAGES I've seen that many of them are already handling this scenario where the exception could be an unknown one

• impacket/impacket/dcerpc/v5/mgmt.py

  Lines 37 to 44 in 06217f0

  	def __str__( self ):
  	key = self.error_code
  	if key in nt_errors.ERROR_MESSAGES:
  	error_msg_short = nt_errors.ERROR_MESSAGES[key][0]
  	error_msg_verbose = nt_errors.ERROR_MESSAGES[key][1]
  	return 'MGMT SessionError: code: 0x%x - %s - %s' % (self.error_code, error_msg_short, error_msg_verbose)
  	else:
  	return 'MGMT SessionError: unknown error code: 0x%x' % self.error_code

• impacket/impacket/dcerpc/v5/nrpc.py

  Lines 53 to 64 in 06217f0

  	def __str__( self ):
  	key = self.error_code
  	if key in system_errors.ERROR_MESSAGES:
  	error_msg_short = system_errors.ERROR_MESSAGES[key][0]
  	error_msg_verbose = system_errors.ERROR_MESSAGES[key][1]
  	return 'NRPC SessionError: code: 0x%x - %s - %s' % (self.error_code, error_msg_short, error_msg_verbose)
  	elif key in nt_errors.ERROR_MESSAGES:
  	error_msg_short = nt_errors.ERROR_MESSAGES[key][0]
  	error_msg_verbose = nt_errors.ERROR_MESSAGES[key][1]
  	return 'NRPC SessionError: code: 0x%x - %s - %s' % (self.error_code, error_msg_short, error_msg_verbose)
  	else:
  	return 'NRPC SessionError: unknown error code: 0x%x' % (self.error_code)

• impacket/impacket/dcerpc/v5/even.py

  Lines 41 to 48 in 06217f0

  	def __str__( self ):
  	key = self.error_code
  	if key in nt_errors.ERROR_MESSAGES:
  	error_msg_short = nt_errors.ERROR_MESSAGES[key][0]
  	error_msg_verbose = nt_errors.ERROR_MESSAGES[key][1]
  	return 'EVEN SessionError: code: 0x%x - %s - %s' % (self.error_code, error_msg_short, error_msg_verbose)
  	else:
  	return 'EVEN SessionError: unknown error code: 0x%x' % self.error_code

• (there are a couple more)

Perhaps we can implement something similar there to keep it synchronized with the lib

I've also found that this error could be triggered from other sources as well, where no check is being done

• impacket/impacket/krb5/kerberosv5.py

  Lines 710 to 721 in 06217f0

  	def __str__( self ):
  	retString = 'Kerberos SessionError: %s(%s)' % (constants.ERROR_MESSAGES[self.error])
  	try:
  	# Let's try to get the NT ERROR, if not, we quit and give the general one
  	if self.error == constants.ErrorCodes.KRB_ERR_GENERIC.value:
  	eData = decoder.decode(self.packet['e-data'], asn1Spec = KERB_ERROR_DATA())[0]
  	nt_error = struct.unpack('<L', eData['data-value'].asOctets()[:4])[0]
  	retString += '\nNT ERROR: %s(%s)' % (nt_errors.ERROR_MESSAGES[nt_error])
  	except:
  	pass
  	return retString

• impacket/impacket/smbconnection.py

  Lines 988 to 989 in 06217f0

  	def getErrorString( self ):
  	return nt_errors.ERROR_MESSAGES[self.error]

[ghost]

Hi @gabrielg5,

Thanks for the response! I changed the mentioned places to handle the errors in a similar way. However, I currently can't test the changes, so I can't give any guarantees that there are no mistakes on my part. Perhaps you can take a look and see if the changes reflect what you had in mind.

[gabrielg5]

thanks @rtpt-lucasvater!!

[Marshall-Hallenbeck]

@rtpt-lucasvater this broke some stuff in NetExec (Pennyw0rth/NetExec#117), since you are returning a string of the entire error instead of a tuple from nt_errors now.
It doesn't look like we can only print the error status, but instead it prints the entire verbose string?

[mpgn]

Can confirm, it's a big breaking change, removing the hability to only have the error status is a downgrade imo

[ghost]

@Marshall-Hallenbeck @mpgn
I'm not really involved in the impacket development and originally only wanted to fix an error in the handling of unknown NTSTATUS codes that we encountered. @gabrielg5 noticed however that the handling of errors is inconsistent between different protocol implementations in impacket. As far as I can tell, getErrorString returning only a string is what is done in all other similar error classes (for other protocols) in impacket and it is what I would expect from the method name. So I think the change makes sense.

To get the status string you could use the getErrorCode method instead and look the status up in the nt_errors.ERROR_MESSAGES dict. Alternatively, you could directly check the error code instead of the error string.

[Marshall-Hallenbeck]

The whole point of getErrorString was to get it from nt_errors.ERROR_MESSAGES. Having to do that ourselves seems unnecessary since it was already working that way, but I guess we'll change our code to do so. nt_errors itself even says "Ideally all the files should grab the error codes from here (big ToDo)".

In the future it'd be nice if breaking changes like this were tested a little more.