Skip to content

fix(coerce_plus): move get_dynamic_endpoint back to module-level function#1131

Open
Marshall-Hallenbeck wants to merge 1 commit intomainfrom
fix/moduleloader-namespace-collision
Open

fix(coerce_plus): move get_dynamic_endpoint back to module-level function#1131
Marshall-Hallenbeck wants to merge 1 commit intomainfrom
fix/moduleloader-namespace-collision

Conversation

@Marshall-Hallenbeck
Copy link
Collaborator

@Marshall-Hallenbeck Marshall-Hallenbeck commented Mar 3, 2026

Description

Commit 81c6d9f (PR #866) moved get_dynamic_endpoint from a module-level function to a @staticmethod on NXCModule. This broke coerce_plus when used with any other -M module, because the module loader uses the same sys.modules key ("NXCModule") for all module files. Whichever module loads last overwrites the NXCModule class in the shared namespace, so coerce_plus's helper classes (PrinterBugTrigger, PetitPotamtTrigger) resolve a different module's NXCModule class — one that doesn't have get_dynamic_endpoint.

The error is order-dependent:

  • -M coerce_plus -M ms17-010broken (ms17-010 loads last, overwrites NXCModule)
  • -M ms17-010 -M coerce_plus → works (coerce_plus loads last, its own class persists)
  • -M coerce_plus alone → works (nothing to overwrite it)

This is why the bug hasn't been reported — it only appears when combining coerce_plus with other modules AND coerce_plus isn't last in the list.

Fix: Move get_dynamic_endpoint back to a module-level function (reverting the relocation from 81c6d9f). As a module-level name, it isn't resolved through the NXCModule class and is unaffected by the namespace collision. Also makes the [dcerpc] endpoint resolution lazy so it only runs when that pipe is selected, avoiding an unnecessary network call to port 135 on every target.

The underlying module loader collision is addressed separately in PR #1132.

AI disclosure: Claude Code (Claude Opus 4.6) was used to assist with root cause analysis and drafting the fix. The bug was discovered during a real pentest scan, root cause was traced and verified by human and AI together, and the fix was human-reviewed and tested on live targets.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Deprecation of feature or functionality
  • This change requires a documentation update
  • This requires a third party update (such as Impacket, Dploot, lsassy, etc)
  • This PR was created with the assistance of AI (list what type of assistance, tool(s)/model(s) in the description)

Setup guide for the review

How to trigger the bug:

Run coerce_plus with any other module where coerce_plus is NOT last:

nxc smb <target> -u <user> -p <pass> -M coerce_plus -M ms17-010

You will see Error in PrinterBug module: type object 'NXCModule' has no attribute 'get_dynamic_endpoint' for every target.

Swap the order so coerce_plus is last and it works:

nxc smb <target> -u <user> -p <pass> -M ms17-010 -M coerce_plus

Tested on:

  • Kali Linux 6.17.10+kali-amd64, Python 3.13.9
  • Targets: Windows Server 2016/2019, Windows 10 Build 17763/19041

Screenshots (if appropriate):

Screenshots to be attached.

Checklist:

  • I have ran Ruff against my changes (poetry: poetry run ruff check ., use --fix to automatically fix what it can)
  • I have added or updated the tests/e2e_commands.txt file if necessary (new modules or features are required to be added to the e2e tests)
  • If reliant on changes of third party dependencies, such as Impacket, dploot, lsassy, etc, I have linked the relevant PRs in those projects
  • I have linked relevant sources that describes the added technique (blog posts, documentation, etc)
  • I have performed a self-review of my own code (not an AI review)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (PR here: https://github.com/Pennyw0rth/NetExec-Wiki)

@Marshall-Hallenbeck Marshall-Hallenbeck added the bug-fix This Pull Request fixes a bug label Mar 3, 2026
@Marshall-Hallenbeck Marshall-Hallenbeck self-assigned this Mar 3, 2026
@Marshall-Hallenbeck
Copy link
Collaborator Author

Marshall-Hallenbeck commented Mar 3, 2026

Claude did a pretty good job here, but checked "I have linked relevant sources that describes the added technique (blog posts, documentation, etc)" for some reason. Overall not bad because I put in minimal feedback!

@Marshall-Hallenbeck Marshall-Hallenbeck force-pushed the fix/moduleloader-namespace-collision branch from 6ae0df5 to 194cb3b Compare March 3, 2026 15:24
…tion

Commit 81c6d9f (PR #866) moved get_dynamic_endpoint from a module-level
function to a @staticmethod on NXCModule. This broke coerce_plus when used
with any other -M module that loads after it, because the module loader
uses the same sys.modules key for all modules, causing the NXCModule class
reference to be overwritten by whichever module loads last.

As a module-level function, get_dynamic_endpoint is not resolved through
the NXCModule class name and is unaffected by the collision.

Also makes the [dcerpc] endpoint resolution lazy so it only runs when
that pipe is actually selected, avoiding an unnecessary network call
to port 135 on every target.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Marshall-Hallenbeck Marshall-Hallenbeck force-pushed the fix/moduleloader-namespace-collision branch from 194cb3b to 41b3fc2 Compare March 3, 2026 17:04
@Marshall-Hallenbeck Marshall-Hallenbeck changed the title fix(moduleloader): isolate module namespaces to prevent NXCModule class collision fix(coerce_plus): move get_dynamic_endpoint back to module-level function Mar 3, 2026
@Marshall-Hallenbeck Marshall-Hallenbeck marked this pull request as ready for review March 3, 2026 17:36
Copilot AI review requested due to automatic review settings March 3, 2026 17:36
@Marshall-Hallenbeck
Copy link
Collaborator Author

@NeffIsBack this is now just the staticmethod fix

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an order-dependent failure in coerce_plus when multiple -M modules are loaded by moving get_dynamic_endpoint back to a module-level helper, avoiding reliance on NXCModule class attributes under the current module loader behavior.

Changes:

  • Move get_dynamic_endpoint from NXCModule staticmethod back to a module-level function.
  • Update callers to use get_dynamic_endpoint(...) directly.
  • Make [dcerpc] endpoint resolution lazy (only resolve when that pipe is selected).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
}
if pipe == "[dcerpc]":
binding_params["[dcerpc]"]["stringBinding"] = get_dynamic_endpoint(uuidtup_to_bin(("12345678-1234-abcd-ef00-0123456789ab", "1.0")), target)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the [dcerpc] path, get_dynamic_endpoint(...) is called with the default timeout=5, which means a blocked/filtered TCP/135 can stall this module for up to 5 seconds per target when attempting the [dcerpc] pipe. Since this is a best-effort fallback after spoolss, consider passing a shorter timeout here (similar to the timeout=1 used for the EFS activation call) to keep scans responsive.

Suggested change
binding_params["[dcerpc]"]["stringBinding"] = get_dynamic_endpoint(uuidtup_to_bin(("12345678-1234-abcd-ef00-0123456789ab", "1.0")), target)
binding_params["[dcerpc]"]["stringBinding"] = get_dynamic_endpoint(
uuidtup_to_bin(("12345678-1234-abcd-ef00-0123456789ab", "1.0")),
target,
timeout=1,
)

Copilot uses AI. Check for mistakes.
Comment on lines +221 to +222
dce.connect()
return epm.hept_map(target, interface, protocol="ncacn_ip_tcp", dce=dce)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes an order-dependent multi-module load issue. There is E2E coverage for coerce_plus already, but it currently only runs coerce_plus by itself or last in a multi-module list. Consider adding an E2E command that loads coerce_plus before another module (e.g., -M coerce_plus -M ms17-010) to prevent regressions of the original ordering bug.

Suggested change
dce.connect()
return epm.hept_map(target, interface, protocol="ncacn_ip_tcp", dce=dce)
try:
dce.connect()
return epm.hept_map(target, interface, protocol="ncacn_ip_tcp", dce=dce)
finally:
with contextlib.suppress(Exception):
dce.disconnect()

Copilot uses AI. Check for mistakes.
rpctransport.set_connect_timeout(timeout)
dce = rpctransport.get_dce_rpc()
dce.connect()
return epm.hept_map(target, interface, protocol="ncacn_ip_tcp", dce=dce)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_dynamic_endpoint() creates and connects a DCE/RPC client (dce.connect()) but never disconnects it. Because this helper can be called repeatedly during scans, it can leave open TCP/135 connections longer than needed and may consume file descriptors over time. Consider wrapping the connect/map in a try/finally and calling dce.disconnect() (or using a context manager if available) after hept_map completes or fails.

Suggested change
return epm.hept_map(target, interface, protocol="ncacn_ip_tcp", dce=dce)
try:
return epm.hept_map(target, interface, protocol="ncacn_ip_tcp", dce=dce)
finally:
with contextlib.suppress(Exception):
dce.disconnect()

Copilot uses AI. Check for mistakes.
Copy link
Member

@NeffIsBack NeffIsBack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ready? There are a lot of unresolved comments by the AI. Otherwise, LGTM:

Image

"port": None
}
}
if pipe == "[dcerpc]":
Copy link
Member

@NeffIsBack NeffIsBack Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait, why should we change this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix This Pull Request fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants