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 bug in start_session_basic while parsing status #1480

Closed
wants to merge 1 commit into from

Conversation

@cnotin
Copy link

commented Feb 17, 2019

When using the "smb-os-discovery" script against a host which refuses connection with anonymous and guest accounts, and with the latest Nmap version from Git, we have an error:

# nmap -Pn -p445 --script smb-os-discovery -d a.b.c.d
Starting Nmap 7.70SVN ( https://nmap.org ) at ....
[...]
NSE: Starting smb-os-discovery against a.b.c.d.
NSE: [smb-os-discovery a.b.c.d] SMB: Added account '' to account list
NSE: [smb-os-discovery a.b.c.d] SMB: Added account 'guest' to account list
NSE: [smb-os-discovery a.b.c.d] SMB: Login as \guest failed (NT_STATUS_ACCESS_DENIED)
NSE: [smb-os-discovery a.b.c.d] SMB: Login as \<blank> failed (NT_STATUS_ACCESS_DENIED)
NSE: [smb-os-discovery a.b.c.d] SMB: ERROR: No accounts left to try
NSE: smb-os-discovery against a.b.c.d threw an error!
/root/tools/nmap/nselib/smb.lua:202: bad argument #2 to 'format' (number expected, got boolean)
stack traceback:
	[C]: in function 'string.format'
	/root/tools/nmap/nselib/smb.lua:202: in function 'smb.get_status_name'
	/root/tools/nmap/nselib/smb.lua:1285: in upvalue 'start_session_basic'
	/root/tools/nmap/nselib/smb.lua:1567: in function 'smb.start_session'
	/root/tools/nmap/nselib/smb.lua:380: in function 'smb.start_ex'
	/root/tools/nmap/nselib/smb.lua:3337: 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...)

After adding a few print, I understood that there was a confusion around status since in the same function it's used for two purposes:

  • getting the status result after calling smb_read, as a boolean:

    nmap/nselib/smb.lua

    Lines 1196 to 1199 in 1650469

    status, header, parameters, data = smb_read(smb)
    if(status ~= true) then
    return false, header
    end
  • getting the status from the SMB header of the received responses, as an integer (and meant to be mapped to common codes such as NT_ACCESS_DENIED later by smb.get_status_name):
    local protocol_version, command, status, flags, flags2, pid_high, signature, unused, tid, pid, uid, mid, pos = string.unpack(header_format, header)

After the patch, we have:

NSE: Starting smb-os-discovery against a.b.c.d.
NSE: [smb-os-discovery a.b.c.d] SMB: Added account '' to account list
NSE: [smb-os-discovery a.b.c.d] SMB: Added account 'guest' to account list
NSE: [smb-os-discovery a.b.c.d] SMB: Login as \guest failed (NT_STATUS_ACCESS_DENIED)
NSE: [smb-os-discovery a.b.c.d] SMB: Login as \<blank> failed (NT_STATUS_ACCESS_DENIED)
NSE: [smb-os-discovery a.b.c.d] SMB: ERROR: No accounts left to try
NSE: Finished smb-os-discovery against a.b.c.d.
[...]

Host script results:
| smb-os-discovery: 
|_  ERROR: No accounts left to try

("No accounts left to try" comes from the smbauth lib)

The error displayed is different from what we see with the latest 7.70 release:

