Open ajanvrin opened 4 months ago
Marshall-Hallenbeck
commented
4 months ago @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
commented
4 months ago 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) > 0Therefore 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)
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):
then run anything involving netexec and smb:
Resulted in:
Expected behavior No stacktrace, just an error because the target does not resolve:
NetExec info
Additional context I think i have a solution for this bug:
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.