Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update powershell sanitizer regex #19028

Merged
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV1/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 1,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV1/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 1,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV2/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 2,
"Minor": 228,
"Patch": 3
"Minor": 229,
"Patch": 1
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV2/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 2,
"Minor": 228,
"Patch": 3
"Minor": 229,
"Patch": 1
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV3/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 3,
"Minor": 228,
"Patch": 3
"Minor": 229,
"Patch": 1
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV3/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 3,
"Minor": 228,
"Patch": 3
"Minor": 229,
"Patch": 1
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV4/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 4,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV4/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 4,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV5/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 5,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"demands": [
"azureps"
Expand Down
4 changes: 2 additions & 2 deletions Tasks/AzureFileCopyV5/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 5,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"demands": [
"azureps"
Expand Down
4 changes: 1 addition & 3 deletions Tasks/Common/Sanitizer/ArgumentsSanitizer.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function Get-SanitizedArguments([string]$inputArgs) {

# regex rule for removing symbols and telemetry.
# '?<!`' - checking if before character no backtick. '([allowedchars])' - checking if character is allowed. Otherwise, replace to $removedSymbolSign
$regex = '(?<!\\)([^a-zA-Z0-9\\ _''"\-=/:.])';
$regex = '(?<!`)([^a-zA-Z0-9\\` _''"\-=\/:\.*,+~?%\n])';

# We're splitting by ``, removing all suspicious characters and then join
$argsArr = $inputArgs -split $argsSplitSymbols;
Expand All @@ -71,8 +71,6 @@ function Get-SanitizedArguments([string]$inputArgs) {
$matchesChunks += , $matches;
$argsArr[$i] = $argsArr[$i] -replace $regex, $removedSymbolSign;
}

$argsArr[$i] = $argsArr[$i] -replace $regex, $removedSymbolSign;
}

$resultArgs = $argsArr -join $argsSplitSymbols;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"loc.messages.PS_ScriptArgsSanitized": "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>",
"loc.messages.PS_ScriptArgsSanitized": "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",
"loc.messages.PS_ScriptArgsNotSanitized": "Arguments passed sanitization without change."
}
19 changes: 14 additions & 5 deletions Tasks/Common/Sanitizer/Tests/L0.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
/// <reference path="../../../../definitions/mocha.d.ts"/>
/// <reference path="../../../../definitions/node.d.ts"/>
/// <reference path="../../../../definitions/Q.d.ts"/>

import Q = require('q');
import assert = require('assert');
import path = require('path');

var psm = require('../../../Tests/lib/psRunner');
var psm = require('../../../../Tests/lib/psRunner');
var psr = null;

describe('Security Suite', function () {
Expand Down Expand Up @@ -49,4 +46,16 @@ describe('Security Suite', function () {
psr.run(path.join(__dirname, 'L0Get-SanitizedArgumentsArray.DoesNotBreakExistingBashFormats.ps1'), done);
});
}
});

if (psm.testSupported()) {
it('Protect-ScriptArguments should pass correctly', (done) => {
psr.run(path.join(__dirname, 'L0Protect-ScriptArguments.Passes.ps1'), done);
});
}

