Skip to content

Fix for RDP asyncio timeout#1211

Merged
Marshall-Hallenbeck merged 1 commit into
mainfrom
fix/rdp-asyncio-deadlock
Apr 21, 2026
Merged

Fix for RDP asyncio timeout#1211
Marshall-Hallenbeck merged 1 commit into
mainfrom
fix/rdp-asyncio-deadlock

Conversation

@Marshall-Hallenbeck
Copy link
Copy Markdown
Collaborator

@Marshall-Hallenbeck Marshall-Hallenbeck commented Apr 20, 2026

Description

Fixes an asyncio deadlock in the RDP protocol that causes nxc rdp to hang indefinitely on hosts with misconfigured RDS Session Host deployments (expired licensing grace period, Connection Broker rejection). When the server sends an unexpected PDU instead of DEMANDACTIVEPDU, aardwolf's __handle_mandatory_capability_exchange blocks forever on MCS.out_queue.get(). NXC's outer asyncio.wait_for eventually fires, but cancelling the connect() coroutine triggers an aardwolf self-deadlock in the __x224_reader_task cleanup path, preventing the process from exiting. In a multi-host scan, a single bad host blocks every subsequent host on the same thread.

This PR fixes the NXC-side cleanup so failed/cancelled connections can always terminate cleanly, regardless of what aardwolf is doing internally:

  • New terminate_conn() helper uses asyncio.wait_for(..., timeout=self.args.rdp_timeout) so cleanup itself can't hang. Calling terminate() explicitly sets aardwolf's __terminate_called flag before asyncio.run() cancels the reader task, breaking the cleanup deadlock chain (reference: aardwolf connection.py:184-185).
  • New connect_rdp_with_cleanup() wraps connect_rdp() with a finally block that always calls terminate_conn(). Sync login paths (create_conn_obj, plaintext_login, hash_login, kerberos_login) use this so the event loop can cleanly tear down before asyncio.run() exits.
  • Async callers that need the connection alive after connecting (screen, nla_screen, execute_shell) continue to use connect_rdp() and handle cleanup in their own finally blocks via terminate_conn().
  • Fixes screen() to always clean up — previously it had no finally block, only returning early on exception.
  • Fixes nla_screen() protocol fallback: the original code returned after the first protocol failure; now it continues to try the next protocol, matching the clear intent of the for proto in self.protoflags_nla loop.
  • Fixes execute_shell() cleanup: the original await self.conn.terminate() had no timeout and could hang on the same aardwolf bug.

Fixes #1169. A complementary upstream aardwolf PR (skelsec/aardwolf#43) addresses the root cause at the library level (__handle_mandatory_capability_exchange timeout + SET_ERROR_INFO_PDU handling), but this NXC-side change provides defense-in-depth and mitigates the hang even against stock aardwolf.

AI assistance disclosure: this PR was produced with Claude Code (Anthropic Claude Opus 4.7) in the following ways: root cause investigation, debugging with network capture and protocol analysis, code authoring, and end-to-end test validation against live lab hosts (Server 2016 / Server 2019 / Server 2022). All changes were manually reviewed by me and verified end-to-end with manual NXC runs before submission.

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

Client: Python 3.13, Kali Linux, aardwolf 0.2.13 (stock upstream).

Reproducing the bug on main:

  1. Stand up a Windows Server 2019 (Build 17763) with the RDS-RD-Server (Session Host) role installed via Server Manager's RDS Deployment Wizard, OR install the role alone and let the 120-day licensing grace period expire without configuring a license server. Either setup produces a server that accepts TCP/CredSSP but rejects the subsequent session via Connection Broker (ERRINFO_CB_CONNECTION_CANCELLED = 0x1192).
  2. Run nxc rdp <ip> -u <valid_user> -p <valid_pass> — the process hangs indefinitely. Multi-host scans with this host in the target list will stall at this target.

