Conversation
Read-StrongPassword had three defects and became two functions. The policy was inline in a prompt loop where no test could reach it. It is Resolve-UnmetPasswordRequirement now, which answers the unmet requirements as data. Two of its five rules rejected nothing: -notmatch is case-insensitive, so [A-Z] matched a lowercase letter. abcdefg1! was accepted as containing an uppercase one. Both use -cnotmatch now, and a password accepted by the old build can be refused by this one. Two more rules were missing entirely. BitLocker refuses anything outside printable ASCII (0x803100A4) and anything past 256 characters (0x803100AA), and the pre-check knew neither, so a password with a Cyrillic letter passed here and was refused by Windows a moment later. Every class is spelled out in ASCII for the same reason: \d and the old negated class both match beyond it. The BSTR from SecureStringToBSTR was never freed, leaving the plaintext in unmanaged memory for the life of the process. It is freed with ZeroFreeBSTR in a finally, so a throw in between cannot skip it. What that erases is the unmanaged copy; $plain is a .NET string and can only be released. Renamed to Request-StrongPassword: it asks the user, which is what Request-* means here. Closes #75 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
From an independent review of the pull request. The printable-ASCII rule tested its pattern with an operator, and an operator copies what it matched into $Matches. That pattern captures the whole password, so checking it left a second plaintext copy behind - in a change whose point is not leaving one. Every pattern rule uses [regex]::IsMatch now, which touches nothing and is case-sensitive by default, so the defect this issue started from cannot come back by forgetting a letter. The BitLocker retry loop asked for a password again without disposing the one BitLocker had refused, so up to ten SecureStrings stayed alive for the rest of the run. The prompt loop was already doing this; the outer one was not. Two claims were overstated and are corrected. Only the ceiling and the ASCII rule are BitLocker's own refusals; the floor and the four character classes are this script's, and are stricter than BitLocker's default policy, which asks only for a length unless complexity is configured. So a long passphrase Windows would have taken is refused here. The docstring and README say that now, in both directions. Tests: the disposal assertion compared text offsets, where a reworded return would have made IndexOf answer -1 and the comparison pass on nothing; it asks the tree for statement positions instead. Two test cases rendered the same name. The all-spaces case the issue named was never covered. Three copies of the read-a-constant-out-of-the-script block became Get-ScriptConstant, and the new syntax-tree tests share the tree the harness already parsed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #75.
Three defects, not two
The issue named two. A third was measured before the work started and recorded in the issue body:
-notmatchis case-insensitive, so[A-Z]matched a lowercase letter and the uppercase and lowercase rules rejected nothing.abcdefg1!was accepted as containing an uppercase letter, on PowerShell 7.6.5 and Windows PowerShell 5.1.26100 alike.Every pattern rule now uses
[regex]::IsMatch, which is case-sensitive by default. That is deliberate: the defect came from an operator whose case-insensitivity is easy to forget, andIsMatchremoves the way back rather than fixing one instance of it.And two rules that were never there
The review raised a fourth thing, and the owner's call was to close it rather than let BitLocker do it: the pre-check should not hand over passwords BitLocker will refuse. Two of its refusals were not checked at all:
0x803100800x803100810x803100A40x803100AAMeasured against the old rule, all of these were accepted here and refused by Windows a moment later, burning a retry each time: a password ending in a Cyrillic capital, one whose only digit was Arabic-Indic, one with an accented Latin letter, one of 257 characters, one containing a tab.
Resolve-BitLockerFailurealready recognised all four codes when they came back from Windows. What was missing was checking for two of them before asking Windows.Every character class is now spelled out in ASCII —
[0-9]rather than\d, an explicit punctuation range rather than a negated class — because both of those match well beyond ASCII. A Cyrillic capital used to stand in for the required special character.Whose rules these are, stated in both directions
Only the ceiling and the ASCII rule are BitLocker's own. The 8-character floor and the four character classes are this script's. BitLocker's own minimum is group-policy configurable, and its complexity requirement applies only where "Require complexity" is enabled — by default, on a standalone machine, it asks for a length and nothing else.
So the pre-check is stricter than BitLocker, in both directions:
Abcdefg1!passes.That is a deliberate house rule, not an accident — the prompt has always said the password must be complex — but the earlier draft of this description and of
README.mdclaimed the pre-check mirrored BitLocker exactly. It does not, and both now say so.The plaintext in memory
The BSTR from
SecureStringToBSTRwas never freed and its pointer was never held, so nothing could free it. It is held now and freed withZeroFreeBSTRin afinally, which covers the throwing path as well.Two more copies were found and closed on review:
-cnotmatchcopies what it matched into$Matches, and that pattern's capture is the whole password.[regex]::IsMatchtouches nothing.SecureStrings stayed alive for the rest of the run. The prompt loop already disposed its own refused attempts; the outer one now does too.Stated precisely rather than oversold:
ZeroFreeBSTRerases the unmanaged copy.$plainis a .NET string — immutable and garbage-collected — so assigning$nullreleases the reference without erasing the bytes. Nothing in the code, the comments or the documentation claims the plaintext is wiped.Shape
Read-StrongPasswordbecame two functions and one rename:Resolve-UnmetPasswordRequirement— pure, answers the unmet requirements as data, fully tested.Request-StrongPassword— prompts and frees. Renamed fromRead-*becauseAGENTS.mdgivesRequest-*to functions that ask the user, exactly asRequest-DevDriveLabeldoes.$PasswordMinLengthand$PasswordMaxLengthsit with the other constants. The suite reads both out of the script rather than repeating them, so changing either turns the suite red instead of leaving it green against a number nothing uses.Tests
737 to 766. Every assertion about the linear body was proved to have teeth by reintroducing its exact defect on a scratch copy and watching it fail — seventeen mutations, each caught, the script restored byte for byte each time:
One slipped on an earlier round: putting
\dback was caught by nothing, because the non-ASCII fixture had an ordinary digit beside it. A case whose only digit is Arabic-Indic closed that.Reviewed twice
First round, before the first commit. The floor's value was pinned nowhere, so changing it in the script left the suite green; the prompt assertion was a negative against today's wording that a parameter default would have slipped past; the caller count was anchored to a variable name rather than to the call; nothing pinned
SecureStringToBSTRas sitting outside thetry, where moving it in would hand thefinallyan already-freed pointer on the next pass; the@()ancestor walk was unbounded;$plain = $nulland the disposal were covered by nothing.Second round, on the pull request as it stood. The
$Matchescopy and the retry loop's undisposed attempts, both above. The claim that every rule mirrored a BitLocker refusal, and the reverse cost that was never named — both corrected here and inREADME.md. The disposal assertion compared text offsets, whereIndexOfanswering-1on a reworded return would have let it pass on nothing; it asks the tree for statement positions now. Two test cases rendered an identical name. The all-spaces password the issue itself named as untested was still untested. Three copies of the read-a-constant-out-of-the-script block becameGet-ScriptConstant, and the new syntax-tree tests share the tree the harness already parses.Declined, with reasons. Pluralising "at least 1 characters" — the only caller passes a constant of 8, and a conditional inside a message for a value nobody passes is noise. Hoisting the four class messages into a shared array — an assertion that spells out the expected sentence reads better than one that indexes into a list. Making the prompt say "characters" instead of "chars" — the prompt is a compact hint, and the failure messages are where the rule is stated precisely.
Behaviour change, stated plainly
A password accepted by the previous build can be refused by this one, in two ways: the case rules now work, and non-ASCII and over-long passwords are refused. That is the point of the change.
README.md's caveat about the password rule described an intention rather than the behaviour before this; it now describes what is enforced and whose rule each part is.Live run
Confirmed on this machine against a scratch
.vhdx: the prompt refuses short, missing-class and non-ASCII answers one requirement at a time and accepts a compliant one, and BitLocker took that password without complaint.Not fixed here, seen during that run
Three defects unrelated to this issue turned up and are filed separately — the plan promising an initial optimization job even when deduplication is skipped and listing the trusted designation out of order (#101), and five BitLocker cmdlets printing their return objects mid-run (added to #82).
🤖 Generated with Claude Code