Pennyw0rth
Introducing a Summary of non-failed authentications to results
Description
I have added a summary so when we are password spraying we don't need to be grepping 100s of results to see where the green or purple results are which might indicate something interesting.
I have just implemeted it for the big 3 protocols which are SMB, LDAP and RDP. Some of the code operates slightly different depending on the protocol so the code had to be tweaked at times so is not identical across the 3 at times.
Initially I didn't have the --summary flag required to run it so I had some logical safeguards like only showing a summary if there were more than 10 machines or users. This would be bypassed if a file was being inputted. I also turned it off when modules were being run but maybe I have missed use cases where this might be useful.
I have left these safeguards in for now (which I can remove eventually) with the --summary flag as maybe with the checks in place we could default to using it without the --summary flag if you think it is worth having permanently rather than users requiring more flags to trigger things.
Type of change
Insert an "x" inside the brackets for relevant items (do not delete options)
- [ x] New feature (non-breaking change which adds functionality)
Screenshots (if appropriate):
Very nice idea! That could solve the long requested feature of "how do i mute failed logins so i can only get the successful ones". I think there is still a few things to do because this probably doesn't belong in the netexec.py file, but we can work of of that.
Related or probably even fixes:
- https://github.com/Pennyw0rth/NetExec/issues/85
- https://github.com/Pennyw0rth/NetExec/issues/849
Vouch for this method
I think it is cool as well, but i am unsure yet on the implementation. It's kinda nasty to hook each login method manually because then you would have to implement it for every single protocols manually. One idea that i had is to use the information when the login method returns True or False and then sum it up in the connection.py file, but then we would also loose info about stuff like is_admin etc.
Based on my analysis of the PR and the current codebase, here's my code review of PR #886: Introducing a Summary of non-failed authentications to results:
PR #886 Code Review
Overview
This PR adds a --summary flag to display a summary of successful/non-failed authentication attempts after password spray operations. This addresses a long-requested feature (issues #85 and #849).
Positive Aspects
- Addresses real user need - Grepping through hundreds of log entries during password sprays is painful; a summary at the end is valuable.
- Safeguards are thoughtful - Only enabling when >10 targets or file input prevents unnecessary output for small scans.
- Variable naming fix - The author addressed the snake_case feedback (changing summaryText to summary_text).
Concerns and Suggestions
- Architecture: Global State is Problematic
Using GLOBAL_SUMMARY_RESULTS = [] as a module-level global variable is concerning:
- Thread safety - netexec.py:43-68 shows ThreadPoolExecutor running protocol objects concurrently. Multiple threads appending to a shared list without locks could cause race conditions or data corruption.
- Suggested fix: Use threading.Lock() or pass a thread-safe queue/collection, or accumulate results per-protocol instance and aggregate at the end.
- Inconsistent Protocol Coverage
The PR only covers SMB, LDAP, and RDP. Other protocols with login methods aren't included:
- nxc/protocols/wmi.py - has kerberos_login, plaintext_login, hash_login
- nxc/protocols/mssql.py - has all three login methods
- nxc/protocols/winrm.py - has plaintext_login, hash_login
- nxc/protocols/ssh.py - has plaintext_login
- nxc/protocols/ftp.py - has plaintext_login
Suggestion: Either implement for all protocols or document that this is SMB/LDAP/RDP-only in the help text.
- Code Duplication
The web fetch indicates the same summary-appending logic is repeated across:
- 3 login methods × 3 protocols = 9 places
This violates DRY. Consider:
- A decorator or mixin method in the base connection class
- A helper function that protocols can call
- The Safeguard Logic Location
Per reviewer NeffIsBack's comment, the implementation location may need adjustment. The check for >10 machines/users happens at display time in start_run(), but the data is collected unconditionally. This means:
- Memory is used storing results even when they won't be displayed
- The safeguard could be moved to collection time
- Error Code Handling Differences
From the web fetch:
- LDAP excludes error code "52e"
- RDP/SMB exclude "STATUS_LOGON_FAILURE"
This inconsistency could confuse users. Consider documenting what "non-failed" means for each protocol.
- Module Disabling Logic
The PR disables summary when modules are running. This is reasonable but should be documented in the --summary help text.
- Missing Type Hints
The codebase already uses some type hints (see cli.py). The new GLOBAL_SUMMARY_RESULTS should be typed: GLOBAL_SUMMARY_RESULTS: list[str] = []
Testing Concerns
- How was thread safety tested?
- Were edge cases tested (0 targets, 1 target, exactly 10 targets)?
- Was performance impact measured for very large scans (10000+ targets)?
Summary
The feature is valuable and addresses a real need. However, the global mutable state with concurrent thread access is the primary concern that should be addressed before merging. The inconsistent protocol coverage is secondary but worth discussing.
Recommendation: Request changes to address thread safety, and clarify the scope of protocol support.
@moscowchill ?
yes?
What is this AI review? :D
Fixing these things without AI just needed a summary hehe :D
@moscowchill ?
yes?
What is this AI review? :D
Fixing these things without AI just needed a summary hehe :D
Feel free, but i wouldn't count on the AI producing a useful review 😅 what is this complaint about thread safety for example...