Sanitize ScriptArguments in AzurePowerShellV2-V5 and ServiceFabricPowerShellV1 (MSRC 115118) - #22171
Merged
wawanawna merged 27 commits intoMay 19, 2026
Merged
Conversation
MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dispatcher Covers the new dispatcher introduced by PR #22163 (MSRC 115118): - Test-ShouldUseSanitizer gate logic across all combinations of org-level`n toggle (Get-SanitizerCallStatus) and per-task pipeline FF (Get-VstsPipelineFeature), including throw, missing-cmdlet, and audit fallthrough. - Invoke-ScriptArgumentSanitization dispatch: no-op when either gate is off; passes through clean args; re-throws localized ScriptArgsSanitized verbatim; swallows unexpected sanitizer exceptions into telemetry; handles empty input. - Publish-SanitizerErrorTelemetry emits the expected ##vso[telemetry.publish] command shape. Follows the precedent set by PR #21947's Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g for re-throw The previous implementation caught any exception from Protect-ScriptArguments and compared its message to Get-VstsLocString -Key 'ScriptArgsSanitized' to decide whether to re-throw (rejection) or swallow into telemetry (unexpected error). Two problems: 1. Protect-ScriptArguments throws Get-VstsLocString -Key 'PS_ScriptArgsSanitized' resolved from the Sanitizer module's resjson, while the dispatcher compared against 'ScriptArgsSanitized' resolved from the task's resjson. The two strings are translated independently. fr-FR uses 'coche' in one and 'backtick' in the other, so on a French agent the -eq comparison returned false and a real sanitizer rejection was silently swallowed into telemetry - the task continued executing the injected arguments. The en-US strings happen to be identical, which is why this latent bug went unnoticed. 2. Even when Protect-ScriptArguments crashes with an unrelated error, the safer default for a security gate is to fail the task rather than execute un-vetted arguments. The dispatcher now fails closed: any exception out of Protect-ScriptArguments produces a telemetry record with the underlying message + stack and re-throws the task-localized 'ScriptArgsSanitized' message to the caller. No locale- dependent string comparison remains. Tests cover all three throw paths (matching message, locale-drifted message, unexpected crash) - all must fail closed with 'ScriptArgsSanitized'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rebase auto-resolve introduced a 'comma on own line' formatting glitch in all AzurePowerShellV5 locale resources.resjson files (and the V5 generated mirrors). Moves the comma to the end of the preceding line, matching the rest of the resjson format and addressing review feedback from @nemanjarogic. No content / behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closed
9 tasks
Contributor
Author
|
@nemanjarogic this replaces #22163 — same branch, same commits (tip |
nemanjarogic
approved these changes
May 13, 2026
MSRC 115118 added a new file (Invoke-ScriptArgumentSanitization.ps1) to Tasks/Common/Sanitizer/. ci/filter-tasks.js requires every task whose Common dependencies change to have its own version bumped, otherwise the 'Filter out unchanged tasks' build step fails with 'The following tasks should have their versions bumped due to changes in common: ...'. This matches the precedent established by PR #19834 (Add audit event for Shell Tasks Validation warnings), which made a similar Common/Sanitizer change and bumped the same set of consumer tasks. Bumps (patch +1, except where the existing _generated mirror's maxVersion already exceeded source — in those cases bumped to maxVersion+1 to satisfy BuildConfigGen's monotonicity invariant): AzureFileCopyV1 1.274.0 -> 1.274.1 AzureFileCopyV2 2.274.0 -> 2.274.1 AzureFileCopyV3 3.274.0 -> 3.274.1 AzureFileCopyV4 4.274.0 -> 4.274.2 (Node24_1 was already 4.274.1) AzureFileCopyV5 5.274.0 -> 5.274.2 (Node24_1was already 5.274.1) AzureFileCopyV6 6.274.2 -> 6.274.4 (Node24_1 was already 6.274.3) PowerShellV2 2.274.0 -> 2.274.1 PowerShellOnTargetMachinesV3 3.273.0 -> 3.273.1 SqlAzureDacpacDeploymentV1 1.274.0 -> 1.274.1 SqlDacpacDeploymentOnMachineGroupV0 0.274.0 -> 0.274.1 WindowsMachineFileCopyV1 1.243.0 -> 1.243.1 WindowsMachineFileCopyV2 2.251.0 -> 2.251.1 Also updates _generated/ mirrors to match (re-run via BuildConfigGen with --write-updates --current-sprint 274). Most of the new files in _generated/ AzureFileCopyV4 and _generated/AzureFileCopyV5 are byte-identical copies of source (54/56 unchanged); they exist because BCG materializes the Default mirror whenever any non-Default config (here Node24_1) is registered. Precedent: _generated/AzureFileCopyV6/ has been checked in this way for the same reason. No source / behavior changes in any of the 12 bumped tasks. The Sanitizer dispatcher (Invoke-ScriptArgumentSanitization.ps1) is consumed only by AzurePowerShellV2-V5 and ServiceFabricPowerShellV1; the 12 tasks bumped here all already have org-level + per-task FF dispatch and gain nothing from the new helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e check) ci/check-downgrading.js failed on the previous push because four tasks had Minor versions older than the current sprint (274). The check enforces minor >= currentSprintVersion. Patch-only bumps were not enough. ServiceFabricPowerShellV1 1.228.2 -> 1.274.0 PowerShellOnTargetMachinesV3 3.273.1 -> 3.274.0 WindowsMachineFileCopyV1 1.243.1 -> 1.274.0 WindowsMachineFileCopyV2 2.251.1 -> 2.274.0 None of these have Node24 generated mirrors so no _generated/ updates are needed (BuildConfigGen verified clean for all four). ServiceFabricPowerShellV1 is one of the 5 sanitizer-adopting tasks, so the Risk Assessment / version table in the PR description has been updated to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The original retrofit added the localized 'ScriptArgsSanitized' message to task.loc.json and Strings/resources.resjson/<locale>/resources.resjson, but forgot to add the canonical source-of-truth entry to task.json's 'messages' block for AzurePowerShellV2-V5 and ServiceFabricPowerShellV1. The CI 'Verify task source changes' step runs 'node make.js build', which regenerates task.loc.json and en-US/resources.resjson from task.json.messages. Without the source entry, that step stripped the ScriptArgsSanitized line on every rebuild — leaving the dispatcher's localized throw with no message to resolve at runtime in en-US. Fix: add the message string to task.json's messages block for each of the 5 tasks. Then runs 'node make.js build' against each so task.loc.json and en-US resources.resjson are emitted by the canonical build path; BCG re-syncs the V4/V5 generated mirrors. No functional change to non-en-US localizations — they continue to use the existing translations from the original retrofit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The CI's static loc-resource check ('Find missing string in .ps1/.psm1')
scans each PS file under Tasks/Common/<Module>/ and requires every
Get-VstsLocString -Key 'X' call to have 'X' declared in that module's
module.json. My dispatcher lives in Tasks/Common/Sanitizer/ and threw with
the task-level key 'ScriptArgsSanitized', which is declared in each task's
task.json but NOT in the Sanitizer module.json — so the test failed.
Fix: switch the throw to the module's own 'PS_ScriptArgsSanitized' key,
which is already declared in Tasks/Common/Sanitizer/module.json and its
10 locale resjson files. This makes the dispatcher self-contained at the
module level (the principle the static check enforces).
Behavior is unchanged: the en-US text of 'PS_ScriptArgsSanitized'
(module) and 'ScriptArgsSanitized' (task) are byte-identical, and the
fail-closed logic is preserved. Non-en-US locales will see the module's
translation instead of the task's; this is actually a small improvement
because the dispatcher and the underlying Protect-ScriptArguments now
speak the same key — no locale-drift surface remains at all.
Updates the dispatcher's L0 tests to assert on 'PS_ScriptArgsSanitized'
accordingly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ter tasks
Earlier commits in this PR added 'ScriptArgsSanitized' to task.json /
task.loc.json / 10 locale resjsons for AzurePowerShellV2-V5 and
ServiceFabricPowerShellV1, mirroring the convention used by PowerShellV2
and the other inline-sanitizer tasks. After consolidating the dispatch
logic into Tasks/Common/Sanitizer/Invoke-ScriptArgumentSanitization.ps1
and switching its throw to the module's own 'PS_ScriptArgsSanitized' key,
those task-level entries became unreachable - no .ps1 in these 5 tasks
calls Get-VstsLocString -Key 'ScriptArgsSanitized' anymore (verified by
grep).
This commit removes the dead entries from each of the 5 tasks:
- task.json messages -> rebuild via 'node make.js build --task X' to
regenerate task.loc.json and en-US/resources.resjson canonically.
- 9 non-en-US locale resjsons -> manually stripped.
BuildConfigGen re-syncs the AzurePowerShellV4/V5 generated mirrors.
Net effect: only the live 'PS_ScriptArgsSanitized' key (declared in
Tasks/Common/Sanitizer/module.json + its 10 locales) is referenced at
runtime by any code path in this PR. The diff vs master shrinks because
we're undoing additions made earlier in the same PR.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lint)
The Tests/L0 'Find missing string in .ps1/.psm1' test does a case-insensitive
substring search for 'Get-VstsLocString' on every line in every .ps1/.psm1
under Tasks/ (excluding Tests/). When it finds the substring, it tries to
parse the line as a 'Get-VstsLocString -Key ...' call. If the parse fails,
it asserts 'Bad format string'. The test doesn't distinguish code from
comments.
My explanatory comment mentioned the cmdlet name verbatim, so the test
flagged it. Reworded the comment to describe the behavior ('loc resource
references') without using the literal cmdlet name. No code change.
Also adds C:/ado/temp/check-loc-strings.js (local equivalent of the CI
step) so future iterations of this PR can be validated before pushing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… cleanup
After the previous commit removed the dead ScriptArgsSanitized entries from
the source task.json files, _generated/AzurePowerShellV4_Node24/task.json
and V5_Node24/task.json were updated by BuildConfigGen but their dependent
task.loc.json + en-US/resources.resjson were not re-emitted from the build
pass. CI 'Verify task source changes' flagged them.
Re-ran 'node make.js build --task AzurePowerShellV{4,5}' which regenerates
all config mirrors (Default + Node24_1) from the updated task.json sources.
Four files updated per task (Default and Node24 both have task.loc.json +
en-US resjson).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…US resjson
Tasks/AzurePowerShellV4 and V5 use the _buildConfigMapping mechanism, so
'node make.js build --task X' regenerates only the _generated/ mirrors —
the source Tasks/<task>/task.loc.json and Strings/.../en-US/resources.resjson
are hand-maintained / BCG-synced, not build-regenerated.
When I removed ScriptArgsSanitized from task.json earlier, the build's regen
pass updated V2/V3/SFV1 source loc files (those tasks don't have build
configs) but left V4/V5 source loc files with the now-stale entry. BCG
verify caught the divergence: V4 task.loc.json had 9 messages (including
ScriptArgsSanitized) while V4 task.json had 8.
Manually stripped ScriptArgsSanitized from Tasks/AzurePowerShellV{4,5}/
task.loc.json + Strings/.../en-US/resources.resjson (non-en-US locales
were already cleaned in a prior commit). BCG verify now reports clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The per-task L0 tests for AzurePowerShellV2-V5 and ServiceFabricPowerShellV1 dot-source Tests/lib/Initialize-Test.ps1 (which mocks Import-Module so the Sanitizer common module isn't actually loaded), then directly invoke the task's main script. After this PR's retrofit, those main scripts call Invoke-ScriptArgumentSanitization — which is undefined under the test mock, so PowerShell aborts with 'term not recognized'. MacOS/Linux CI passed because per-task PowerShell-based L0 suites only run on Windows. My local validation also missed it; adding the per-task L0 step to validate-pr-22171.ps1 so future iterations catch this class of failure before push. Fix: add Register-Mock Invoke-ScriptArgumentSanitization (stub no-op) right after Initialize-Test.ps1 in each of the 40 affected source test files across the 5 tasks. BuildConfigGen propagated the mock to the V4/V5 generated mirror Tests/ trees automatically. Local: all 5 tasks now pass L0 (8-13 passing each). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
manolerazvan
approved these changes
May 13, 2026
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
sanjuyadav24
approved these changes
May 18, 2026
Contributor
Author
|
Re-running CI: the previous
None of the modified tasks (AzurePowerShellV2-V5, ServiceFabricPowerShellV1, the version-only bumps on AzureFileCopy*, PowerShellV2, SqlAzureDacpacDeploymentV1, SqlDacpacDeploymentOnMachineGroupV0, PowerShellOnTargetMachinesV3, WindowsMachineFileCopy*) failed any actual assertion. /azp run |
The sprint version on master rolled from 274 to 275 (via #22175), so ci/check-downgrading.js now rejects every task left at *.274.*. Bump each task's Default minor to 275, with the smallest patch >= master's current patch. Tasks with a Node24_1 BuildConfig get Node24_1 = Default + 1 (per the same convention used earlier in this PR). New versions: AzureFileCopyV1 1.275.0 AzureFileCopyV2 2.275.0 AzureFileCopyV3 3.275.0 AzureFileCopyV4 4.275.0 / 4.275.1 (Node24_1) AzureFileCopyV5 5.275.0 / 5.275.1 (Node24_1) AzureFileCopyV6 6.275.2 / 6.275.3 (Node24_1) AzurePowerShellV2 2.275.0 AzurePowerShellV3 3.275.0 AzurePowerShellV4 4.275.2 / 4.275.3 (Node24_1) AzurePowerShellV5 5.275.4 / 5.275.5 (Node24_1) PowerShellOnTargetMachinesV3 3.275.0 PowerShellV2 2.275.0 ServiceFabricPowerShellV1 1.275.0 SqlAzureDacpacDeploymentV1 1.275.0 SqlDacpacDeploymentOnMachineGroupV0 0.275.0 WindowsMachineFileCopyV1 1.275.0 WindowsMachineFileCopyV2 2.275.0 Source task.json/task.loc.json, _generated mirrors, _buildConfigMapping entries, and *.versionmap.txt files are all kept consistent. No code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback on PR #22171: 1. ServiceFabricPowerShellV1 was pinning VstsTaskSdk 0.11.0, but Get-VstsPipelineFeature (used by the new Invoke-ScriptArgumentSanitization dispatcher) only landed in 0.20.0+. The dispatcher's Test-ShouldUseSanitizer falls open to no-op when the cmdlet is missing, which silently made the SF V1 sanitization inert in production - exactly the regression the new helper was designed to surface only on real failures. Bumped to 0.21.0 to match AzurePowerShellV5 / PowerShellV2 (sister sanitizer-adopting PowerShell tasks). Verified safe: - SF V1 only uses Get-VstsInput / Trace-VstsEnteringInvocation, which are stable across SDK versions. - All SF V1 L0 tests Register-Mock Invoke-ScriptArgumentSanitization at top, so the actual SDK call is never exercised in tests. - Tests/lib/Initialize-Test.ps1 already provides a global stub for Get-VstsPipelineFeature, so the dispatcher's cmdlet-presence check is neutralized in test runs regardless of SDK version. 2. L0Invoke-ScriptArgumentSanitization.ps1 header comment referenced the predecessor fork PR #22163 (which this PR explicitly replaces); updated to #22171. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nemanjarogic
approved these changes
May 19, 2026
wawanawna
enabled auto-merge (squash)
May 19, 2026 06:42
tarunramsinghani
approved these changes
May 19, 2026
wawanawna
deleted the
users/wawanawna/msrc-115118-azurepowershell-sanitizer
branch
May 19, 2026 07:20
wawanawna
pushed a commit
to wawanawna/azure-pipelines-tasks
that referenced
this pull request
May 26, 2026
Fix Expand-EnvVariables.ps1 so that YAML folded scalars ('arguments: >'), which emit literal newlines between joined lines, don't make the env-name greedily span across lines, miss the lookup, and fall through to the sanitizer allowlist.
Companion to PR microsoft#22181 / issue microsoft#22173, which fixed the same bug in the TypeScript twin (Tasks/PowerShellV2/helpers.ts -> expandPowerShellEnvVariables). The PowerShell expander shares the design (and the bug) but had not been ported. This PR ('AzurePowerShellV2-V5 + ServiceFabricPowerShellV1 sanitization', microsoft#22171) is the first time Expand-EnvVariables is wired into a task family where folded-scalar arguments containing $env:* references is a documented pattern, so fixing it here removes a known false-positive before rollout.
Adds five regression cases to L0Expand-EnvVariables (LF, CRLF, tab, multiple env vars separated by LF, and the full folded-scalar reproducer from the AzurePowerShell scenario). Verified the new tests fail on the unfixed expander and pass with the fix; remaining Common/Sanitizer L0s (Protect-ScriptArguments.Passes/Throws, Get-SanitizedArgumentsArray.*) still green.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced May 26, 2026
Closed
wawanawna
added a commit
that referenced
this pull request
May 28, 2026
…— PS twin of #22181 (#22207) * Common/Sanitizer: terminate $env: name on whitespace (LF/CR/TAB) Fix Expand-EnvVariables.ps1 so that YAML folded scalars ('arguments: >'), which emit literal newlines between joined lines, don't make the env-name greedily span across lines, miss the lookup, and fall through to the sanitizer allowlist. Companion to PR #22181 / issue #22173, which fixed the same bug in the TypeScript twin (Tasks/PowerShellV2/helpers.ts -> expandPowerShellEnvVariables). The PowerShell expander shares the design (and the bug) but had not been ported. This PR ('AzurePowerShellV2-V5 + ServiceFabricPowerShellV1 sanitization', #22171) is the first time Expand-EnvVariables is wired into a task family where folded-scalar arguments containing $env:* references is a documented pattern, so fixing it here removes a known false-positive before rollout. Adds five regression cases to L0Expand-EnvVariables (LF, CRLF, tab, multiple env vars separated by LF, and the full folded-scalar reproducer from the AzurePowerShell scenario). Verified the new tests fail on the unfixed expander and pass with the fix; remaining Common/Sanitizer L0s (Protect-ScriptArguments.Passes/Throws, Get-SanitizedArgumentsArray.*) still green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump dependent task versions for Common/Sanitizer change ci/filter-tasks.js requires that every task with Tasks/Common/Sanitizer in its make.json have its source version bumped in the same PR that modifies the Sanitizer module (same policy that drove the bump batch in #22171). Patch is bumped by +1 on tasks without a Node24 build config and by +2 on tasks with a Node24_1 config so that Node24_1 stays Default+1. _generated/<task>{,_Node24}/task.json + task.loc.json + <task>.versionmap.txt mirrors are updated to match for the five tasks with Node24_1 buildconfigs (AzureFileCopy V4/V5/V6, AzurePowerShell V4/V5). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Regenerate _generated mirrors via BuildConfigGen Updates _buildConfigMapping version strings in _generated task.json/task.loc.json to match the bumped Patch values. Produced by: dotnet run --project BuildConfigGen/BuildConfigGen.csproj -- `n --task "AzureFileCopyV4|AzureFileCopyV5|AzureFileCopyV6|AzurePowerShellV4|AzurePowerShellV5" `n --write-updates Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Uladzimir Tratsiakou (Vladimir/Vova) <utratsiakou@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 12, 2026
wawanawna
added a commit
that referenced
this pull request
Jun 25, 2026
…llOnTargetMachinesV3 and AzureCloudPowerShellDeploymentV1 (#22286) * Wire Invoke-ScriptArgumentSanitization on POTM/AzureCloud (MSRC 115118 Plan 1) Closes the two remaining PowerShell command-injection sinks tracked by MSRC 115118 that pass user-controlled task inputs straight to Invoke-Expression, by wiring the shared Invoke-ScriptArgumentSanitization dispatcher (Tasks/Common/Sanitizer/, shipped by #22171 and hardened in #22273) onto each input. Validate-only: the dispatcher throws on a violation when both the org-level 'Enable shell tasks arguments validation' toggle AND the new per-task pipeline feature flag are on, which fails the task before the iex runs. Otherwise it is a no-op. Bug 1 - PowerShellOnTargetMachinesV3 / NewPsSessionOptionArguments Sink: Utility.ps1:57-59 Invoke-Expression "New-PSSessionOption $arguments" Wire: PowerShellOnTargetMachines.ps1 calls Invoke-ScriptArgumentSanitization with TaskName 'PowerShellOnTargetMachinesV3' and PipelineFeatureFlagName 'EnablePowerShellOnTargetMachinesArgumentsSanitization' BEFORE the call that flows the input into Get-NewPSSessionOption. Module already bundled (RemoteDeployer + Sanitizer in make.json); no build config change. Version bump 3.276.1 -> 3.276.2. Bug 2 - AzureCloudPowerShellDeploymentV1 / NewServiceAdditionalArguments Sink: Publish-AzureCloudDeployment.ps1:75-77 raw append to $azureService then Invoke-Expression -Command $azureService. Wire: Add ../Common/Sanitizer to make.json common, Import-Module Sanitizer, and call Invoke-ScriptArgumentSanitization with TaskName 'AzureCloudPowerShellDeploymentV1' and PipelineFeatureFlagName 'EnableAzureCloudPowerShellArgumentsSanitization' right after the input is read and before the new-service block. Version bump 1.276.0 -> 1.276.1. FF registration in mseng/AzureDevOps lands in a companion PR (mirrors #22171's two FFs: declared in all four FeatureAvailability XMLs, defaulted on in the two DevFabric XMLs only). Production rollout via the FF service is staged audit -> enforce -> 100%, then the FF can be retired. The merged #22273 AST backstop (Test-SanitizerArgumentAst) is what closes the newline-bypass and data-constructor injection that a strict-mode char allow-list alone would not catch; the dispatcher therefore calls Protect-ScriptArguments with -AllowDataConstructors. Both wirings inherit that hardening for free. L0 tests added: PowerShellOnTargetMachinesV3/Tests/L0NewPsSessionOptionArgumentsSanitizerWiring.ps1 AzureCloudPowerShellDeploymentV1/Tests/L0NewServiceAdditionalArgumentsSanitizerWiring.ps1 Each is a wiring contract: asserts the dispatcher call is present, validates the right input variable, uses the task's exact TaskName, uses the FF name registered in the companion ADO PR, AND runs before the vulnerable Invoke-Expression so the throw aborts the task before the sink. Comprehensive dispatcher runtime behavior is already covered by Tasks/Common/Sanitizer/Tests/L0Invoke-ScriptArgumentSanitization.ps1. MSRC 115118 / IcM 31000000596029. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bug 2: also sanitize ServiceName / ServiceLocation / NewServiceAffinityGroup Adversarial review (Opus-4.8 max) demonstrated RCE on AzureCloudPowerShellDeploymentV1 even after the initial Bug 2 fix, by feeding the payload through ServiceName instead of NewServiceAdditionalArguments. Three other task inputs flow into the SAME Invoke-Expression -Command $azureService sink in the new-service-creation path: Publish-AzureCloudDeployment.ps1 :84 $azureService = "New-AzureService -ServiceName `"$ServiceName`"" :86 $azureService += " -AffinityGroup `"$NewServiceAffinityGroup`"" :89 $azureService += " -Location `"$ServiceLocation`"" :94 $azureService += " $NewServiceAdditionalArguments" :96 $azureService = Invoke-Expression -Command $azureService Each input is interpolated inside "..." and can break the quoting (literal " or subexpression $(...)) to inject PowerShell at the iex. Fix: validate all four inputs through Invoke-ScriptArgumentSanitization (same gates, same FF name). Empirically verified: the reviewer's payload ServiceName='svc"; $global:PWNED=$true; #' now throws PS_ScriptArgsSanitized with the FF on and the injection does NOT execute. Legitimate values (DNS-style service names, 'West US' locations, affinity-group names) remain ALLOWED (tested against the real Protect-ScriptArguments). Wiring L0 updated to assert all four -InputArgs anchors are present. MSRC 115118 Bug 2 / IcM 31000000596029. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzureCloud: move sanitizer call inside if (!$azureService) branch Second adversarial review (GPT-5.5 xhigh) flagged that the new-service inputs (ServiceName / ServiceLocation / NewServiceAffinityGroup / NewServiceAdditionalArguments) are only used inside the if (!$azureService) new-service-creation branch, but the sanitizer ran at top of try{}. With the FF on, this would falsely reject existing-service deployments (e.g. an unrelated -Description "Prod (EU)" left in the field), since the strict char allow-list rejects parens. Move all four dispatcher calls just inside the if (!$azureService) branch, right before the iex sink. Existing-service deployments now keep their pre-fix behavior; new-service deployments still get full validation. L0 wiring contract extended to assert the dispatcher position is after the if (!$azureService) opening brace (i.e. inside the branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * L0: drop stale reviewer-attribution from test comments Copilot review on PR #22286: the parenthetical (<reviewer>, <date>) notes on the two test-comment blocks will go stale and aren't actionable for future maintainers. Keep the rationale (multi-input, branch-scoped), drop the dates and model names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Uladzimir Tratsiakou (Vladimir/Vova) <utratsiakou@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
6 participants
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.
Context
The PowerShell-runner task family appended the YAML-supplied
ScriptArgumentsinput directly into a generated PowerShell command (& '<scriptPath>' $scriptArguments/. '<scriptPath>' $scriptArguments) without sanitization. A YAML template-parameter splice containing shell metacharacters (;,|,&, ``, etc.) would be parsed by PowerShell as additional statements and executed under the service connection's identity. This is the same class of injection that motivated the original work item #75787 and that has been progressively retrofitted across the repo:The PowerShell-flavored sister tasks that had not been retrofitted were
AzurePowerShellV2-V5andServiceFabricPowerShellV1. This PR closes that gap.Related: ICM
31000000596029, MSRC case115118(resolved as Duplicate; this PR is the proper code-level remediation in the line of #18646 / #18744 / #22066).Task Names
AzurePowerShellV2,AzurePowerShellV3,AzurePowerShellV4,AzurePowerShellV5,ServiceFabricPowerShellV1, plus a new helper file inTasks/Common/Sanitizer/.AzureCloudPowerShellDeploymentV1-deprecated: trueintask.json.AzureCloudPowerShellDeploymentV2-deprecated: trueand preview; noScriptArgumentsconcat sink.PowerShellOnTargetMachinesV2-deprecated: true(PowerShellOnTargetMachinesV3fixed in Fix security issue for PowerShellOnTargetMachines@3 #21968).ChefKnifeV1-deprecated: true, removal date 2024-01-31 intask.json.AzureCLIV1-deprecated: true, explicitly skipped in feat(AzureCLI): Implement security enhancements for scriptArguments validation #22066.ServiceFabricDeployV1- uses its ownSfSafeParserfeature-flagged safe parser instead ofInvoke-Expression.AzureVmssDeploymentV{0,1}/Resources/customScriptInvoker.ps1- by design runs customer-supplied script on the target VM (the customer is the script author, not a third-party injecting into a YAML splice).AzurePowerShellV1andServiceFabricPowerShellV2do not exist in any branch of the repo.Description
Two-flag rollout, mirroring PR #21947's
Should-UseSanitizedArgumentspattern (and #22066'sEnableAzureCliArgsValidationpattern). Sanitization fires only when BOTH:AZP_75787_*env triplet on the agent. Surfaced viaGet-SanitizerCallStatusin the existingTasks/Common/Sanitizermodule.Get-VstsPipelineFeature:EnableAzurePowerShellArgumentsSanitization- shared acrossAzurePowerShellV2-V5.EnableServiceFabricPowerShellArgumentsSanitization-ServiceFabricPowerShellV1.This means a customer who has the org toggle on today (e.g. because they enabled it for the PowerShellV2 fix) will not see any behavior change on these newly-onboarded tasks until Microsoft turns on the per-task pipeline flag, preventing accidental regression. The same rollout model was used by #21947 and #22066.
New shared helper -
Tasks/Common/Sanitizer/Invoke-ScriptArgumentSanitization.ps1(sourced fromSanitizer.psm1and exported). Three functions:Test-ShouldUseSanitizer -TaskName <name> -PipelineFeatureFlagName <name>- combined check, emits FF-check failure telemetry like PR Prevent security exploit in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 where Invoke-Expression is used #21947.Invoke-ScriptArgumentSanitization -InputArgs <args> -TaskName <name> -PipelineFeatureFlagName <name>- single-call wrapper aroundProtect-ScriptArguments+ try/catch.Publish-SanitizerErrorTelemetry- small##vso[telemetry.publish]emitter shared by both.Fail-closed dispatch (deliberate divergence from PowerShellV2's PS port):
Invoke-ScriptArgumentSanitizationtreats any exception out ofProtect-ScriptArgumentsas a sanitization failure and aborts the task with the task-localizedScriptArgsSanitizedmessage. We deliberately do not compare.Exception.MessageagainstGet-VstsLocString -Key 'ScriptArgsSanitized'to distinguish "rejection" from "unexpected error":Get-VstsLocString -Key 'PS_ScriptArgsSanitized'(its own resjson key) while the task resjson uses keyScriptArgsSanitized. The two strings are translated independently.fr-FRalready diverges today:cochevsbacktick. An-eqcomparison on a French agent would let a real sanitizer rejection fall into the "swallow into telemetry" branch and bypass the gate.This is a divergence from the inline
catchblocks in PowerShellV2 / SqlAzureDacpacDeploymentV1 / AzureCLIV2 (which compare toGet-VstsLocString); those task-specific implementations carry the same latent locale-drift bug but ship in TypeScript / inline PS where it has been masked by en-US-identical strings. The new shared helper fixes it once.Per-task collapse - each task entry script went from a ~25-line inline try/catch + local
Publish-Telemetryhelper to:powershell if ($scriptType -ne "InlineScript") { Invoke-ScriptArgumentSanitization-InputArgs $scriptArguments
-TaskName 'AzurePowerShellV5'-PipelineFeatureFlagName 'EnableAzurePowerShellArgumentsSanitization'
}
`
Per task, the change set is still the proven
PowerShellV2-style retrofit:make.jsonadds../Common/Sanitizerto thecommonarray.The entry script imports the module and calls the shared helper right after the existing
[\r\n]newline check, only when-ne "InlineScript".task.json/task.loc.jsonpatch-bumped (post-rebase against current master):Patch-number gaps (V4: +4, V5: +2 from current master Default) are intentional - they reserve headroom for currently open in-flight PRs (Update package dependencies and fix security vulnerabilities #22058
Update package dependencies and fix security vulnerabilitiesis still based on older V4/V5 patches and is expected to land first).task.loc.json.messagesadds"ScriptArgsSanitized": "ms-resource:loc.messages.ScriptArgsSanitized".All ten locale
Strings/resources.resjson/<locale>/resources.resjsonfiles add the newloc.messages.ScriptArgsSanitizedentry, copying the already-translated text fromPowerShellV2._generated/AzurePowerShellV{4,5}{,_Node24}mirrors the source changes and_buildConfigMapping/*.versionmap.txtare kept consistent (Default = master patch, Node24_1 = Default+1).Common/Sanitizer dependents bumped to satisfy
ci/filter-tasks.jsAdding
Tasks/Common/Sanitizer/Invoke-ScriptArgumentSanitization.ps1(and modifyingSanitizer.psm1) trips the repo's policy that every task with../Common/Sanitizerin itsmake.jsonmust have its own version bumped in the same PR. Same precedent as PR #19834 (Common/Sanitizer audit-event change, May 2024). These 12 tasks gain no functional change; onlytask.json/task.loc.json/_generated/mirrors are touched.Bumping rule: smallest legal patch increment that satisfies BuildConfigGen's
source > max(versionmap)invariant. Tasks without a Node24 build config get +1 (Default+1). Tasks with a Node24_1 config get +2 so that Node24_1 stays the conventional Default+1. Four tasks (PowerShellOnTargetMachinesV3,ServiceFabricPowerShellV1,WindowsMachineFileCopyV1,WindowsMachineFileCopyV2) had Minor versions older than the current sprint, so they were rolled forward to*.274.0to satisfyci/check-downgrading.js(minor >= currentSprintVersion).AzureFileCopyV1AzureFileCopyV2AzureFileCopyV3AzureFileCopyV4AzureFileCopyV5AzureFileCopyV6PowerShellV2PowerShellOnTargetMachinesV3SqlAzureDacpacDeploymentV1SqlDacpacDeploymentOnMachineGroupV0WindowsMachineFileCopyV1WindowsMachineFileCopyV2_generated/AzureFileCopyV4/and_generated/AzureFileCopyV5/Default mirrors are re-materialized by BuildConfigGen as a consequence of the bump (PR #20819 had previously removed them when consolidating Node20 configs). The mirror content is byte-identical to source except fortask.json/task.loc.jsonversion fields - same shape as the existing_generated/AzureFileCopyV6/Default mirror that has been checked in for the same reason.InlineScriptandScriptPathare intentionally not sanitized: inline scripts are author-written; the path is not shell-spliced (it goes through''quoting).Risk Assessment - Low
Test-ShouldUseSanitizerfalls open (returnsFalse) on cmdlet-missing or FF-check error and emits diagnostic telemetry - never a hard failure on the FF query path itself.Tasks/Common/Sanitizermodule (no new sanitization code).Sanitizer.psm1and running five scenarios:ScriptArgsSanitizedmessage.Get-VstsPipelineFeaturemissing -> no-op, emitscmdletMissingtelemetry.Documentation changes required: Y
The Learn topic at https://learn.microsoft.com/azure/devops/pipelines/security/inputs#enable-shell-tasks-arguments-validation has not been updated since the 2023 retrofit and is missing the four subsequent retrofits (#21947, #21968, #22066, this PR). Companion docs PR: MicrosoftDocs/azure-devops-docs#14371.
Added unit tests: Y
Tasks/Common/Sanitizer/Tests/L0Invoke-ScriptArgumentSanitization.ps1covers the new dispatcher across six gating branches, fail-closed behaviour, and locale-drift regression. Registered inTasks/Common/Sanitizer/Tests/L0.ts. The existingTasks/Common/Sanitizer/Tests/L0*suite continues to cover the underlying regex/throw/audit/telemetry FF behaviour and bash/cmd/PowerShell format-compatibility.Attached related issue: ICM 31000000596029, MSRC 115118, AB#75787
Checklist
Register-Mock Import-ModuleinTests/lib/Initialize-Test.ps1neutralizes the new import; the helper falls open through the FF gate whenGet-VstsPipelineFeatureis unavailable)._generated/regenerated for the two tasks that have generated mirrors (V4, V5)._generated/and versionmaps re-validated.EnableAzurePowerShellArgumentsSanitizationandEnableServiceFabricPowerShellArgumentsSanitizationinTools.PipelineFeatures(mseng/AzureDevOps) with defaultoff. Without this, the PR is a permanent no-op.Tasks/Common/Sanitizerowners requested before merge.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com