Skip to content

Prevent security exploit in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 where Invoke-Expression is used#21947

Open
rm-dmiri wants to merge 25 commits intomasterfrom
users/rm-dmiri/fix-sqldacpac-injection
Open

Prevent security exploit in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 where Invoke-Expression is used#21947
rm-dmiri wants to merge 25 commits intomasterfrom
users/rm-dmiri/fix-sqldacpac-injection

Conversation

@rm-dmiri
Copy link
Copy Markdown
Contributor

@rm-dmiri rm-dmiri commented Mar 27, 2026 •
edited
Loading

Context

This PR addresses a critical security vulnerability in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 tasks that allows arbitrary PowerShell command injection through the SqlAdditionalArguments parameter.

Task Name

SqlAzureDacpacDeploymentV1, SqlDacpacDeploymentOnMachineGroupV0

Description



1. Safe Argument Execution

  • Replaced: Invoke-Expression with string concatenation
  • With: PowerShell AST Parser tokenization → array-based argument passing
  • Arguments are parsed, validated for syntax errors, and passed safely to sqlpackage.exe

2. Dual Feature Flag Gating

Sanitization activates only when both flags are enabled:

  • Org-level: AZP_75787 environment variable
  • Pipeline-level: EnableSqlAdditionalArgumentsSanitization pipeline variable

This allows gradual rollout and testing without breaking existing pipelines.

3. Sanitization Layer

  • Integrates existing Sanitizer common module
  • Calls Protect-ScriptArguments to validate and sanitize SQL arguments
  • Fail-closed: Task fails if sanitization errors occur (when flags enabled)
  • Fail-open: Task proceeds if feature flags unavailable (backward compatibility)

4. Telemetry

Added telemetry tracking for:

  • Feature flag availability and status
  • Sanitization activation and results
  • Blocked inputs and error conditions

5. Backward Compatibility

  • Old behavior preserved: When feature flags disabled, tasks work exactly as before
  • Graceful degradation: Handles older agents without Get-VstsPipelineFeature cmdlet
  • Clear messaging: Verbose logging shows sanitization status

Changes

SqlAzureDacpacDeploymentV1 (v1.273.6):

  • Updated Execute-Command to use AST Parser instead of Invoke-Expression
  • Added Should-UseSanitizedArguments with dual feature flag checks
  • Added Get-SanitizedSqlArguments with fail-closed validation
  • Added telemetry helper function

SqlDacpacDeploymentOnMachineGroupV0 (v0.273.4):

  • Same security improvements as V1

Risk Assessment (Low / Medium / High)

Medium. Needs rollout monitoring not to break this task and introduce regression. Such monitors are already in place. But we should monitor telemetry emitted from this task too (there is newly added telemetry).

Change Behind Feature Flag (Yes / No)

Sanitization activates only when BOTH flags are enabled:

  • Org-level: Enable shell tasks arguments validation in organisation settings
  • Pipeline-level: EnableSqlAdditionalArgumentsSanitization (dynamic FF), can be used as DISTRIBUTED_TASK_EnableSqlAdditionalArgumentsSanitization evnironment variable

This allows gradual rollout and testing before full enforcement.

Tech Design / Approach

/

Documentation Changes Required (Yes/No)

Probably.

Unit Tests Added or Updated (Yes / No)

Added some tests.

Additional Testing Performed

Manual testing.

Logging Added/Updated (Yes/No)

No.

Telemetry Added/Updated (Yes/No)

No.

Rollback Scenario and Process (Yes/No)

  • Disable dynamic FF (EnableSqlAdditionalArgumentsSanitization).
  • Or downgrade task version. Or disable org level pipeline setting -> "Enable shell tasks arguments validation"

Dependency Impact Assessed and Regression Tested (Yes/No)

No.

Checklist

  • Related issue linked (if applicable)
  • Task version was bumped — see versioning guide
  • Verified the task behaves as expected

@rm-dmiri rm-dmiri requested review from a team and manolerazvan as code owners March 27, 2026 12:25
@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Mar 27, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 27, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Mar 27, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 27, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Mar 27, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 27, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Mar 28, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 28, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Mar 28, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Mar 28, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 1, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 1, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 2, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 2, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri rm-dmiri changed the title Users/rm dmiri/fix sqldacpac injection Prevent security exploit in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 where Invoke-Expression is used Apr 2, 2026
ivanduplenskikh
ivanduplenskikh reviewed Apr 2, 2026
View reviewed changes
ivanduplenskikh
ivanduplenskikh reviewed Apr 2, 2026
View reviewed changes
ivanduplenskikh
ivanduplenskikh reviewed Apr 2, 2026
View reviewed changes
@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 2, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 6, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Bump SqlAzureDacpacDeploymentV1 to v1.273.0 for sprint 273
7e06ec1 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 6, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 6, 2026

Azure Pipelines successfully started running 3 pipeline(s).

@rm-dmiri
Bump SqlDacpacDeploymentOnMachineGroupV0 to v0.273.0 for sprint 273
98afd69 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 6, 2026

/azp run

@rm-dmiri rm-dmiri enabled auto-merge (squash) April 6, 2026 19:40
@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 6, 2026

Azure Pipelines successfully started running 3 pipeline(s).

manolerazvan
manolerazvan reviewed Apr 7, 2026
View reviewed changes
@rm-dmiri
Replace splatting with Sanitizer module for injection prevention
e49d076 
Revert splatting/ConvertTo-SqlCmdParameterHashtable approach and instead
apply Protect-ScriptArguments from Common/Sanitizer module, matching the
pattern used by AzureFileCopyV5/V6 and other tasks.

