Skip to content

fix: add thread-safety to failed login counters#1172

Merged
NeffIsBack merged 2 commits intoPennyw0rth:mainfrom
A3-N:fix/connection-thread-safety
Mar 30, 2026
Merged

fix: add thread-safety to failed login counters#1172
NeffIsBack merged 2 commits intoPennyw0rth:mainfrom
A3-N:fix/connection-thread-safety

Conversation

@A3-N
Copy link
Copy Markdown
Contributor

@A3-N A3-N commented Mar 24, 2026

Description

There is a race condition on --gfail-limit and --ufail-limit flags. The global_failed_logins and user_failed_logins counters were shared across threads but read and written without sync.

This fix is adding threading.Lock access to the shared counters and re-checks over_fail_limit() inside the existing semaphore block to close the TOCTOU gap.

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

I used a local Samba server instead of prod, but essentially, you should be able to recreate with the --ufail-limit or --gfail-limit limit defined in a simple command like:

nxc smb targets.txt -u admin -p wrongpass --gfail-limit 5

Below screenshot, limit 5, but count is 9.

Screenshot 2026-03-24 105939

Below screenshot of fix, limit 5, count is 5.

Screenshot 2026-03-24 110432

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)

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.

Thanks for the PR!

I think initially the BoundedSemaphore were supposed to solve this problem, but got misplaced after several refactors. Also, a proper Lock() is probably better suited anyway. Could you remove that logic and retry the race conditions?

Comment thread nxc/connection.py
@NeffIsBack NeffIsBack added the bug-fix This Pull Request fixes a bug label Mar 24, 2026
@A3-N A3-N requested a review from NeffIsBack March 24, 2026 18:41
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.

LGTM:
Image

@NeffIsBack NeffIsBack merged commit 4a5cbf6 into Pennyw0rth:main Mar 30, 2026
14 checks passed
@A3-N A3-N deleted the fix/connection-thread-safety branch March 30, 2026 09:54
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.

2 participants