Modules args parsing rework

[Dfte]

This PR aims at reworking the entire module argparsing to make it easier to use. Currently the shadowrdp module works using the following command:

nxc smb ip -u -p -M shadowrdp --enable

You can also list protocol options (which will list available modules

nxc smb ip -u -p -h

And list module options:

nxc smb ip -u -p -M shadowrdp -h

HUUUUUUGE WORK IN PROGRESS

[Dfte]

With the latests commits, -h will print protocole available modules:

While -M module -h will print available options:

[Dfte]

These commits allow running 3 modules entirely (adcs, schtask_as, shadowrdp):

[Dfte]

These commits refactor cli.py and loadermodule so that cli.py retrievse the list of modules from loadermodule.py. It also makes sure that loaded modules do have specific attributes.

[NeffIsBack]

As always, thanks for your work!

My thoughts on the changes: First of all, having separate argparsers for modules is imo a huge win, because then we can rely on argparse to do its job which is much more reliable and easier to use than our manual VARIABLE=VALUE method.

However, there are some problems with the new way of loading modules. The new implementations will always load all modules. This has the major downside that on every execution all module files and with that all imports are loaded. This takes a HUGE amount of time. This is also the reason why currently list_modules() only looks at the file names, the module loader is only loaded when either -L or -M is called (netexec.py LL163-165) and also modules are only loaded into the connection object when the arg is actually set (connection.py LL248-251). Simple showcase is loading nxc itself which is noticeably slower (3x-4x times) in this PR:

Imo we should also keep the -L screen as this is a much prettier and much clearer in terms of which modules need admin privs/categories etc. So my proposal would be to keep as much as possible of the old loading infrastructure and only attach the parent parsers to the module parsers, if requested by -L/-M.

Some minor details that are probably easily fixable:

• The screen for the nxc command is currently broken
• We should not do the manual parsing for -M, but just use parse_known_args()

[Dfte]

Okay! I'll rollback things you mentioned so that it matches current state :)!

[Dfte]

Alright new stuff incoming. Here is the detail. As requested the modules are not all loaded, instead moduleloader takes an arguments that is parse_module_attributes:

• If True, parse all the class
• If False; only return the list of modules:

def list_modules(self, parse_modules_attributes: bool = False):
        """
        Liste les modules dans nxc/modules.

        Args:
            parse_modules_attributes (bool):
                - False → retourne juste les noms de fichiers .py valides
                - True  → retourne un mapping modules_map et per_proto_modules

        Returns:
            - Si parse_modules_attributes=False → list[str]
            - Si parse_modules_attributes=True  → (dict[str, class], dict[str, list[tuple[str, str]]])
        """
        for module_file in listdir(self.modules_dir):
            if not module_file.endswith(".py") or module_file == "example_module.py":
                continue

            # If parsing modules is not requested, only retrieves the module's name
            mod_name = module_file[:-3]
            if not parse_modules_attributes:
                self.module_names.append(mod_name)
                continue

            # Else, instance the module and retrieve necessary attributes
            try:
                module_pkg = importlib.import_module(f"{self.import_base}.{mod_name}")
                module_class = getattr(module_pkg, "NXCModule", None)
            except Exception:
                continue

            if not module_class:
                continue

            required_attrs = {"name", "description", "supported_protocols", "__init__", "register_module_options", "category", }
            if not all(hasattr(module_class, attr) for attr in required_attrs):
                continue

            self.modules_map[module_class.name] = module_class
            for proto in module_class.supported_protocols:
                self.per_proto_modules.setdefault(proto, []).append((module_class.name, module_class.description))

        return (self.modules_map, self.per_proto_modules) if parse_modules_attributes else sorted(self.module_names, key=str.casefold)

Otherwise:

• nxc:

• nxc smb -L:

I will reimplement the entire category/priv thing no worry :)

• nxc smb -M shadowrdp --options

• nxc smb -M shadowrdp --enable:

I'll remove the HELLO THERE also (forgot that). As of now, we can only use one module at the time but it's just a for loop.

Let me know if you spot other things !

[Dfte]

Now it's possible to use multiple module in a single command:

nxc smb 192.168.56.10 -u administrateur -p Defte@WF -M wdigest --check -M ntlmv1

But it's not possible to run -M mod1 -M mod2 --options:

[Dfte]

So far I have added 10 modules to test the PR which can be chained with a -M :

nxc smb 192.168.56.0/24 -u administrateur -p Defte@WF -M enum_ca -M webdav -M ntlmv1 -M wdigest --check

[Dfte]

-L option works as expected

Also, simplified the moduleloader so that it only returns either the module names or the fully parsed dict.

[NeffIsBack]

Quick thought of the top of my head: I think it is good to incentivize people to use class variables by using them to init and making them available there. What about the connection variable? Haven't tested that but i think it should work just as well as the others, as this should always be called when the connection object is already instantiated as a protocol.

[Dfte]

Quick thought of the top of my head: I think it is good to incentivize people to use class variables by using them to init and making them available there. What about the connection variable? Haven't tested that but i think it should work just as well as the others, as this should always be called when the connection object is already instantiated as a protocol.

Already done my friend :D

EDIT: just realized I didn't push that change rofl, 2 sec. ALthought keep in mind we'll have to check each and every futur modules because some of them do initiate self.connection, some use connection, some use connection.conn. It's a bit messy but doable ;)

As of now the only 10 modules will work btw.

[Dfte]

Can print multiple modules output:

[Dfte]

Better looking

[NeffIsBack]

Quick thought of the top of my head: I think it is good to incentivize people to use class variables by using them to init and making them available there. What about the connection variable? Haven't tested that but i think it should work just as well as the others, as this should always be called when the connection object is already instantiated as a protocol.

Already done my friend :D

Awesome 😄

EDIT: just realized I didn't push that change rofl, 2 sec. ALthought keep in mind we'll have to check each and every futur modules because some of them do initiate self.connection, some use connection, some use connection.conn. It's a bit messy but doable ;)

As of now the only 10 modules will work btw.

That actually has a use case, because connection is the NetExec SMB protocol, while the connection.conn is the raw impacket one. Sometimes you just need direct access to functions that are implemented in the impacket connection.

[Dfte]

Quick thought of the top of my head: I think it is good to incentivize people to use class variables by using them to init and making them available there. What about the connection variable? Haven't tested that but i think it should work just as well as the others, as this should always be called when the connection object is already instantiated as a protocol.

Already done my friend :D

Awesome 😄

EDIT: just realized I didn't push that change rofl, 2 sec. ALthought keep in mind we'll have to check each and every futur modules because some of them do initiate self.connection, some use connection, some use connection.conn. It's a bit messy but doable ;) As of now the only 10 modules will work btw.

That actually has a use case, because connection is the NetExec SMB protocol, while the connection.conn is the raw impacket one. Sometimes you just need direct access to functions that are implemented in the impacket connection.

Yeah I got that, what I meant is that when adapting modules to the new module core, we'll have to be meticulous and not mix these conn* things together ahah!

[Dfte]

Todo:

• [ ] Translate all modules to the new architecture
• [ ] Make "check" default value for any module that reads registry key values
• [ ] Make a on_login function for all module that reads registry keys enabling remote registry as a simple user