Verifying the fix:

  • Single target: nxc rdp <broken_host> -u <user> -p <pass> — should fail within --rdp-timeout seconds (5 by default) and exit cleanly.
  • Multi-host: nxc rdp <broken_host> <good_host1> <good_host2> -u <user> -p <pass> — the broken host fails fast and the good hosts still succeed with Pwn3d!.
  • Authenticated screenshot: nxc rdp <good_host> -u <user> -p <pass> --screenshot — still works; PNG saved to ~/.nxc/screenshots/.
  • Exec: nxc rdp <good_host> -u <user> -p <pass> -x whoami — still works; returns output via clipboard channel.

Tested against:

  • Windows Server 2016 (Build 14393) — domain controller, FilterAdministratorToken=1
  • Windows Server 2019 (Build 17763) — member server, misconfigured RDS (reproduction target)
  • Windows Server 2022 (Build 20348) — domain controller

Screenshots (if appropriate):

Before:
nxc-asyncio-timeout

After:
nxc-rdp-timeout-fixed

Checklist:

  • I have ran Ruff against my changes
  • 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 self-assigned this Apr 20, 2026
@Marshall-Hallenbeck Marshall-Hallenbeck added the bug-fix This Pull Request fixes a bug label Apr 20, 2026
@NeffIsBack
Copy link
Copy Markdown
Member

Sounds good, but isn't that something we should fix in aardwolf? Also, why do we always establish and tear down a connection in each call anyway? Can't we just establish the conn once and terminate it once the protocol finishes?

@Marshall-Hallenbeck
Copy link
Copy Markdown
Collaborator Author

Marshall-Hallenbeck commented Apr 21, 2026

Sounds good, but isn't that something we should fix in aardwolf? Also, why do we always establish and tear down a connection in each call anyway? Can't we just establish the conn once and terminate it once the protocol finishes?

I also have a PR for Aardwolf, but we should have this anyway - we should be terminating connections after we are done with them.

I have a second PR for exactly what you are suggesting as well, but that requires more work, and I know you prefer smaller PRs that do specific things (I do as well, but I get carried away sometimes lol).

This PR is to fix a massive issue that people run into all the time, and then I'll have another PR after this is approved to refactor the RDP connection lifecycle to properly terminate connections via the connection object itself, since the RDP protocol does it a bit janky right now.

@NeffIsBack
Copy link
Copy Markdown
Member

Sounds good, but isn't that something we should fix in aardwolf? Also, why do we always establish and tear down a connection in each call anyway? Can't we just establish the conn once and terminate it once the protocol finishes?

I also have a PR for Aardwolf, but we should have this anyway - we should be terminating connections after we are done with them.

Nice 👍 can you cross ref it?
Yeah definitely, but why don't we just establish the connection object when logging in, use it in all the functions (screenshot, command execution etc.) and then tear it down at the end?

I have a second PR for exactly what you are suggesting as well, but that requires more work, and I know you prefer smaller PRs that do specific things (I do as well, but I get carried away sometimes lol).

This PR is to fix a massive issue that people run into all the time, and then I'll have another PR after this is approved to refactor the RDP connection lifecycle to properly terminate connections via the connection object itself, since the RDP protocol does it a bit janky right now.

Okok, appreciate that 😄 fyi, a while ago I added self.disconnect() to the connection object that is called at the end of a run, so we can properly shut down a connection if needed.

Copy link
Copy Markdown
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.

Looking good so far, let's go with that for now:
Image

@mpgn
Copy link
Copy Markdown
Collaborator

mpgn commented Apr 21, 2026

Nice well done 🫶

@Marshall-Hallenbeck Marshall-Hallenbeck merged commit 43094f1 into main Apr 21, 2026
12 checks passed
@Marshall-Hallenbeck Marshall-Hallenbeck deleted the fix/rdp-asyncio-deadlock branch April 21, 2026 20:51
@NeffIsBack
Copy link
Copy Markdown
Member

The corresponding aardwolf PR: skelsec/aardwolf#44

@Marshall-Hallenbeck Marshall-Hallenbeck restored the fix/rdp-asyncio-deadlock branch April 28, 2026 22:43
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.

RDP Login checks are not reliable and connections are hanging

3 participants