- Import Sanitizer module in DeploySqlAzure.ps1 and Main.ps1
- Sanitize sqlcmdAdditionalArguments, sqlcmdInlineAdditionalArguments,
  and sqlpackageAdditionalArguments when AZP_75787 FF is active
- Add Sanitizer to make.json for both tasks
- Bump versions to 1.273.0 / 0.273.0

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 8, 2026

Azure Pipelines:
3 pipeline(s) require an authorized user to comment /azp run to run.

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 8, 2026

Azure Pipelines:
3 pipeline(s) require an authorized user to comment /azp run to run.

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 9, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

1 similar comment
@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

@rm-dmiri
Copy link
Copy Markdown
Contributor Author

rm-dmiri commented Apr 9, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 9, 2026

Azure Pipelines:
Successfully started running 3 pipeline(s).

nemanjarogic
nemanjarogic reviewed Apr 9, 2026
View reviewed changes
Tasks/SqlAzureDacpacDeploymentV1/DeploySqlAzure.ps1

if ($deploymentAction -eq "DriftReport" -and $LASTEXITCODE -eq 1) {
$errorMessage += Get-VstsLocString -Key "SAD_DriftReportWarning"
$errorMessage += " " + (Get-VstsLocString -Key "SAD_DriftReportWarning")
Copy link
Copy Markdown
Contributor

@nemanjarogic nemanjarogic Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need space at the beginning here?

Copy link
Copy Markdown
Contributor Author

@rm-dmiri rm-dmiri Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is a link ending in the error message to which this part is appended to. And the URL is broken then.
https://aka.ms/ado/75787
vs
https://aka.ms/ado/75787Check

I noticed it and decided to fix it.

It goes like this (bolded part is where separation is added):

##[error]SECURITY ERROR: Failed to sanitize SQL arguments. Task cannot proceed safely. Error: Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using backtick (`). More information is available here: https://aka.ms/ado/75787 Check out how to troubleshoot failures at https://aka.ms/sqlazuredeployreadme#troubleshooting-

Tasks/SqlAzureDacpacDeploymentV1/Utility.ps1

function Should-UseSanitizedArguments {
try {
$orgLevelEnabled = Get-SanitizerCallStatus
Copy link
Copy Markdown
Contributor

@nemanjarogic nemanjarogic Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow vulnerability if toggle is disabled in organization settings? @manolerazvan you proposed this initially, probably due to option to rollback changes if something goes wrong (or maybe not), but after some thinking not sure whether it's smart to tie security fix with option that allows users to disable it on their own.

In other words, I think it would be best to avoid any disablement by users.

The same question goes for my PR (other security fix), because I used the same approach.

Copy link
Copy Markdown
Contributor Author

@rm-dmiri rm-dmiri Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to disable this, you need to be organization admin.
And we try to protect pipelines from user that don't even have permission to edit pipelines, but just to run them and modify their environment variables.
I think this is why we allow this to be toggled on org level.
However, I am happy to hear insights from Razvan on this topic, as I don't have prior experience with such issues.

Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1
Describe "Execute-Command Security Tests" {
Context "AST Parser Validation" {
It "Should use token .Value instead of .Text" {
# This test verifies the fix for Ivan's concern about quote handling
Copy link
Copy Markdown
Contributor

@nemanjarogic nemanjarogic Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ivan's concern won't be clear once PR is merged. Lets be clear about use case

Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1
@@ -0,0 +1,129 @@
# Unit tests for SQL argument sanitization security functions
[CmdletBinding()]
Copy link
Copy Markdown
Contributor

@nemanjarogic nemanjarogic Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to mock the feature flags and invoke Get-SanitizedSqlArguments with malicious input to verify such inputs are blocked?

Tasks/SqlDacpacDeploymentOnMachineGroupV0/Utility.ps1
$additionalArguments = EscapeSpecialChars $additionalArguments

#Execute the query
Invoke-Expression "Invoke-SqlCmd @spaltArguments $additionalArguments"
Copy link
Copy Markdown
Contributor

@nemanjarogic nemanjarogic Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Invoke-SqlScriptsInTransaction function still executes:

Invoke-Expression "Invoke-SqlCmd @spaltArguments $additionalArguments"

I understand that $additionalArguments is sanitized upstream before it reaches this function, so today all callers pass clean input.

However, this is a public function that could be called from other entry points in the future without the sanitization gate. Compare with what was done for Execute-Command in V1 task — there, the Invoke-Expression was replaced entirely with AST-parsed array-based invocation (& $FileName $argArray), eliminating the dangerous pattern at the sink itself regardless of whether the caller sanitized.

The same treatment would be straightforward here since the function already uses splatting (@spaltArguments) for most parameters — the $additionalArguments string just needs to be parsed and merged into the splat hashtable (or passed as an array) instead of being concatenated into an Invoke-Expression string.

Not a blocker for this PR since all current callers pass sanitized input, but worth considering as a follow-up hardening pass to fully eliminate the Invoke-Expression pattern from both tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nemanjarogic nemanjarogic nemanjarogic left review comments

@manolerazvan manolerazvan manolerazvan left review comments

@ivanduplenskikh ivanduplenskikh ivanduplenskikh left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rm-dmiri @nemanjarogic @manolerazvan @ivanduplenskikh