v2026.09.25.002: an unknown password length is not scored or reported as 0 - #3
Open
cdburgess75 wants to merge 2 commits into
Open
cdburgess75 wants to merge 2 commits into
cdburgess75 wants to merge 2 commits into
Conversation
… as 0 $Script:MinPasswordLen started at 0, and only the Assessment Engine's 'Password policy' check set it, by parsing 'net accounts'. When the engine aborted or was disabled, or 'net accounts' gave no 'Minimum password length' value, it stayed 0. The scoring then took 20 points and the CIS block added the High finding "Password minimum length is 0 (CIS 1.1.1)". Battlefield alerts on every High finding and maps that title to the VULN password-policy-blank. That is a collection failure scored and alerted as a vulnerability, which ADR 0009 rules out. It now starts at $null and is set only from a number actually read ([int]'' is 0, so a length line with no number no longer parses as 0). The scoring and CIS 1.1.1 skip a $null length, and the log says it is unknown. Both guards are needed: $null -lt 8 is $true, so without them an unknown length would still cost 10 points and raise a finding with a blank length. A length that was read, including a real 0, is scored and reported exactly as before. tests/Test-EngineScope.ps1 now runs the CIS Benchmark block verbatim too and asserts the CIS 1.1.1 finding. The engine-aborts and engine-disabled scenarios no longer expect -20. New scenarios cover net accounts returning nothing, no length line, a length line with no number, and read lengths of 0, 6 and 10. It fails 10 assertions against v2026.09.25.001 and passes here. It is a mock test, not a Windows run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The minimum password length read from 'net accounts', as a JSON number, or null when it was not read. Without it Battlefield cannot tell "unknown" from "8 or more", because neither sends a CIS 1.1.1 finding. Battlefield stores the whole report as JSONB (ADR 0002) and requires only hostname, so it accepts the field unchanged; nothing displays it yet. Part of v2026.09.25.002, which has not shipped, so the version is not bumped again; both changelog entries are extended. tests/Test-EngineScope.ps1 asserts the field as serialized JSON in every scenario: the read length as a number, null when unknown. Removing the field, sending it as a string, or sending an unknown length as 0 each fails the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is stacked on #2, which is stacked on #1. Its base is
fix/engine-child-scope, so the diff shows only this change.mainbefore merging it.The bug
$Script:MinPasswordLenstarted at0in the Phase 2 init. Only the engine'sPassword policyblock set it, by parsingnet accounts.0whenever the length was never read:net accountsprinted nothing;net accountsprinted noMinimum password lengthline. This is probably the case on any non-English Windows, whosenet accountsoutput is localized.0as a real value:if ($Script:MinPasswordLen -eq 0) { -20 }.Password minimum length is 0 (CIS 1.1.1). Battlefield'sconditions_from(bf/alerts.py) raises an alert for every High finding, andbf/findings.pymaps^Password minimum length is 0\bto the VULNpassword-policy-blank.Changes
$Script:MinPasswordLenstarts at$null(unknown).[int]($line -replace '[^\d]',''), and[int]''is0, so a length line with no number also read as a real 0.$nulllength.$nulllength and logs[CIS 1.1.1] Password minimum length: unknown - not checked.minimum length unknown ... not scoredwhen it finds no value.$null -lt 8is$true, so without the guards an unknown length would still cost 10 points and raise a finding with a blank length. The mutation runs below show this.password_min_length: the length as a JSON number, ornullwhen it was not read.hostnameand stores the whole report as JSONB (ADR 0002), so it accepts the field with no server change. Nothing displays it yet.Scoring: what changes when this ships
sync_run_alertsresolves a finding alert assystemonce a later run no longer carries it. The falsePassword minimum length is 0 (CIS 1.1.1)alerts on these devices therefore clear on each device's first run of this version.password-policy-blankfrom the VULN-scored findings.LATEST_SK_VERSIONinbattlefield.envwhen this ships.net accounts, ornet.exefailing.Tests
tests/Test-EngineScope.ps1from #2 is extended:netmock prints realistic Englishnet accountsoutput with the scenario's length.net-accounts-empty: no output. Expects 100 and no finding.net-accounts-no-length: output without the length line. Expects 100 and no finding.net-accounts-no-number: a length line with no number. Expects 100 and no finding.password-length-0,-6and-10: expect 80, 90 and 95. The first two also expect the finding, which for 0 has the exact title Battlefield maps.password_min_lengthas serialized JSON: the read length as a number, ornullwhen it was not read.Results (20 checks: 19 scenarios plus the scope check):
Against v2026.09.25.001: engine results reach the payload and the score #2's head it fails 10 assertions: a score of 80 and the
...is 0 (CIS 1.1.1)finding in each of the 5 unknown-length scenarios. It passes on this branch.Four mutations each fail the test:
0[int]parse0Test-BiosDateandTest-DeviceIdentitystill pass.PSScriptAnalyzer was not run locally; CI runs it.
It is a mock test, not a Windows run.
Before merging: one real run
Run this in an elevated Windows PowerShell 5.1. Any machine will do, and RLG-JANE-PC or RLG-DCFS also cover #1.
When it finishes, check the newest
C:\ProgramData\ShellKnight\Logs\ShellKnight_*.log:Assessment Engine skipped,Device identity skippedorPassword policy skippedline.Password policy: minimum length Nline. If it saysunknowninstead, that box is a real example of this bug; note whatnet accountsprints there.[CIS 1.1.1]line agrees with it.C:\ProgramData\ShellKnight\JSON\*.json:password_min_lengthis N, as a number;nullif the log saidunknown.findingshasPassword minimum length is N (CIS 1.1.1)only if N is below 8.Battlefield push OK - run_id: <n>.Then remove the test task with
schtasks /delete /tn SK-Test /f.The unknown path is covered by the mock test only. On an enrolled box,
config.jsonturns the Battlefield push on, and I didn't want the test steps to push a run with the engine disabled to production.Left alone (follow-ups)
password_min_lengthyet. A device page or compliance view could show "not collected" fornull.net accountsis parsed by its English label. On a localized Windows the length is now unknown rather than 0. ReadingMinimumPasswordLengthfromsecedit /exportorNetUserModalsGetwould work in any language.Get-MpComputerStatusis not found. In pwsh 7 that is a terminating error even with-ErrorAction SilentlyContinue, and I expect the same in 5.1. The CIS block then stops there and skips its summary line. This affects log text only, and the test excludes it explicitly.maincontain em-dashes (lines 350, 352, 398, 2350 and 2442 on v2026.09.25.001: engine results reach the payload and the score #2's head). They are harmless in comments, and I left them alone to keep the diff focused.🤖 Generated with Claude Code