Fix login function with --continue-on-success

[trietend]

The login function does not return True or False in any case.

This leads to a problem if the flag --continue-on-success is used in combination with modules. The login function does not return anything explicit, but it is has to return true in order to launch a module on a successful login.

[Marshall-Hallenbeck]

Nice catch, and great fix, very simple. Thanks for the PR!

[NeffIsBack]

This is indeed intended behaviour!

@Marshall-Hallenbeck not sure if we want to change that. That could lead to unexpected side effects. If we want to we should test that thoroughly

[Marshall-Hallenbeck]

It's intended that we don't run modules after a successful authentication? If the user specifies it, we should definitely run them, I don't understand that.

[NeffIsBack]

Yes and I think there was a reason for it, give me a bit time to think it through.

[Marshall-Hallenbeck]

Roger. We can definitely hold off on merging. I'll run the full test suite as well - I just ran a couple tests, but you're right that we should do more testing, so I'm going to remove that tag for now.

[NeffIsBack]

The problem is it is undefined which user we would use.
Use the first one? Than you could just not use --continue-on-success
Use the one with Pwned? That could potentially never happen
Use the last one? That's some weird shit.

It just doesn't make sense. Use the option to get all users you want and than specify him explicitly.

Therefore this is why it explicitly never return True if we use --continue-on-success and imo it should stay that way. We could disallow using modules though, so it doesn't get confusing.

[NeffIsBack]

Although as i am looking at the code the second if not self.args.continue_on_success: is duplicate and could be removed.

[trietend]

Wouldn't it be the best to run it for all valid users?

[NeffIsBack]

In which scenario would that be useful?
Despite that i think it isn't possible at the moment anyway, because nxc is not designed to have multiple connections open to a single host at a time.

[trietend]

I think you only use "--continue-on-success" if the user context matters and therefore you also want to run it for all users.
Some plugins would be e.g.

• whoami
• spider_plus.py
• keepass_discover.py
• empire_exec.py
  Or also all modules that require admin permissions.

I also think it is not possible at the moment

[NeffIsBack]

All functionality in netexec and therefore all the modules should not be coded to vary with different user contexts. I can't tell you exactly how KeePass works without looking at the code but for most stuff it should work the same for any admin on the pc.
For example Winscp&Putty will simply load all user objects into the registry to be able to query for all users.

Ideally you should not need any specific user context to extract stuff. Tell me if you encounter otherwise and we should fix that

[NeffIsBack]

Obviously "whoami" is an exception to that rule.
Though you shouldn't need to login as different users into a specific target in the first place.

[Marshall-Hallenbeck]

What's the downside of running the modules for each valid user? If someone specifies that, I think we should do it. If they didn't want it to run, they just simply don't have to specify modules to run. We could output a disclaimer if there are multiple users passed in with --continue-on-success enabled, but I think limiting the capabilities doesn't make sense.

Maybe we want to try to get a shell on each user via met_inject, or maybe there's some permissions for some users that would restrict the results of some modules. Of course a lot of them might have the same output, but that's on the user to get spammed.

[NeffIsBack]

NetExec is just not designed to fulfill that. We have only one protocol object and one module object per connection. If we would allow to login with multiple users in one connection that protocol object would have to handle different states of each object it holds (smb connection, modules, variables etc.). Atleast without some serious work or completely redesigning how nxc works under the hood.

Nxc would just go

[NeffIsBack]

Nxc is designed to differentiate between low priv and high priv (admin). There should be no difference for almost all tools/modules, atleast that's how it is designed to function. If there are modules that would behave differently depending on the user context we should change that. From the development perspektive there is only admin or no admin.

[trietend]

All functionality in netexec and therefore all the modules should not be coded to vary with different user contexts. I can't tell you exactly how KeePass works without looking at the code but for most stuff it should work the same for any admin on the pc. For example Winscp&Putty will simply load all user objects into the registry to be able to query for all users.

The behavior is the same, but the outcome may be another because of e.g access to different files.

Yes, at the moment protocol and connection is the same. I thought a bit about it and i think it wouldn't be that much work.

Currently the whole --continue-on-success logic is in login of connection.py. If we move this to main before asyncio.run and launch as currently implemented one connection per host if --continue-on-success is not set, but one connection per host and user pair if it is set.

[NeffIsBack]

Which functionality/modules would depend on the users context? So far only spider_plus would come to my mind.

Moving the user/password/hash processing out of the protocol object into the main function would probably be a cleaner solution regardless of having multiple users per host. Though this would be a pretty significant change which would require probably a lot of work.

[trietend]

Which functionality/modules would depend on the users context? So far only spider_plus would come to my mind.

At least the ones i mentioned above and as @Marshall-Hallenbeck mentioned met_inject.py. Furthermore all plugins where you specify an administrative and a non administrative user.
And not only modules are affected, command execution with '-x' is also affected.

Moving the user/password/hash processing out of the protocol object into the main function would probably be a cleaner solution regardless of having multiple users per host. Though this would be a pretty significant change which would require probably a lot of work.

I think we should move this stuff to a credentials class.
If we do all the parsing in main and launch a asyncio task for every user and host combination it will be hard to stop on success i guess.

[trietend]

I think we can it this way:

Case 1 - without --continue-on-success

One Task per host with
users = ["admin", "user"]
passwords = ["admin", "password"]

This will stop on the first success.

Case 2 - with --continue-on-success

One Task per host and user
Task1
users = ["admin"]
passwords = ["admin", "password"]

Task2
users = [ "user"]
passwords = ["admin", "password"]

This will run everything for both users.

[NeffIsBack]

At least the ones i mentioned above and as @Marshall-Hallenbeck mentioned met_inject.py.

Honestly, yes you could open a thousand of meterpreter shells, but I don't reeealy see the value in it. @Marshall-Hallenbeck as it was your suggestion, what would be the point of having multiple admin connections to a host?

And that's the actually the point. There is no user dependent context if you are local admin. You have full control over the target and therefor can do anything on the system, including impersonating other users if you would like to.

Furthermore all plugins where you specify an administrative and a non administrative user.

The same is for all admin privs modules. It does not depend on the user context and therefore there is no need of executing the module a thousand times (also applies to -x). The same applies to most low privilege modules. Most of them do enumeration with low privileges, which just require a user to authenticate with, but nothing that would change depending on the user context.

The only exceptions to this are spider_plus as it enumerates files which explicitly the user has access to and what was mentioned in #275. Being able to execute --shares with one command and different users might be a good addition.

Moving the user/password/hash processing out of the protocol object into the main function would probably be a cleaner solution regardless of having multiple users per host. Though this would be a pretty significant change which would require probably a lot of work.

I think we should move this stuff to a credentials class.

Agreed on this. It would be a cleaner solution and maybe save a bit of RAM&computation when you got a huge list of users&password (though, the scenario is quite rare imo).

[Marshall-Hallenbeck]

I'm confused on what the problem is to allow this in the first place? It fixes an annoying issue: not running modules when valid credentials are found. What's the downside? If they pop multiple users it runs multiple times? I think that's a fair trade off.

[trietend]

This PR would run the module, but with the last username/secret combination (valid or not). It would be necessary to reestablish the connection for valid creds, to have a valid session and run further stuf. That's why the question came up, which one to use.

In my opinion we should run for all users which would need a bit more effort as it would be necessary to launch multiple connection objects per host.

[NeffIsBack]

To have it also in here, @Marshall-Hallenbeck this implementation will crash. If we want to realise this idea this needs to be done at the main netexec.py level

Dismissing review