Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in WSUS applicability, WDigest legacy handling, PowerShell detection, and WMI error handling.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Updates the WCC module to recognize modern Windows defaults and avoid false security failures on non-DC hosts.
Changes:
- Adds default-aware SMB, WSUS, LDAP, and PowerShell checks.
- Corrects missing-value handling, PPL evaluation, and operator messages.
- Registers WCC in end-to-end commands.
| File | Summary |
|---|---|
tests/e2e_commands.txt |
Adds the WCC end-to-end command. |
nxc/modules/wcc.py |
Revises Windows security checks; five moderate issues require changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7eb430b to
4fd38a0
Compare
|
Thanks for the PR! I think we should merge #893 before this one |
| ConfigCheck("SMB signing enabled", "Checks if SMB signing is enabled", checkers=[self.check_smb_signing]), | ||
| ConfigCheck("LDAP signing enabled", "Checks if LDAP signing is enabled", checkers=[self.check_ldap_signing]), |
There was a problem hiding this comment.
Why are you removing the Regsitry checks tho and move it to some separate function?
There was a problem hiding this comment.
- SMB signing: value missing (24H2/Server 2025 default) → fall back to the signing negotiated in the SMB session (connection.signing), a non-registry source.
- LDAP signing: NTDS\Parameters exists only on DCs → "not applicable" on non-DCs instead of KO.
- WSUS: UseWUServer must override the WUServer URL check.
- PowerShell v2: version gate (only 1.x/2.x counts) + WMI feature state.
requires more than tenary operation so wanted to seperate it
There was a problem hiding this comment.
Most of this is querying registry values and then compare it to the predefined values. This logic should remain if possible and use the built-in comparators instead of extracting such logic (e.g. WSUS, LDAP signing etc.). From what I can tell most of it can be solved by providing the correct path or setting the correct comparator option
…hosts
The wcc module treats missing registry values as insecure (KOIfMissing
default) and applies override-only or DC-only checks to every host,
producing false KO verdicts on stock modern Windows:
- SMB signing: requiresecuritysignature is only written when explicitly
configured (GPO). Windows 11 24H2 / Server 2025 require SMB signing
by default without materializing the value, so wcc reported KO on
hosts that do require signing, contradicting the signing:True status
negotiated from ServerSecurityMode. New check_smb_signing(): the
registry value stays authoritative when present; when missing, fall
back to the signing requirement negotiated with the server.
- UAC: LocalAccountTokenFilterPolicy absent means remote UAC filtering
is enforced (default 0), not disabled.
- AlwaysInstallElevated: policy key absent means the feature is
disabled (default), which is the secure state.
- WDigest: UseLogonCredential absent means WDigest does not store
cleartext credentials (default since Win8.1/Server2012R2).
- WSUS: no WUServer configured means updates come from Microsoft
Update over HTTPS; only flag non-HTTPS WSUS servers
(new check_wsus_config).
- Lsass PPL: RunAsPPL=2 ("Enabled without UEFI lock") also means PPL
is active, accept >= 1.
- No PowerShell v2: PSCompatibleVersion always lists 2.0 on PowerShell
5.1 (engine compatibility), so the old check could never pass; check
the v1/v2 engine registration and the PowerShell 2.0 optional
feature state instead (new check_powershell_v2).
- LDAP signing: LDAPServerIntegrity/LdapEnforceChannelBinding only
apply to domain controllers; on non-DC hosts report not-applicable
instead of KO, and read LdapEnforceChannelBinding from
NTDS\Parameters (per ADV190023) rather than the NTDS service key
(new check_ldap_signing).
Also fix the negated reason string for custom operators in
check_registry (always printed '== True') and drop the now-unused
startswith/not_ helpers.
Verified read-only against a Windows 11 24H2 (build 26100) host: the
eight checks above flipped KO->OK with correct reasons, all other
checks unchanged. Defender checks deliberately untouched: on the
verification host a third-party AV is active and WinDefend is stopped,
so their KO verdicts are correct and a "missing = default enabled"
fallback would produce false positives.
4fd38a0 to
06338c9
Compare
|
Ima need to test the github connector here. @codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06338c91e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ConfigCheck("Spooler service disabled", "Checks if the spooler service is disabled", checkers=[self.check_spooler_service]), | ||
| ConfigCheck("WDigest authentication disabled", "Checks if WDigest authentication is disabled", checker_args=[[self, ("HKLM\\SYSTEM\\CurrentControlSet\\Control\\SecurityProviders\\WDigest", "UseLogonCredential", 0)]]), | ||
| ConfigCheck("WSUS configuration", "Checks if WSUS configuration uses HTTPS", checkers=[self.check_wsus_running, None], checker_args=[[], [self, ("HKLM\\Software\\Policies\\Microsoft\\Windows\\WindowsUpdate", "WUServer", "https://", startswith), ("HKLM\\Software\\Policies\\Microsoft\\Windows\\WindowsUpdate\\AU", "UseWUServer", 0, operator.eq)]], checker_kwargs=[{}, {"options": {"lastWins": True}}]), | ||
| ConfigCheck("WDigest authentication disabled", "Checks if WDigest authentication is disabled", checker_args=[[self, ("HKLM\\SYSTEM\\CurrentControlSet\\Control\\SecurityProviders\\WDigest", "UseLogonCredential", 0)]], checker_kwargs=[{"options": {"KOIfMissing": False}}]), |
There was a problem hiding this comment.
On Windows 8/Server 2012 and earlier, including an unconfigured Windows 7/Server 2008 R2 host, an absent UseLogonCredential retains the legacy behavior of caching WDigest credentials; the secure absent-value default only begins with Windows 8.1/Server 2012 R2. Applying KOIfMissing=False unconditionally therefore reports these legacy hosts as secure even though plaintext credentials may still be retained, so the missing-value fallback needs to be gated on an OS version with the secure default.
Useful? React with 👍 / 👎.
| state = records[0].get("InstallState", {}).get("value") | ||
| if state == 1: | ||
| return False, ["PowerShell 2.0 engine optional feature enabled"] | ||
| return True, ["PowerShell v2 engine not installed"] |
There was a problem hiding this comment.
When Win32_OptionalFeature returns InstallState=4 (Unknown), or the property is missing or otherwise unexpected, this fallthrough reports that PowerShell v2 is not installed. That turns an indeterminate WMI result into an OK security verdict; only the known disabled/absent states should pass, while unknown values should remain KO like a failed query.
Useful? React with 👍 / 👎.
Description
The wcc module treats missing registry values as insecure (
KOIfMissingdefault) and applies override-only or DC-only checks to every host, producing false KO verdicts on stockmodern Windows:
requiresecuritysignatureis only written when explicitly configured (GPO). Windows 11 24H2 / Server 2025 require SMB signing by default without materializingthe value, so wcc reported KO on hosts that do require signing, contradicting the
signing:Truestatus negotiated fromServerSecurityMode. Newcheck_smb_signing(): theregistry value stays authoritative when present; when missing, fall back to the signing requirement negotiated with the server.
LocalAccountTokenFilterPolicyabsent means remote UAC filtering is enforced (default 0), not disabled.UseLogonCredentialabsent means WDigest does not store cleartext credentials (default since Win 8.1 / Server 2012 R2).WUServerconfigured means updates come from Microsoft Update over HTTPS; only flag non-HTTPS WSUS servers (newcheck_wsus_config).RunAsPPL=2("Enabled without UEFI lock") also means PPL is active, accept>= 1.PSCompatibleVersionalways lists 2.0 on PowerShell 5.1 (engine compatibility), so the old check could never pass; check the v1/v2 engine registrationand the PowerShell 2.0 optional feature state instead (new
check_powershell_v2).LDAPServerIntegrity/LdapEnforceChannelBindingonly apply to domain controllers; on non-DC hosts report not-applicable instead of KO, and readLdapEnforceChannelBindingfromNTDS\Parameters(per ADV190023) rather than the NTDS service key (newcheck_ldap_signing).Also fixes the negated reason string for custom operators in
check_registry(always printed== True) and drops the now-unusedstartswith/not_helpers.Verified read-only against a Windows 11 24H2 (build 26100) host: the eight checks above flipped KO → OK with correct reasons, all other checks unchanged. Defender checks
deliberately untouched: on the verification host a third-party AV is active and WinDefend is stopped, so their KO verdicts are correct and a "missing = default enabled" fallback
would produce false positives.
No third-party dependencies are required for this change.
AI assistance: This PR was prepared with the help of an AI coding assistant (omp agent, model: zai/glm-5.3). Extent of assistance: git branch/commit/push hygiene, applying
the authored patch, fixing two Ruff findings in the patch (raw-string docstring prefix, trailing newlines at EOF), adding the
tests/e2e_commands.txtentry, and stub-basedsmoke-testing of the new checker logic (19/19 scenarios, no live target involved). The fix design, patch content, and live-target verification were done by the author.
Type of change
Setup guide for the review
main):nxc smb <TARGET> -u <ADMIN_USER> -p <ADMIN_PASS> -M wccBefore: the eight checks listed above report KO on a stock host — e.g.
KO SMB signing enabled (requiresecuritysignature: Value not found)while the host banner itself showssigning:True. After this PR: the same checks report OK with accurate reasons, e.g.OK SMB signing enabled (requiresecuritysignature: Value not found, Registry value not set, but SMB signing is required by the server as observed during session negotiation).requiresecuritysignature=1(e.g. via the "Microsoft network server: Digitally sign communications (always)" GPO) → OK via registry, registry stays authoritative; value0→ KO.
HKLM\SYSTEM\CurrentControlSet\Control\Lsa\RunAsPPL=2→ still OK (PPL without UEFI lock).InstallState=1) → KO.WUServerwith anhttp://URL → KO;https://→ OK; no WSUS configured → OK.LDAPServerIntegrity=2andLdapEnforceChannelBinding=2underHKLM\SYSTEM\CurrentControlSet\Services\NTDS\Parameters.Screenshots
Text before/after on the verification host (replace with real terminal screenshots if desired):
Checklist
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)https://learn.microsoft.com/en-us/windows-server/storage/file-server/smb-signing-overview
requiresecuritysignature):https://learn.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/microsoft-network-server-digitally-sign-communications-always
LdapEnforceChannelBindingunderNTDS\Parameters(ADV190023 / KB4520412): https://support.microsoft.com/en-us/help/4520412https://learn.microsoft.com/en-us/windows-server/security/credentials-protection-and-management/credentials-protection-and-management
RunAsPPLvalues 1/2 (additional LSA protection):https://learn.microsoft.com/en-us/windows-server/security/credentials-protection-and-management/configure-additional-lsa-protection