if (psm.testSupported()) {
it('Protect-ScriptArguments should throw', (done) => {
psr.run(path.join(__dirname, 'L0Protect-ScriptArguments.Throws.ps1'), done);
});
}
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ foreach ($argument in $bashArgumentsFormats) {
$sanitizedArguments = Get-SanitizedArguments -InputArgs $argument

# Assert
Assert-AreEqual $sanitizedArguments $argument
}
Assert-AreEqual -Actual $sanitizedArguments -Expected $argument
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ foreach ($argument in $cmdArgumentsFormats) {
$sanitizedArguments = Get-SanitizedArguments -InputArgs $argument

# Assert
Assert-AreEqual $sanitizedArguments $argument
Assert-AreEqual -Actual $sanitizedArguments -Expected $argument
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ foreach ($argument in $powershellArgumentsFormats) {
$sanitizedArguments = Get-SanitizedArguments -InputArgs $argument

# Assert
Assert-AreEqual $sanitizedArguments $argument
Assert-AreEqual -Actual $sanitizedArguments -Expected $argument
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ $sanitizedArguments = Get-SanitizedArguments -InputArgs $arguments

# Assert

# We need to use $sanitizedArguments[1] because $sanitizedArguments contains buffer with Write-Output message from the function execution.
Assert-AreEqual $sanitizedArguments[1] "start notepad.exe _#removed#_ echo 'hello' _#removed#_ calc.exe"
# We need to use $sanitizedArguments[1] because $sanitizedArguments contains buffer with Write-Output message from the function execution.
Assert-AreEqual -Expected "start notepad.exe _#removed#_ echo 'hello' _#removed#_ calc.exe" -Actual $sanitizedArguments
41 changes: 41 additions & 0 deletions Tasks/Common/Sanitizer/Tests/L0Protect-ScriptArguments.Passes.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
[CmdletBinding()]
param()

Set-Item -Path env:AZP_75787_ENABLE_NEW_LOGIC -Value 'true'

. $PSScriptRoot\..\..\..\..\Tests\lib\Initialize-Test.ps1
. $PSScriptRoot\..\ArgumentsSanitizer.ps1

$inputArgsSuites = @(
"/parameter", # Traditional way to pass parameters in CMD
"-parameter", # Modern applications accept parameters with a hyphen
"--parameter", # Many modern applications accept parameters with double hyphen
"parameter=value", # Format for passing values to parameters
"parameter value.txt", # Argument with dot in the middle
"-parameter", # Single hyphen followed by a single letter or digit (POSIX style)
"-parameter value", # When the parameter needs a value
"--parameter", # Double hyphen followed by a word (GNU style)
"--parameter=value", # Value directly attached to the parameter with an equals sign
"parameter=value", # Used to pass environment variables to a command
"parameter value.txt", # Argument with dot in the middle
"-Parameter Value", # Most common form
"-Parameter:Value", # Colon connects the parameter and its value
"/p:Parameter=Value", # Specific syntax for tools like MSBuild or NuGet
"--Parameter Value", # Used by cmdlets or scripts for cross-platform compatibility
"--Parameter=Value", # Used by cross-platform tools
"parameter value.txt" # Argument with dot in the middle
'a A 1 \ ` _ '' " - = / : . * , + ~ ? %', # Just each allowed symbol
'',
'test 1',
'test `; whoami `&`& echo test',
"line 1 `n line 2"
)

foreach ($inputArgs in $inputArgsSuites) {
try {
Protect-ScriptArguments $inputArgs
}
catch {
throw "Error occured with '$inputArgs' input args: $($_.Exception.Message)"
}
}
22 changes: 22 additions & 0 deletions Tasks/Common/Sanitizer/Tests/L0Protect-ScriptArguments.Throws.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
[CmdletBinding()]
param()

Set-Item -Path env:AZP_75787_ENABLE_NEW_LOGIC -Value 'true'

. $PSScriptRoot\..\..\..\..\Tests\lib\Initialize-Test.ps1
. $PSScriptRoot\..\ArgumentsSanitizer.ps1

$inputArgsSuites = @(
'test; whoami',
'test && whoami',
'echo "$(rm ./somedir)"',
'test | whoami'
)

$expectedMsg = Get-VstsLocString -Key 'PS_ScriptArgsSanitized'

foreach ($inputArgs in $inputArgsSuites) {
Assert-Throws {
Protect-ScriptArguments $inputArgs
} -MessagePattern $expectedMsg
}
4 changes: 2 additions & 2 deletions Tasks/Common/Sanitizer/Tests/L0Split-Arguments.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@ for ($i = 0; $i -lt $argumentsFormats.Length; $i++) {
[string[]]$splitArguments = Split-Arguments -arguments $argumentsFormats[$i]

# Assert
Assert-AreEqual $splitArguments $expectedOutputs[$i]
}
Assert-AreEqual -Expected $splitArguments -Actual $expectedOutputs[$i]
}
2 changes: 1 addition & 1 deletion Tasks/Common/Sanitizer/module.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"messages": {
"PS_ScriptArgsSanitized": "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>",
"PS_ScriptArgsSanitized": "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",
"PS_ScriptArgsNotSanitized": "Arguments passed sanitization without change."
}
}
12 changes: 12 additions & 0 deletions Tasks/Common/Sanitizer/runTests.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
$startLoc = Get-Location

Set-Location $PSScriptRoot/../../..

try {
Remove-Item .\_build\Tasks\Common\Sanitizer -Recurse -Force -ErrorAction SilentlyContinue
node make build --task AzureFileCopyV5
KonstantinTyukalov marked this conversation as resolved.
Show resolved Hide resolved
node make test --task Common/Sanitizer
}
finally {
Set-Location $startLoc
}
4 changes: 2 additions & 2 deletions Tasks/WindowsMachineFileCopyV1/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 1,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"minimumAgentVersion": "1.104.0",
"groups": [
Expand Down
4 changes: 2 additions & 2 deletions Tasks/WindowsMachineFileCopyV1/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 1,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"minimumAgentVersion": "1.104.0",
"groups": [
Expand Down
4 changes: 2 additions & 2 deletions Tasks/WindowsMachineFileCopyV2/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 2,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"releaseNotes": "What's new in Version 2.0: <br/>&nbsp;&nbsp;Proxy support is being added. <br/>&nbsp;&nbsp; Removed support of legacy DTL machines.",
"minimumAgentVersion": "1.104.0",
Expand Down
4 changes: 2 additions & 2 deletions Tasks/WindowsMachineFileCopyV2/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 2,
"Minor": 228,
"Patch": 1
"Minor": 229,
"Patch": 0
},
"releaseNotes": "ms-resource:loc.releaseNotes",
"minimumAgentVersion": "1.104.0",
Expand Down
4 changes: 2 additions & 2 deletions _generated/AzureFileCopyV2.versionmap.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Default|2.228.3
Node16-225|2.228.2
Default|2.229.1
Node16-225|2.229.0
8 changes: 4 additions & 4 deletions _generated/AzureFileCopyV2/task.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 2,
"Minor": 228,
"Patch": 3
"Minor": 229,
"Patch": 1
},
"demands": [
"azureps"
Expand Down Expand Up @@ -370,7 +370,7 @@
"ExpiredServicePrincipal": "Could not fetch access token for Azure. Verify if the Service Principal used is valid and not expired."
},
"_buildConfigMapping": {
"Default": "2.228.3",
"Node16-225": "2.228.2"
"Default": "2.229.1",
"Node16-225": "2.229.0"
}
}
8 changes: 4 additions & 4 deletions _generated/AzureFileCopyV2/task.loc.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
"author": "Microsoft Corporation",
"version": {
"Major": 2,
"Minor": 228,
"Patch": 3
"Minor": 229,
"Patch": 1
},
"demands": [
"azureps"
Expand Down Expand Up @@ -370,7 +370,7 @@
"ExpiredServicePrincipal": "ms-resource:loc.messages.ExpiredServicePrincipal"
},
"_buildConfigMapping": {
"Default": "2.228.3",
"Node16-225": "2.228.2"
"Default": "2.229.1",
"Node16-225": "2.229.0"
}
}
Loading