Repository navigation
Darling upgrade: a staging folder with [ or ] in its name no longer copies nothing under a success message (#4745) - #4762
Merged
Conversation
…s the copy or the backup hardening match nothing (#4745) PowerShell reads [ and ] in a -Path value as wildcard characters. The upgrade script copied a folder -Source with Copy-Item -Path over "$Source\*", so a staging folder named build[1] matched nothing, threw nothing, and the script still printed "New build in place." over the old build. The install script's search for older darling.json.bak-* files used -Path the same way and found none, and its Get-Acl/Set-Acl -Path calls could resolve to a same-named file in a sibling folder. - upgrade-darling.ps1: the folder copy is now Get-ChildItem -LiteralPath $Source -Force | Copy-Item -Destination $InstallRoot -Recurse -Force. - upgrade-darling.ps1: for a folder -Source, the script compares the SHA-256 of the service executable in the source and in the install root before it says "New build in place.", and stops with a plain message if they differ. - install-darling.ps1: every Test-Path, Copy-Item, Get-ChildItem, Get-Acl and Set-Acl call on a path under the install folder now uses -LiteralPath. - Tests: text pins for both statements, a parsed-script census that fails on any wildcard-expanding cmdlet given a bare path, the guard's place between the copy check and the success message, and behavioural runs of the shipped copy statement (staging folder a[x]b) and of the shipped executable check.
… scripts now use (#4745) DarlingFileSecurityTests, DarlingInstallLocationTests and DarlingRuntimePreflightTests quoted the -Path statement text the scripts had before #4745. Each pin now quotes the -LiteralPath form: Get-Acl and Set-Acl on $secretFile, Copy-Item of the sample config, and the folder copy in the upgrade script. The Set-Acl loop in TheInstaller_NeverAclsTheInstallTree_OnlyTheCredentialFiles searched "Set-Acl -Path ", which no longer occurs in code, so it counted nothing. It now searches "Set-Acl -LiteralPath " with a window wide enough for the whole 32-character call, and a DoesNotContain for "Set-Acl -LiteralPath $root" sits beside the -Path one.
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.
Fixes #4745
Why
PowerShell reads
[and]in a-Pathvalue as wildcard characters. The Darling upgrade script copied a folder-SourcewithCopy-Item -Path (Join-Path $Source '*'), so a staging folder named likebuild[1]matched nothing, threw nothing, and the script still printed "New build in place." over the old build. The install script's search for olderdarling.json.bak-*files used-Paththe same way and found none to harden under such an install folder.Measured on Windows PowerShell 5.1.26100 and PowerShell 7.6.6 (identical results), with a folder
a[x]bholding two files and a subfolder with one file:Copy-Item -Path (Join-Path $Source '*') -Destination $dst -Recurse -Force(old)Get-ChildItem -LiteralPath $Source -Force | Copy-Item -Destination $dst -Recurse -Force(new)The other wildcard-expanding calls under the same roots misread such a name the same way:
Get-ChildItem -Pathfound no backups to harden,Get-Acl -PathandSet-Acl -Pathcould resolve to a different path than the backup, and a positionalTest-Pathreturned False for a file that exists. Each now takes-LiteralPath.A destination containing
[is not affected: a copy into an existingd[yz]elanded there and left a same-shaped siblingdyeempty.What changes
Darling/tools/upgrade-darling.ps1Get-ChildItem -LiteralPath $Source -Force | Copy-Item -Destination $InstallRoot -Recurse -Force.-LiteralPathonCopy-Itemalone would make the*literal too, so the listing is piped in and each item binds its own literal path. The comment that quoted the old form is updated.-Sourceonly, it compares the SHA-256 ofPerformanceMonitor.Darling.Service.exein the source and in the install root. If they differ it stops with "The copy reported success, but the installed PerformanceMonitor.Darling.Service.exe is not the new build's, and the service is still STOPPED - re-run this script with the same arguments." A zip source is not checked; it is already read with-LiteralPath.Darling/tools/install-darling.ps1: every wildcard-expanding cmdlet that took a path under$rootnow uses-LiteralPath. Sites changed (line numbers on dev):Test-Path $serviceExeTest-Path $configPath, 1037Test-Path $samplePathCopy-Item $samplePath $configPath, nowCopy-Item -LiteralPath $samplePath -Destination $configPathGet-ChildItem -Path $root -Filter 'darling.json.bak-*'(the reported site)Set-Acl -Path $secretFile; 1200 and 1210Get-Acl -Path $secretFileTest-Path $viewerExeLeft alone on purpose:
Join-PathandNew-Item(they do not expand wildcards); the twoGet-ItemProperty -Path 'HKLM:\...'lookups (constant registry paths, not under$Source,$InstallRootor$root);Split-Path; anduninstall-darling.ps1, whoseTest-PathandRemove-Itemcalls take a shortcut path and the ProgramData folder, not a path under those roots.Darling/Darling.Tests/DarlingDeployStaleFileTests.cs: the new tests below.RunWindowsPowerShellalso now starts Windows PowerShell with the machine-levelPSModulePath. Started frompwsh(the CI runner's default shell), Windows PowerShell 5.1 inherited the parent's module directories and could not findGet-FileHash("The term 'Get-FileHash' is not recognized"); the executable-check test hit this when run from apwshwindow, and the machine-level value is what an operator's own 5.1 session starts with.New tests (all in
DarlingDeployStaleFileTests, which already parses these scripts):TheDeployScript_CopiesAFolderSourceByLiteralPath_NotThroughAWildcard: text pin on the new copy statement; the old form must be gone.TheInstallScript_FindsOlderConfigBackupsByLiteralPath: text pin on the.baksearch.TheUpgradeAndInstallScripts_NameEveryPathLiterally: census over the parsed scripts (Windows PowerShell AST). Any of 25 wildcard-expanding path cmdlets (aliases resolved) that is first in its pipeline and has no-LiteralPath, or that has-Path, fails it, so a positional path or a new-Pathsite is caught. ConstantHKLM:paths are exempt.TheDeployScript_ChecksTheInstalledServiceExecutableBeforeItReportsTheNewBuildInPlace: the hash check sits between the$copiedcheck and "New build in place.", is gated on-not $sourceIsZip, and uses-LiteralPath.TheShippedFolderCopy_CarriesEveryFileOutOfAStagingFolderWhoseNameHasBrackets: runs the copy statement the script really has (found by its place in the script, not its wording) against a staging foldera[x]bwith a subfolder and a hidden file; every file arrives, a file the install already had is overwritten, and one the new build does not ship survives.TheShippedExecutableCheck_StopsOnlyWhenAFolderCopyLeftADifferentServiceExecutable(3 cases): the shipped check, lifted out and run: same bytes pass, different bytes stop with the message, a zip source is skipped.Older pins that quoted the old statement text now quote the
-LiteralPathforms:DarlingFileSecurityTests.InstallScript_SetsTheOwnerOnTheCurrentDescriptor_SoTheHardenedDaclSurvives,DarlingInstallLocationTests.LocationGuard_RunsBeforeAnythingIsInstalled,DarlingInstallLocationTests.TheInstaller_NeverAclsTheInstallTree_OnlyTheCredentialFiles(itsSet-Aclloop now searchesSet-Acl -LiteralPathwith a 40-character window, and aDoesNotContainforSet-Acl -LiteralPath $rootsits beside the-Pathone),DarlingInstallLocationTests.TheWritableTreeCheck_RunsBeforeEitherScriptChangesAnythingandDarlingRuntimePreflightTests.RuntimeGate_RunsBeforeTheServiceExeIsEverInvoked. Run against dev's unchanged scripts, all five fail (the upgrade script's folder-copy marker also fails on its own when only that script is put back).Test plan
DarlingDeployStaleFileTestsfailed. The census listed exactly the 11 sites above (1 in the upgrade script, 10 in the install script). The behavioral copy test failed withExpected: "new one" Actual: "old one", that is, nothing was copied and nothing threw.DarlingDeployStaleFileTests: 20 of 20 pass after the change, run from apwshwindow (the case where Windows PowerShell inherits pwsh's module directories, which thePSModulePathchange handles).dotnet build Darling/Darling.Tests/Darling.Tests.csproj: 0 Warning(s), 0 Error(s), before and after merging origin/dev.TheShippedScripts_*tests, part of the 20).Darling.Testssuite withoutDARLING_TEST_PG, after merging origin/dev: Total: 17158, Errors: 0, Failed: 1, Skipped: 1121, Not Run: 1 (1119 of the skips needDARLING_TEST_PGorDARLING_TEST_PGRUNTIME; the other 2 are conditions of the machine the run was on). The one failure,ManagedConfMigrationRunnerTests.RunStepA_Mismatch_RestoresAndListsTheKey(simulated crash between steps), does not come from this change:ManagedConfMigrationStepsTestssets the staticManagedConfMigrationSteps.FailBetweenStepsto throw, neither class carries a[Collection]attribute, and the stack shows that hook firing inside the runner test.ManagedConfMigrationRunnerTestsalone: 24 of 24 pass; run together withManagedConfMigrationStepsTests, a runner test fails again. This PR touches neither class norManagedConfMigrationSteps. The eleven classes that read these scripts, run alone first: Total: 209, Failed: 0, Skipped: 0.CHANGELOG
SECTION: Fixed
ENTRY:
REF:
[A folder name with [ or ] breaks the Darling upgrade copy and the install's backup hardening #4745]: A folder name with [ or ] breaks the Darling upgrade copy and the install's backup hardening #4745