Fix all kinds of security issues - #1444
NeffIsBack wants to merge 23 commits into
Conversation
Strip non-alphanumeric characters (except hyphens and dots) from
server-provided NTLM hostname before use in file paths or content.
Prevents:
- Path traversal via ../ in hostname (file creation outside ~/.nxc/logs/)
- DoS via null byte (ValueError crash in open())
- DoS via { characters (KeyError crash in str.format())
- Newline injection in --generate-hosts-file output
- Affects: SMB, RDP, VNC, WinRM, MSSQL credential dump and screenshot paths
Log a warning showing the original and sanitized hostname so users are alerted to potential non-compliant implementations or rogue servers.
Move the hostname sanitization regex from an inline check in connection.py into a reusable sanitize_hostname() function in nxc/helpers/misc.py, and apply it at the source in each protocol's enum_host_info where hostnames are received from NTLM/server data. Protocols covered: SMB, WinRM, WMI, MSSQL, RDP. Addresses review feedback on PR #1243.
Marshall-Hallenbeck
left a comment
There was a problem hiding this comment.
I didn't test it but the code looks good.
|
Ironically the LLM didn't find and patch the security issue in spider_plus yet. Gonna fix that tomorrow and double check that everything we know of has been caught. |
|
are we all sure about all |
It's probably unnecessary since we are always taking the first element anyway (and I can revert it if you want), but I think it is supposed to prevent breaking the domain apart where it should remain one blob. |
|
One of the new tests in this PR currently fails: The In the real flow the source is sanitized in The other 73 tests in (review by opus 4.8 ^^) |
|
Yeah sol also really really wanted me to sanitize this part, but I refused so far because this is kind of the entire point of sanitizing the sources. The krb/host file sanitization added a LOT of really uggly code. I will recheck |
|
in this case this is just the test that fail, maybe remove it |
Description
Based on #1243 but got too much so here is a proper description of what happened.
This handles all kinds of security issues:
enum_host_info()as first line of defense against poisoned dataCodex with gpt 5.6-sol on ultra was used to fix the issues. Have manually reviewed (and fixed where AI was stupid) the code.
Type of change
Insert an "x" inside the brackets for relevant items (do not delete options)
Setup guide for the review
TEST EVERYTHING
Screenshots (if appropriate):
Example of basic path operations still working:


Checklist:
Insert an "x" inside the brackets for completed and relevant items (do not delete options)
poetry run ruff check ., use--fixto automatically fix what it can)tests/e2e_commands.txtfile if necessary (new modules or features are required to be added to the e2e tests)