Starting Nmap 7.70 ( https://nmap.org ) at 
[...]
NSE: Starting smb-os-discovery against a.b.c.d.
NSE: [smb-os-discovery a.b.c.d] SMB: Added account '' to account list
NSE: [smb-os-discovery a.b.c.d] SMB: Added account 'guest' to account list
NSE: [smb-os-discovery a.b.c.d] LM Password: 
NSE: [smb-os-discovery a.b.c.d] SMB: Login as \guest failed (NT_STATUS_ACCESS_DENIED)
NSE: [smb-os-discovery a.b.c.d] SMB: Login as \<blank> failed (NT_STATUS_ACCESS_DENIED)
NSE: [smb-os-discovery a.b.c.d] SMB: ERROR: No accounts left to try
NSE: Finished smb-os-discovery against a.b.c.d.
[...]

Host script results:
| smb-os-discovery: 
|_  ERROR: NT_STATUS_ACCESS_DENIED

This error was better but I'm wondering if it was intended or if it is thanks to a lucky bug. We have a loop which tries to connect with two accounts, here the status is the same for both (NT_STATUS_ACCESS_DENIED) but it could have been different. And with this code, we only would see the status code for the last try, not the previous.

FYI, git bisect tells me that the difference between the latest version from source and the latest 7.70 release, comes from fd86015

@cnotin

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

Alternative solution for same issue in #1714

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

@nnposter

This comment has been minimized.

Copy link

commented Sep 6, 2019

A fix for this issue has been committed as r37730.

Please note that your fix does not properly rectify the issue: By replacing the status variable with another one, smb_read_status, while leaving the inner status intact, you are causing the outer status to stay unassigned. On one hand, this prevents the error by assuring that line

return false, get_status_name(status)

never gets executed but it also means that the real status, captured in the inner status variable, is not returned back to the caller.

Thank you for contributing to nmap!

@nmap-bot nmap-bot closed this in ce28753 Sep 6, 2019

@cnotin cnotin deleted the cnotin:patch-6 branch Sep 6, 2019

@cnotin

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

Thanks for your feedback!
I agree that the fix in r37730 is valid.

I chose to use a different variable because IMO there's a confusion risk since status is used first as a boolean to get the return from smb_read:

    -- Read the result
    status, header, parameters, data = smb_read(smb)
    if(status ~= true) then
      return false, header
    end

Then it's used to collect integer return codes that are mapped with get_status_name

Anyway, if the return status from smb_read is not true, then the status returned is false:

    if(status ~= true) then
      return false, header

I don't see how the caller could get something else or miss the real status in this case. Especially considering that status was never set to something else prior to this line where the function returns.
I don't know Lua very well I might miss something :)

@nnposter

This comment has been minimized.

Copy link

commented Sep 6, 2019

Choosing a different variable is fine, as it prevents confusion when the same variable is used for different purposes. No dispute there.

If you still do not see where your fix is falling short then ask yourself the following question: With your fix, is the function ever going to execute line

return false, get_status_name(status)

that is outside of the while-loop and correctly interpret the numerical code collected inside the loop?

@cnotin

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

No it won't be executed because the following return, inside the loop, will return immediately and exit the loop and the function:

nmap/nselib/smb.lua

Lines 1198 to 1201 in ce28753

-- Read the result
status, header, parameters, data = smb_read(smb)
if(status ~= true) then
return false, header

Even before it has a chance of collecting a return code in status and reaching what you mentioned:

nmap/nselib/smb.lua

Lines 1288 to 1289 in ce28753

if (status ~= nil) then
return false, get_status_name(status)

But according to me it's the same with r37730, isn't it?

If it would have been a break, to exit the loop, instead of return, then fine. But even then, I don't think that get_status_name would be able to deal with the case where status=false 😕
(Just curious, no worries if you have more important stuff to do :))

@cnotin

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

(doing some more tests... :))

@nnposter

This comment has been minimized.

Copy link

commented Sep 6, 2019

Consider the consequences of your fix causing the outer status never to be assigned a value

@nnposter

This comment has been minimized.

Copy link

commented Sep 6, 2019

But according to me it's the same with r37730, isn't it?

If it would have been a break, to exit the loop, instead of return, then fine. But even then, I don't think that get_status_name would be able to deal with the case where status=false

Show me that r37730 will ever have status==false outside of the loop.

@cnotin

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

Okayyy I got it :) I was misinterpreting something... Thank you for insisting!

I look forward to getting a review from your keen eyes on my other SMB PRs!

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.