nxc/protocols/smb/database:447 is_credential_local(self, credential_id) blows up

[ajanvrin]

Describe the bug
When executing db.is_credential_local on any id, string or int, local or not, a sqlalchemy stacktrace is produced

To Reproduce
Steps to reproduce the behavior i.e.:
Add this snippet to nxc/netexec.py line 44 (just after "async def start_run(protocol_obj, args, db, targets):

async def start_run(protocol_obj, args, db, targets):
    db.is_credential_local(db.get_credentials()[0][0])

then run anything involving netexec and smb:

poetry run nxc smb showbug --verbose

Resulted in:

Traceback (most recent call last):
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1969, in _exec_single_context
    self.dialect.do_execute(
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/default.py", line 922, in do_execute
    cursor.execute(statement, parameters)
sqlite3.ProgrammingError: Error binding parameter 1: type 'list' is not supported

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/user/gits/NetExec/nxc/netexec.py", line 230, in main
    asyncio.run(start_run(protocol_object, args, db, targets))
  File "/usr/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 654, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/user/gits/NetExec/nxc/netexec.py", line 44, in start_run
    db.is_credential_local(db.get_credentials()[0][0])
  File "/home/user/gits/NetExec/nxc/protocols/smb/database.py", line 453, in is_credential_local
    results = self.conn.execute(q).all()
              ^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/orm/session.py", line 2308, in execute
    return self._execute_internal(
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/orm/session.py", line 2199, in _execute_internal
    result = conn.execute(
             ^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1416, in execute
    return meth(
           ^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/sql/elements.py", line 517, in _execute_on_connection
    return connection._execute_clauseelement(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1639, in _execute_clauseelement
    ret = self._execute_context(
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1848, in _execute_context
    return self._exec_single_context(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1988, in _exec_single_context
    self._handle_dbapi_exception(
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 2344, in _handle_dbapi_exception
    raise sqlalchemy_exception.with_traceback(exc_info[2]) from e
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1969, in _exec_single_context
    self.dialect.do_execute(
  File "/home/user/.cache/pypoetry/virtualenvs/netexec-7SzfTu9v-py3.11/lib/python3.11/site-packages/sqlalchemy/engine/default.py", line 922, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (sqlite3.ProgrammingError) Error binding parameter 1: type 'list' is not supported
[SQL: SELECT hosts.id, hosts.ip, hosts.hostname, hosts.domain, hosts.os, hosts.dc, hosts.smbv1, hosts.signing, hosts.spooler, hosts.zerologon, hosts.petitpotam 
FROM hosts 
WHERE lower(hosts.id) = lower(?)]
[parameters: ([('sevenkingdoms.local',)],)]
(Background on this error at: https://sqlalche.me/e/20/f405)

Expected behavior
No stacktrace, just an error because the target does not resolve:

❯ poetry run nxc smb showbug --verbose
[17:17:58] INFO     Error resolving hostname showbug: [Errno -3] Temporary failure in name resolution 

NetExec info

• OS: Kali
• Version of nxc: pulled from github, branch main, commit id 669a55f
• Installed from: git
• Running with poetry run nxc from git folder

Additional context
I think i have a solution for this bug:

git diff nxc/protocols/smb/database.py
diff --git a/nxc/protocols/smb/database.py b/nxc/protocols/smb/database.py
index f32d7bee..81479322 100755
--- a/nxc/protocols/smb/database.py
+++ b/nxc/protocols/smb/database.py
@@ -446,7 +446,7 @@ class database:
 
     def is_credential_local(self, credential_id):
         q = select(self.UsersTable.c.domain).filter(self.UsersTable.c.id == credential_id)
-        user_domain = self.conn.execute(q).all()
+        user_domain = self.conn.execute(q).first()[0]
 
         if user_domain:
             q = select(self.HostsTable).filter(func.lower(self.HostsTable.c.id) == func.lower(user_domain))

I'll do a PR later.
I'm guessing this feature is very seldom used, explaining why no one came across this bug, so there is no rush.
I don't think it is a user-facing bug anyway.

[Marshall-Hallenbeck]

@ajanvrin I'm very confused. Why would you add that line in start_run? The stacktrace also tells you what the problem is, a list is being passed in, which isn't expected. This isn't a bug, you're just not using the function correctly.

[ajanvrin]

Hello @Marshall-Hallenbeck,

To clarify, the invalid value "[('sevenkingdoms.local',)]" that is shown in the stacktrace is not the argument that was passed as argument to db.is_credential_local
The argument that was passed in this case is just the id of the first credential in the creds db.

There is no point in adding that in "start_run" other than allowing you to reproduce the bug.
It's a convenient line for illustration purposes, as the db has already been instantiated, and we can call the function with known-good input.

The line "db.is_credential_local(db.get_credentials()[0][0])" is just a contrived example of calling "db.is_credential_local" with known valid arguments (here i'm passing to it the id of the first credential returned by db.get_credentials, so it is certain that it is a valid input, as it's what the function expects: a credential id).

The bug is not caused by the input to db.is_credential_local, here is where the bug lies, with comments:

# nxc/protocols/smb/database.py line 411
    def is_credential_local(self, credential_id):
        q = select(self.UsersTable.c.domain).filter(self.UsersTable.c.id == credential_id)
        user_domain = self.conn.execute(q).all()
        # The previous line executes without error, as the query is correct.
        # However, conn.execute(q).all returns a list containing one tuple

        # At this point user_domain is a non-empty list of one tuple, which is evaluated as True
        if user_domain:
            # However the following query does not expect a list,
            # it expects a string, this is the source of the bug and the stacktrace
            q = select(self.HostsTable).filter(func.lower(self.HostsTable.c.id) == func.lower(user_domain))
            results = self.conn.execute(q).all()
            return len(results) > 0

Therefore a simple fix consists of changing the following line:

        user_domain = self.conn.execute(q).all()

into

        user_domain = self.conn.execute(q).first()[0]

first does not return a list of lines but a single table line (a tuple, or rather, in this case a one-uple), and [0] unwraps the one-uple to get the user_domain as a string, which is what I believe was intended.

Again, the reason this bug went unnoticed for so long is that db.is_credential_local is called absolutely nowhere in the entire codebase (/bin/grep -rni is_credential_local yields a single result: where the function is defined)