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.

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

smb.lua: fix SMB Extended Security parsing by resetting the offset index #1476

Closed
wants to merge 1 commit into from

Conversation

@cnotin
Copy link

commented Feb 16, 2019

The SMB library (smb.lua) fails to properly parse "Negotiate Protocol Response" messages when SMB Extended Security is enabled. This cause many SMB scripts to fail, in particular against Samba servers based on my observations in real networks. Below are some observations with smb-os-discovery, but I've also noticed that with smb-vuln-ms17-010 which cause it to not run since it generates an exception in the early phase.

Example before the patch and with the latest public Nmap 7.70 release:

# nmap -p445 --script smb-os-discovery a.b.c.d -vv -dd
Starting Nmap 7.70 ( https://nmap.org ) at ....
[...]
NSE: [smb-os-discovery M:55db8b26fb38 a.b.c.d] Couldn't negotiate a SMBv1 connection:SMB: ERROR: Server returned less data than it was supposed to (one or more fields are missing); aborting [12]
Host script results:
| smb-os-discovery: 
|   OS: Unix (Samba 3.0.20b)
|   NetBIOS computer name: 
|   Workgroup: WORKGROUP\x00
|_  System time: 2019-02-14T07:43:25+01:00

Example before the patch and with the latest Nmap version from Git as of now. The error is more visible:

# nmap -p445 --script smb-os-discovery a.b.c.d -vv -dd
Starting Nmap 7.70SVN ( https://nmap.org ) at ...
[...]
NSE: smb-os-discovery M:55fe86116508 against a.b.c.d threw an error!
/root/tools/nmap/nselib/smb.lua:1030: bad argument #2 to 'unpack' (data string too short)
stack traceback:
	[C]: in function 'string.unpack'
	/root/tools/nmap/nselib/smb.lua:1030: in function 'smb.negotiate_v1'
	/root/tools/nmap/nselib/smb.lua:1074: in function 'smb.negotiate_protocol'
	/root/tools/nmap/nselib/smb.lua:372: in function 'smb.start_ex'
	/root/tools/nmap/nselib/smb.lua:3363: in function 'smb.get_os'
	/root/tools/nmap/scripts/smb-os-discovery.nse:152: in function </root/tools/nmap/scripts/smb-os-discovery.nse:149>
	(...tail calls...)

This difference seems to be caused by the change from bin.unpack to string.unpack but this is just a symptom, not the problem itself.

pos, smb['server_guid'] = bin.unpack("<A16", data, pos)

And after the patch, no error. The results aren't very good, but it is an unrelated issue:

Host script results:
| smb-os-discovery: 
|   OS: Unix (Samba 3.0.20b)
|   Computer name: A19FP-REDACTED
|   NetBIOS computer name: 
|   Domain name: 
|   FQDN: A19FP-REDACTED
|_  System time: 2019-02-14T07:43:57+01:00

My detailed thoughts are the following.
This section of the function negotiate_v1 parses the "Negotiate Protocol Response" message, where SMB Extended Security is true.
In this case, as explained in [MS-SMB], the response has two parts: parameters and data.
These are returned by smb_read():

local status, header, parameters, data = smb_read(smb)

I understand that the pos variable is used as an offset index. It is first used against parameters, like:
smb['dialect'], pos = string.unpack(dialect_format, parameters)

But then it is reused against data without being reset to 1. Which creates a read problem.

smb.server_guid, pos = string.unpack("<c16", data, pos)

The patch creates a new index data_pos dedicated to parse this data array.

I've confirmed this by adding print statements to view the variables values along the way.
With the patch, the smb.server_guid is properly filled which wasn't the case before:

smb.server_guid: a19fp-REDACTED

Wireshark parsing, if that helps:
image

I've tested against hosts that didn't show this problem (no Extended Security in this Negotiate Protocol Response) and I didn't observe any regression.

@cnotin

This comment has been minimized.

Copy link
Author

commented Sep 2, 2019

PR effectiveness confirmed by:
#1707 (comment)

@nnposter

This comment has been minimized.

Copy link

commented Sep 8, 2019

A fix for this issue has been committed as r37733. Thank you for contributing to nmap by researching the code discrepancy and proposing a patch.

P.S. I agree with your assessment of what roughly needs to be changed but your rationale is somewhat off. The root cause is a spec non-compliance inside Samba. For compliant Samba versions and for Windows the current code seems to be working just fine.

In particular,

I understand that the pos variable is used as an offset index. It is first used against parameters ... but then it is reused against data without being reset to 1. Which creates a read problem.

Variable pos is in fact getting reset be the following line:

smb.server_challenge, pos = string.unpack(string.format("<c%d", smb['key_length']), data)

Under compliant circumstances, smb['key_length'] should be zero, which results in pos being set to 1.

You can see the Samba non-compliance in your Wireshark screenshot. Here smb['key_length'] is 58, same as BCC. This causes the entire data block to be slurped into smb.server_challenge, leaving pos pointing just past it.

The true fix is to completely skip extracting smb.server_challenge when Extended Security is in effect.

@nnposter nnposter assigned nnposter and unassigned cldrn Sep 8, 2019

@nnposter nnposter added the bug label Sep 8, 2019

@nmap-bot nmap-bot closed this in c491143 Sep 8, 2019

@cnotin

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

Thanks for your thorough review and for fixing it :)
I'll re-try later against the non-compliant host, with this code, to confirm.

@cnotin cnotin deleted the cnotin:patch-2 branch Sep 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.