From e43eaa68201c5c8bf90cb7cdb480dcb28199d3be Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Thu, 7 Sep 2023 03:18:25 +0400 Subject: [PATCH 01/25] Update env expand logic --- .../Handlers/Helpers/ProcessHandlerHelper.cs | 14 ++++++++++++-- .../L0/Worker/Handlers/ProcessHandlerHelperL0.cs | 15 ++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 8f927f68a5..7d0eac652e 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -93,7 +93,15 @@ public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) telemetry.VariablesWithESInside++; } - var envValue = System.Environment.GetEnvironmentVariable(envName) ?? ""; + var envValue = Environment.GetEnvironmentVariable(envName); + // In case we don't have such variable, we just leave it as is + if (string.IsNullOrEmpty(envValue)) + { + telemetry.NotExistingEnv++; + startIndex = envEndIndex; + continue; + } + var tail = result[(envEndIndex + envPostfix.Length)..]; result = head + envValue + tail; @@ -134,6 +142,7 @@ public class CmdTelemetry public int VariablesWithESInside { get; set; } = 0; public int QuotesNotEnclosed { get; set; } = 0; public int NotClosedEnvSyntaxPosition { get; set; } = 0; + public int NotExistingEnv { get; set; } = 0; public Dictionary ToDictionary() { @@ -149,7 +158,8 @@ public Dictionary ToDictionary() ["bracedVariables"] = BracedVariables, ["bariablesWithESInside"] = VariablesWithESInside, ["quotesNotEnclosed"] = QuotesNotEnclosed, - ["notClosedBraceSyntaxPosition"] = NotClosedEnvSyntaxPosition + ["notClosedBraceSyntaxPosition"] = NotClosedEnvSyntaxPosition, + ["notExistingEnv"] = NotExistingEnv }; } diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index b0e746d36a..9b8b4e22e0 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -68,7 +68,20 @@ public void NestedVariablesNotExpands() Environment.SetEnvironmentVariable("VAR2", "2"); Environment.SetEnvironmentVariable("NESTED", "nested"); - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandArguments(argsLine); + + Assert.Equal(expectedArgs, actualArgs); + } + + [Fact] + public void SkipsInvalidEnv() + { + string argsLine = "%VAR1% 2"; + Environment.SetEnvironmentVariable("VAR1", null); + + string expectedArgs = "%VAR1% 2"; + + var (actualArgs, _) = ProcessHandlerHelper.ExpandArguments(argsLine); Assert.Equal(expectedArgs, actualArgs); } From 170ed65f853bed8eeef46791e2b50dbc37ac0876 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 8 Sep 2023 18:08:28 +0400 Subject: [PATCH 02/25] Upd process handler args validation --- .../Handlers/Helpers/CmdArgsSanitizer.cs | 94 +++++++++++++ .../Handlers/Helpers/ProcessHandlerHelper.cs | 20 +-- src/Agent.Worker/Handlers/ProcessHandler.cs | 124 +++++++++--------- src/Misc/layoutbin/en-US/strings.json | 1 + .../L0/Worker/Handlers/CmdArgsSanitizerL0.cs | 77 +++++++++++ .../Worker/Handlers/ProcessHandlerHelperL0.cs | 18 ++- .../ProcessHandlerHelperTelemetryL0.cs | 17 +-- 7 files changed, 255 insertions(+), 96 deletions(-) create mode 100644 src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs create mode 100644 src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs new file mode 100644 index 0000000000..cf2b8389e8 --- /dev/null +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -0,0 +1,94 @@ +using Microsoft.VisualStudio.Services.Agent.Util; +using System.Collections.Generic; +using System.Linq; +using System.Text.RegularExpressions; + +namespace Agent.Worker.Handlers.Helpers +{ + public static class CmdArgsSanitizer + { + public static (string, CmdArgsSanitizingTelemetry) SanitizeArguments(string inputArgs) + { + if (inputArgs == null) + { + return (null, null); + } + + const string removedSymbolSign = "_#removed#_"; + const string argsSplitSymbols = "^^"; + + var argsChunks = inputArgs.Split(argsSplitSymbols); + var matchesChunks = new List(); + + var saniziteRegExp = new Regex("(? 0) + { + matchesChunks.Add(matches); + argsChunks[i] = saniziteRegExp.Replace(argsChunks[i], removedSymbolSign); + } + } + + var resultArgs = string.Join(argsSplitSymbols, argsChunks); + + CmdArgsSanitizingTelemetry telemetry = null; + + if (resultArgs != inputArgs) + { + var symbolsCount = matchesChunks + .Select(chunk => chunk.Count) + .Aggregate(0, (acc, mc) => acc + mc); + telemetry = new CmdArgsSanitizingTelemetry + ( + RemovedSymbols: CmdArgsSanitizingTelemetry.ToSymbolsDictionary(matchesChunks), + RemovedSymbolsCount: symbolsCount + ); + } + + return (resultArgs, telemetry); + } + } + + public record CmdArgsSanitizingTelemetry + ( + Dictionary RemovedSymbols, + int RemovedSymbolsCount + ) + { + public static Dictionary ToSymbolsDictionary(List matches) + { + ArgUtil.NotNull(matches, nameof(matches)); + + var symbolsDict = new Dictionary(); + foreach (var mc in matches) + { + foreach (var m in mc.Cast()) + { + var symbol = m.Value; + if (symbolsDict.TryGetValue(symbol, out _)) + { + symbolsDict[symbol] += 1; + } + else + { + symbolsDict[symbol] = 1; + } + } + } + + return symbolsDict; + } + + public Dictionary ToDictionary() + { + return new() + { + ["removedSymbols"] = RemovedSymbols, + ["removedSymbolsCount"] = RemovedSymbolsCount, + }; + } + } +} diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 7d0eac652e..ae6d125e41 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -1,12 +1,11 @@ using System; using System.Collections.Generic; -using System.Text; namespace Agent.Worker.Handlers.Helpers { public static class ProcessHandlerHelper { - public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) + public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs) { const char quote = '"'; const char escapingSymbol = '^'; @@ -85,7 +84,7 @@ public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) } var head = result[..prefixIndex]; - if (envName.Contains(escapingSymbol)) + if (envName.Contains(escapingSymbol, StringComparison.Ordinal)) { head += envName.Split(escapingSymbol)[1]; envName = envName.Split(escapingSymbol)[0]; @@ -144,9 +143,9 @@ public class CmdTelemetry public int NotClosedEnvSyntaxPosition { get; set; } = 0; public int NotExistingEnv { get; set; } = 0; - public Dictionary ToDictionary() + public Dictionary ToDictionary() { - return new Dictionary + return new Dictionary { ["foundPrefixes"] = FoundPrefixes, ["quottedBlocks"] = QuottedBlocks, @@ -162,16 +161,5 @@ public Dictionary ToDictionary() ["notExistingEnv"] = NotExistingEnv }; } - - public Dictionary ToStringsDictionary() - { - var dict = ToDictionary(); - var result = new Dictionary(); - foreach (var key in dict.Keys) - { - result.Add(key, dict[key].ToString()); - } - return result; - } }; } diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 2dda8ef527..f84276d598 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -4,8 +4,9 @@ using Agent.Sdk.Knob; using Agent.Worker.Handlers.Helpers; using Microsoft.VisualStudio.Services.Agent.Util; -using Newtonsoft.Json; +using Microsoft.VisualStudio.Services.Common; using System; +using System.Collections.Generic; using System.IO; using System.Text; using System.Threading.Tasks; @@ -26,7 +27,6 @@ public sealed class ProcessHandler : Handler, IProcessHandler private volatile int _errorCount; private bool _foundDelimiter; private bool _modifyEnvironment; - private string _generatedScriptPath; public ProcessHandlerData Data { get; set; } @@ -132,35 +132,31 @@ public async Task RunAsync() cmdExe = "cmd.exe"; } - var enableSecureArguments = AgentKnobs.ProcessHandlerSecureArguments.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable secure arguments: '{enableSecureArguments}'"); - var enableSecureArgumentsAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable secure arguments audit: '{enableSecureArgumentsAudit}'"); - var enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable telemetry: '{enableTelemetry}'"); - - var enableFileArgs = disableInlineExecution && enableSecureArguments; - - if ((disableInlineExecution && (enableSecureArgumentsAudit || enableSecureArguments)) || enableTelemetry) + try + { + throw new Exception("Test exc"); + ValidateInputArguments(arguments); + } + catch (ArgsSanitizedException) { - var (processedArgs, telemetry) = ProcessHandlerHelper.ProcessInputArguments(arguments); + throw; + } + catch (Exception ex) + { + Trace.Error($"Failed to validate process handler input arguments. Publishing telemetry. Ex: {ex}"); - if (disableInlineExecution && enableSecureArgumentsAudit) + var telemetry = new Dictionary() { - ExecutionContext.Warning($"The following arguments will be executed: '{processedArgs}'"); - } - if (enableFileArgs) - { - GenerateScriptFile(cmdExe, command, processedArgs); - } - if (enableTelemetry) - { - ExecutionContext.Debug($"Agent PH telemetry: {JsonConvert.SerializeObject(telemetry.ToDictionary(), Formatting.None)}"); - PublishTelemetry(telemetry.ToDictionary(), "ProcessHandler"); - } + ["UnexpectedError"] = ex.Message, + ["ErrorStackTrace"] = ex.StackTrace + }; + PublishTelemetry(telemetry, "ProcessHandler"); } - string cmdExeArgs = PrepareCmdExeArgs(command, arguments, enableFileArgs); + string cmdExeArgs = $"/c \"{command} {arguments}"; + cmdExeArgs += _modifyEnvironment + ? $" && echo {OutputDelimiter} && set \"" + : "\""; // Invoke the process. ExecutionContext.Debug($"{cmdExe} {cmdExeArgs}"); @@ -211,47 +207,48 @@ public async Task RunAsync() } } - private string PrepareCmdExeArgs(string command, string arguments, bool enableFileArgs) - { - string cmdExeArgs; - if (enableFileArgs) - { - cmdExeArgs = $"/c \"{_generatedScriptPath}\""; - } - else - { - // Format the input to be invoked from cmd.exe to enable built-in shell commands. For example, RMDIR. - cmdExeArgs = $"/c \"{command} {arguments}"; - cmdExeArgs += _modifyEnvironment - ? $" && echo {OutputDelimiter} && set \"" - : "\""; - } - - return cmdExeArgs; - } - - private void GenerateScriptFile(string cmdExe, string command, string arguments) + private void ValidateInputArguments(string inputArgs) { - var scriptId = Guid.NewGuid().ToString(); - string inputArgsEnvVarName = VarUtil.ConvertToEnvVariableFormat("AGENT_PH_ARGS_" + scriptId[..8]); - - System.Environment.SetEnvironmentVariable(inputArgsEnvVarName, arguments); + var enableValidation = AgentKnobs.ProcessHandlerSecureArguments.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable args validation: '{enableValidation}'"); + var enableAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable args validation audit: '{enableAudit}'"); + var enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable telemetry: '{enableTelemetry}'"); - var agentTemp = ExecutionContext.GetVariableValueOrDefault(Constants.Variables.Agent.TempDirectory); - _generatedScriptPath = Path.Combine(agentTemp, $"processHandlerScript_{scriptId}.cmd"); + if (enableValidation || enableAudit || enableTelemetry) + { + ExecutionContext.Debug("Starting args sanitization"); + var (expandedArgs, envExpandTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs); - var scriptArgs = $"/v:ON /c \"{command} !{inputArgsEnvVarName}!"; + var (sanitizedArgs, sanitizeTelemetry) = CmdArgsSanitizer.SanitizeArguments(expandedArgs); - scriptArgs += _modifyEnvironment - ? $" && echo {OutputDelimiter} && set \"" - : "\""; + if (sanitizedArgs != inputArgs) + { + if (enableTelemetry) + { + var telemetry = envExpandTelemetry.ToDictionary(); + if (sanitizeTelemetry != null) + { + telemetry.AddRange(sanitizeTelemetry.ToDictionary()); + } - using (var writer = new StreamWriter(_generatedScriptPath)) + PublishTelemetry(telemetry, "ProcessHandler"); + } + if (enableAudit && !enableValidation) + { + ExecutionContext.Warning(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } + if (enableValidation) + { + throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } + } + } + else { - writer.WriteLine($"{cmdExe} {scriptArgs}"); + ExecutionContext.Debug("Args sanitization skipped."); } - - ExecutionContext.Debug($"Generated script file: {_generatedScriptPath}"); } private void FlushErrorData() @@ -326,4 +323,11 @@ private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs e) } } } + + public class ArgsSanitizedException : Exception + { + public ArgsSanitizedException(string message) : base(message) + { + } + } } diff --git a/src/Misc/layoutbin/en-US/strings.json b/src/Misc/layoutbin/en-US/strings.json index 2250c75638..71fe167b0c 100644 --- a/src/Misc/layoutbin/en-US/strings.json +++ b/src/Misc/layoutbin/en-US/strings.json @@ -458,6 +458,7 @@ "ProcessCompletedWithCode0Errors1": "Process completed with exit code {0} and had {1} error(s) written to the error stream.", "ProcessCompletedWithExitCode0": "Process completed with exit code {0}.", "ProcessExitCode": "Exit code {0} returned from process: file name '{1}', arguments '{2}'.", + "ProcessHandlerScriptArgsSanitized": "Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using circumflex (^). More information is available here: https://aka.ms/ado/75787", "ProfileLoadFailure": "Unable to load the user profile for the user {0}\\{1} AutoLogon configuration is not possible.", "ProjectName": "Project name", "Prompt0": "Enter {0}", diff --git a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs new file mode 100644 index 0000000000..ba24114903 --- /dev/null +++ b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs @@ -0,0 +1,77 @@ +using Agent.Worker.Handlers.Helpers; +using System; +using System.Collections.Generic; +using Xunit; + +namespace Test.L0.Worker.Handlers +{ + public class CmdArgsSanitizerL0 + { + [Fact] + public void EmptyLineTest() + { + string argsLine = ""; + string expectedArgs = ""; + + var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(argsLine); + + Assert.Equal(expectedArgs, actualArgs); + } + + [Theory] + [InlineData("1; 2", "1_#removed#_ 2")] + [InlineData("1 ^^; 2", "1 ^^_#removed#_ 2")] + [InlineData("1 ; 2 && 3", "1 _#removed#_ 2 _#removed#__#removed#_ 3")] + [InlineData("; & > < |", "_#removed#_ _#removed#_ _#removed#_ _#removed#_ _#removed#_")] + public void SanitizeTest(string inputArgs, string expectedArgs) + { + var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.Equal(expectedArgs, actualArgs); + } + + [Theory] + [InlineData("1 2")] + [InlineData("1 ^; 2")] + [InlineData("1 ^; 2 ^&^& 3 ^< ^> ^| ^^")] + [InlineData(", / \\ aA zZ 09 ' \" - = : . * + ? ^")] + public void SanitizeSkipTest(string inputArgs) + { + var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.Equal(inputArgs, actualArgs); + } + + [Theory] + [ClassData(typeof(SanitizerTelemetryTestsData))] + public void Telemetry_BasicTest(string inputArgs, int expectedRemovedSymbolsCount, Dictionary expectedRemovedSymbols) + { + var (_, resultTelemetry) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.NotNull(resultTelemetry); + Assert.Equal(expectedRemovedSymbolsCount, resultTelemetry.RemovedSymbolsCount); + Assert.Equal(expectedRemovedSymbols, resultTelemetry.RemovedSymbols); + } + + public class SanitizerTelemetryTestsData : TheoryData> + { + public SanitizerTelemetryTestsData() + { + Add("; &&&;; $", 7, new() { [";"] = 3, ["&"] = 3, ["$"] = 1 }); + Add("aA zZ 09;", 1, new() { [";"] = 1 }); + Add("; & > < |", 5, new() { [";"] = 1, ["&"] = 1, [">"] = 1, ["<"] = 1, ["|"] = 1 }); + } + } + + [Theory] + [InlineData("")] + [InlineData("123")] + [InlineData("1 ^; ^&")] + public void Telemetry_ReturnsNull(string inputArgs) + { + var (_, resultTelemetry) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.Null(resultTelemetry); + } + } +} diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 9b8b4e22e0..add83ae86e 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -1,7 +1,5 @@ -using Agent.Worker.Handlers.Helpers; -using System; -using System.Collections.Generic; -using System.Text; +using System; +using Agent.Worker.Handlers.Helpers; using Xunit; namespace Test.L0.Worker.Handlers @@ -14,7 +12,7 @@ public void EmptyLineTest() string argsLine = ""; string expectedArgs = ""; - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedArgs, actualArgs); } @@ -26,7 +24,7 @@ public void BasicTest() string expectedArgs = "value1 2"; Environment.SetEnvironmentVariable("VAR1", "value1"); - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedArgs, actualArgs); } @@ -39,7 +37,7 @@ public void TestWithMultipleVars() Environment.SetEnvironmentVariable("VAR1", "value1"); Environment.SetEnvironmentVariable("VAR2", "value2"); - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedArgs, actualArgs); } @@ -54,7 +52,7 @@ public void TestWithCloseVars(string inputArgs, string expectedArgs) Environment.SetEnvironmentVariable("VAR2", "2"); Environment.SetEnvironmentVariable("VAR3", "3"); - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(inputArgs); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs); Assert.Equal(expectedArgs, actualArgs); } @@ -68,7 +66,7 @@ public void NestedVariablesNotExpands() Environment.SetEnvironmentVariable("VAR2", "2"); Environment.SetEnvironmentVariable("NESTED", "nested"); - var (actualArgs, _) = ProcessHandlerHelper.ExpandArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedArgs, actualArgs); } @@ -81,7 +79,7 @@ public void SkipsInvalidEnv() string expectedArgs = "%VAR1% 2"; - var (actualArgs, _) = ProcessHandlerHelper.ExpandArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedArgs, actualArgs); } diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index cd43585157..bd4e9a9fcf 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -1,7 +1,4 @@ using Agent.Worker.Handlers.Helpers; -using System; -using System.Collections.Generic; -using System.Text; using Xunit; namespace Test.L0.Worker.Handlers @@ -15,7 +12,7 @@ public void FoundPrefixesTest() // we're thinking that whitespaces are also may be env variables, so here the '% %' and '%' env enterances. var expectedTelemetry = new { foundPrefixes = 2 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.foundPrefixes, resultTelemetry.FoundPrefixes); } @@ -26,7 +23,7 @@ public void NotClosedEnv() string argsLine = "%1"; var expectedTelemetry = new { NotClosedEnvSyntaxPosition = 0 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.NotClosedEnvSyntaxPosition, resultTelemetry.NotClosedEnvSyntaxPosition); } @@ -37,7 +34,7 @@ public void NotClosedEnv2() string argsLine = "\"%\" %"; var expectedTelemetry = new { NotClosedEnvSyntaxPosition = 4 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.NotClosedEnvSyntaxPosition, resultTelemetry.NotClosedEnvSyntaxPosition); } @@ -48,7 +45,7 @@ public void NotClosedQuotes() string argsLine = "\" %var%"; var expectedTelemetry = new { quotesNotEnclosed = 1 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.quotesNotEnclosed, resultTelemetry.QuotesNotEnclosed); } @@ -59,7 +56,7 @@ public void NotClosedQuotes_Ignore_if_no_envVar() string argsLine = "\" 1"; var expectedTelemetry = new { quotesNotEnclosed = 0 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.quotesNotEnclosed, resultTelemetry.QuotesNotEnclosed); } @@ -71,7 +68,7 @@ public void QuotedBlocksCount() string argsLine = "\"%VAR1%\" \"%VAR2%\" \"3\""; var expectedTelemetry = new { quottedBlocks = 2 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.quottedBlocks, resultTelemetry.QuottedBlocks); } @@ -82,7 +79,7 @@ public void CountsVariablesStartFromEscSymbol() string argsLine = "%^VAR1% \"%^VAR2%\" %^VAR3%"; var expectedTelemetry = new { variablesStartsFromES = 2 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); Assert.Equal(expectedTelemetry.variablesStartsFromES, resultTelemetry.VariablesStartsFromES); } From 2daebb50df53c070ebecf99efdad1baebcd718b7 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 8 Sep 2023 18:31:11 +0400 Subject: [PATCH 03/25] Remove test ex --- src/Agent.Worker/Handlers/ProcessHandler.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index f84276d598..82cc1ba33a 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -134,7 +134,6 @@ public async Task RunAsync() try { - throw new Exception("Test exc"); ValidateInputArguments(arguments); } catch (ArgsSanitizedException) From 4c034fd1ca6db9acd384b69bbb3f7cc25ee80e9f Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Tue, 19 Sep 2023 13:48:32 +0400 Subject: [PATCH 04/25] Add copyright + cleanup imports --- src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs | 5 ++++- src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs | 5 ++++- src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs | 8 +++++--- src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs | 7 +++++-- .../L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs | 5 ++++- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs index cf2b8389e8..4dedf78eeb 100644 --- a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -1,7 +1,10 @@ -using Microsoft.VisualStudio.Services.Agent.Util; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; +using Microsoft.VisualStudio.Services.Agent.Util; namespace Agent.Worker.Handlers.Helpers { diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index ae6d125e41..a035d496ca 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -1,4 +1,7 @@ -using System; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; using System.Collections.Generic; namespace Agent.Worker.Handlers.Helpers diff --git a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs index ba24114903..bac2e2265e 100644 --- a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs +++ b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs @@ -1,7 +1,9 @@ -using Agent.Worker.Handlers.Helpers; -using System; -using System.Collections.Generic; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + using Xunit; +using System.Collections.Generic; +using Agent.Worker.Handlers.Helpers; namespace Test.L0.Worker.Handlers { diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index add83ae86e..bd994e8142 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -1,6 +1,9 @@ -using System; -using Agent.Worker.Handlers.Helpers; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; using Xunit; +using Agent.Worker.Handlers.Helpers; namespace Test.L0.Worker.Handlers { diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index bd4e9a9fcf..cd03a911a2 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -1,5 +1,8 @@ -using Agent.Worker.Handlers.Helpers; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + using Xunit; +using Agent.Worker.Handlers.Helpers; namespace Test.L0.Worker.Handlers { From 981ada657fbe71c571f98c9978e2376f346d8d01 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Tue, 19 Sep 2023 14:17:33 +0400 Subject: [PATCH 05/25] Simplify telemetry type --- src/Agent.Worker/Handlers/Handler.cs | 4 ++-- src/Agent.Worker/Handlers/ProcessHandler.cs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Agent.Worker/Handlers/Handler.cs b/src/Agent.Worker/Handlers/Handler.cs index 2e4306e795..8c73d78c4f 100644 --- a/src/Agent.Worker/Handlers/Handler.cs +++ b/src/Agent.Worker/Handlers/Handler.cs @@ -308,8 +308,8 @@ protected void RemovePSModulePathFromEnvironment() } } - protected void PublishTelemetry( - Dictionary telemetryData, + protected void PublishTelemetry( + object telemetryData, string feature = "TaskHandler" ) { diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 82cc1ba33a..e4950f5f65 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -144,10 +144,10 @@ public async Task RunAsync() { Trace.Error($"Failed to validate process handler input arguments. Publishing telemetry. Ex: {ex}"); - var telemetry = new Dictionary() + var telemetry = new { - ["UnexpectedError"] = ex.Message, - ["ErrorStackTrace"] = ex.StackTrace + UnexpectedError = ex.Message, + ErrorStackTrace = ex.StackTrace }; PublishTelemetry(telemetry, "ProcessHandler"); } From 32b209feb0b6083bf54900ea9770528d5d5ad63a Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Tue, 19 Sep 2023 14:24:16 +0400 Subject: [PATCH 06/25] Simplify PH telemetry tests --- .../ProcessHandlerHelperTelemetryL0.cs | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index cd03a911a2..27fb4cbc84 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -12,56 +12,49 @@ public sealed class ProcessHandlerHelperTelemetryL0 public void FoundPrefixesTest() { string argsLine = "% % %"; - // we're thinking that whitespaces are also may be env variables, so here the '% %' and '%' env enterances. - var expectedTelemetry = new { foundPrefixes = 2 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.foundPrefixes, resultTelemetry.FoundPrefixes); + Assert.Equal(2, resultTelemetry.FoundPrefixes); } [Fact] public void NotClosedEnv() { string argsLine = "%1"; - var expectedTelemetry = new { NotClosedEnvSyntaxPosition = 0 }; var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.NotClosedEnvSyntaxPosition, resultTelemetry.NotClosedEnvSyntaxPosition); + Assert.Equal(0, resultTelemetry.NotClosedEnvSyntaxPosition); } [Fact] public void NotClosedEnv2() { string argsLine = "\"%\" %"; - var expectedTelemetry = new { NotClosedEnvSyntaxPosition = 4 }; var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.NotClosedEnvSyntaxPosition, resultTelemetry.NotClosedEnvSyntaxPosition); + Assert.Equal(4, resultTelemetry.NotClosedEnvSyntaxPosition); } [Fact] public void NotClosedQuotes() { string argsLine = "\" %var%"; - var expectedTelemetry = new { quotesNotEnclosed = 1 }; var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.quotesNotEnclosed, resultTelemetry.QuotesNotEnclosed); + Assert.Equal(1, resultTelemetry.QuotesNotEnclosed); } [Fact] public void NotClosedQuotes_Ignore_if_no_envVar() { string argsLine = "\" 1"; - var expectedTelemetry = new { quotesNotEnclosed = 0 }; var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.quotesNotEnclosed, resultTelemetry.QuotesNotEnclosed); + Assert.Equal(0, resultTelemetry.QuotesNotEnclosed); } [Fact] @@ -69,22 +62,20 @@ public void QuotedBlocksCount() { // We're ignoring quote blocks where no any env variables string argsLine = "\"%VAR1%\" \"%VAR2%\" \"3\""; - var expectedTelemetry = new { quottedBlocks = 2 }; var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.quottedBlocks, resultTelemetry.QuottedBlocks); + Assert.Equal(2, resultTelemetry.QuottedBlocks); } [Fact] public void CountsVariablesStartFromEscSymbol() { string argsLine = "%^VAR1% \"%^VAR2%\" %^VAR3%"; - var expectedTelemetry = new { variablesStartsFromES = 2 }; var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); - Assert.Equal(expectedTelemetry.variablesStartsFromES, resultTelemetry.VariablesStartsFromES); + Assert.Equal(2, resultTelemetry.VariablesStartsFromES); } } } From fc640dfa84216f317e9f48da7adf0a84208fe928 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 03:23:27 +0400 Subject: [PATCH 07/25] Update debug logs --- src/Agent.Worker/Handlers/ProcessHandler.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index e4950f5f65..b560795ad7 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -217,9 +217,11 @@ private void ValidateInputArguments(string inputArgs) if (enableValidation || enableAudit || enableTelemetry) { - ExecutionContext.Debug("Starting args sanitization"); + ExecutionContext.Debug("Starting args env expansion"); var (expandedArgs, envExpandTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs); + ExecutionContext.Debug($"Expanded args={expandedArgs}"); + ExecutionContext.Debug("Starting args sanitization"); var (sanitizedArgs, sanitizeTelemetry) = CmdArgsSanitizer.SanitizeArguments(expandedArgs); if (sanitizedArgs != inputArgs) From c6c6bcf3faeff6727ce026af043de08cd437fe5f Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 17:13:42 +0400 Subject: [PATCH 08/25] Exclude % from allowed symbols --- src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs index 4dedf78eeb..696d6020e4 100644 --- a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -23,7 +23,7 @@ public static (string, CmdArgsSanitizingTelemetry) SanitizeArguments(string inpu var argsChunks = inputArgs.Split(argsSplitSymbols); var matchesChunks = new List(); - var saniziteRegExp = new Regex("(? Date: Fri, 22 Sep 2023 17:30:42 +0400 Subject: [PATCH 09/25] Fix cmd expand env --- .../Handlers/Helpers/ProcessHandlerHelper.cs | 14 ++-- src/Agent.Worker/Handlers/ProcessHandler.cs | 2 +- .../Worker/Handlers/ProcessHandlerHelperL0.cs | 69 ++++++++++++++----- .../ProcessHandlerHelperTelemetryL0.cs | 14 ++-- 4 files changed, 69 insertions(+), 30 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index a035d496ca..8878350270 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -3,13 +3,17 @@ using System; using System.Collections.Generic; +using Microsoft.VisualStudio.Services.Agent.Util; namespace Agent.Worker.Handlers.Helpers { public static class ProcessHandlerHelper { - public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs) + public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary environment) { + ArgUtil.NotNull(inputArgs, nameof(inputArgs)); + ArgUtil.NotNull(environment, nameof(environment)); + const char quote = '"'; const char escapingSymbol = '^'; const string envPrefix = "%"; @@ -95,12 +99,14 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs) telemetry.VariablesWithESInside++; } - var envValue = Environment.GetEnvironmentVariable(envName); + // Since Windows have case-insensetive environment, and Process handler is windows-specific, we should allign this behavior. + var windowsEnvironment = new Dictionary(environment, StringComparer.OrdinalIgnoreCase); + // In case we don't have such variable, we just leave it as is - if (string.IsNullOrEmpty(envValue)) + if (!windowsEnvironment.TryGetValue(envName, out string envValue) || string.IsNullOrEmpty(envValue)) { telemetry.NotExistingEnv++; - startIndex = envEndIndex; + startIndex = envEndIndex + 1; continue; } diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index b560795ad7..02baa265d9 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -218,7 +218,7 @@ private void ValidateInputArguments(string inputArgs) if (enableValidation || enableAudit || enableTelemetry) { ExecutionContext.Debug("Starting args env expansion"); - var (expandedArgs, envExpandTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs); + var (expandedArgs, envExpandTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, Environment); ExecutionContext.Debug($"Expanded args={expandedArgs}"); ExecutionContext.Debug("Starting args sanitization"); diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index bd994e8142..392c3a8e09 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -1,9 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -using System; using Xunit; using Agent.Worker.Handlers.Helpers; +using System.Collections.Generic; namespace Test.L0.Worker.Handlers { @@ -15,7 +15,7 @@ public void EmptyLineTest() string argsLine = ""; string expectedArgs = ""; - var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(expectedArgs, actualArgs); } @@ -25,9 +25,11 @@ public void BasicTest() { string argsLine = "%VAR1% 2"; string expectedArgs = "value1 2"; - Environment.SetEnvironmentVariable("VAR1", "value1"); - - var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var testEnv = new Dictionary() + { + ["VAR1"] = "value1" + }; + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } @@ -37,10 +39,13 @@ public void TestWithMultipleVars() { string argsLine = "1 %VAR1% %VAR2%"; string expectedArgs = "1 value1 value2"; - Environment.SetEnvironmentVariable("VAR1", "value1"); - Environment.SetEnvironmentVariable("VAR2", "value2"); + var testEnv = new Dictionary() + { + ["VAR1"] = "value1", + ["VAR2"] = "value2" + }; - var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } @@ -51,11 +56,14 @@ public void TestWithMultipleVars() [InlineData("%VAR1%%VAR2%%VAR3%", "123")] public void TestWithCloseVars(string inputArgs, string expectedArgs) { - Environment.SetEnvironmentVariable("VAR1", "1"); - Environment.SetEnvironmentVariable("VAR2", "2"); - Environment.SetEnvironmentVariable("VAR3", "3"); + var testEnv = new Dictionary() + { + { "VAR1", "1" }, + { "VAR2", "2" }, + { "VAR3", "3" } + }; - var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, testEnv); Assert.Equal(expectedArgs, actualArgs); } @@ -65,11 +73,14 @@ public void NestedVariablesNotExpands() { string argsLine = "%VAR1% %VAR2%"; string expectedArgs = "%NESTED% 2"; - Environment.SetEnvironmentVariable("VAR1", "%NESTED%"); - Environment.SetEnvironmentVariable("VAR2", "2"); - Environment.SetEnvironmentVariable("NESTED", "nested"); + var testEnv = new Dictionary() + { + { "VAR1", "%NESTED%" }, + { "VAR2", "2"}, + { "NESTED", "nested" } + }; - var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } @@ -78,13 +89,35 @@ public void NestedVariablesNotExpands() public void SkipsInvalidEnv() { string argsLine = "%VAR1% 2"; - Environment.SetEnvironmentVariable("VAR1", null); + var testEnv = new Dictionary() + { + { "VAR1", null} + }; string expectedArgs = "%VAR1% 2"; - var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + [Trait("SkipOn", "darwin")] + [Trait("SkipOn", "linux")] + public void WindowsCaseInsensetiveTest() + { + string argsLine = "%var1% 2"; + var testEnv = new Dictionary() + { + { "VAR1", "value1"} + }; + + string expandedArgs = "value1 2"; + + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); + Assert.Equal(expandedArgs, actualArgs); + } } } diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index 27fb4cbc84..a12f8696a5 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -12,7 +12,7 @@ public sealed class ProcessHandlerHelperTelemetryL0 public void FoundPrefixesTest() { string argsLine = "% % %"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(2, resultTelemetry.FoundPrefixes); } @@ -22,7 +22,7 @@ public void NotClosedEnv() { string argsLine = "%1"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(0, resultTelemetry.NotClosedEnvSyntaxPosition); } @@ -32,7 +32,7 @@ public void NotClosedEnv2() { string argsLine = "\"%\" %"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(4, resultTelemetry.NotClosedEnvSyntaxPosition); } @@ -42,7 +42,7 @@ public void NotClosedQuotes() { string argsLine = "\" %var%"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(1, resultTelemetry.QuotesNotEnclosed); } @@ -52,7 +52,7 @@ public void NotClosedQuotes_Ignore_if_no_envVar() { string argsLine = "\" 1"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(0, resultTelemetry.QuotesNotEnclosed); } @@ -63,7 +63,7 @@ public void QuotedBlocksCount() // We're ignoring quote blocks where no any env variables string argsLine = "\"%VAR1%\" \"%VAR2%\" \"3\""; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(2, resultTelemetry.QuottedBlocks); } @@ -73,7 +73,7 @@ public void CountsVariablesStartFromEscSymbol() { string argsLine = "%^VAR1% \"%^VAR2%\" %^VAR3%"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(2, resultTelemetry.VariablesStartsFromES); } From 280eb30170b4b5f109d52e7db388cf863aaccf08 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 18:07:08 +0400 Subject: [PATCH 10/25] remove useless telemetry --- src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 8878350270..97c4f99aac 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -76,8 +76,6 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary ToDictionary() ["escapedEscapingSymbols"] = EscapedEscapingSymbols, ["variablesStartsFromES"] = VariablesStartsFromES, ["braceSyntaxEntries"] = BraceSyntaxEntries, - ["bracedVariables"] = BracedVariables, ["bariablesWithESInside"] = VariablesWithESInside, ["quotesNotEnclosed"] = QuotesNotEnclosed, ["notClosedBraceSyntaxPosition"] = NotClosedEnvSyntaxPosition, From 4a3ec54d4e76443d3b3d1924d3bcec5e5a955af4 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 18:24:12 +0400 Subject: [PATCH 11/25] Add test traits --- src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs | 10 ++++++++++ .../L0/Worker/Handlers/ProcessHandlerHelperL0.cs | 13 +++++++++++++ .../Handlers/ProcessHandlerHelperTelemetryL0.cs | 14 ++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs index bac2e2265e..ae18d3c38f 100644 --- a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs +++ b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs @@ -10,6 +10,8 @@ namespace Test.L0.Worker.Handlers public class CmdArgsSanitizerL0 { [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void EmptyLineTest() { string argsLine = ""; @@ -25,6 +27,8 @@ public void EmptyLineTest() [InlineData("1 ^^; 2", "1 ^^_#removed#_ 2")] [InlineData("1 ; 2 && 3", "1 _#removed#_ 2 _#removed#__#removed#_ 3")] [InlineData("; & > < |", "_#removed#_ _#removed#_ _#removed#_ _#removed#_ _#removed#_")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void SanitizeTest(string inputArgs, string expectedArgs) { var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(inputArgs); @@ -37,6 +41,8 @@ public void SanitizeTest(string inputArgs, string expectedArgs) [InlineData("1 ^; 2")] [InlineData("1 ^; 2 ^&^& 3 ^< ^> ^| ^^")] [InlineData(", / \\ aA zZ 09 ' \" - = : . * + ? ^")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void SanitizeSkipTest(string inputArgs) { var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(inputArgs); @@ -46,6 +52,8 @@ public void SanitizeSkipTest(string inputArgs) [Theory] [ClassData(typeof(SanitizerTelemetryTestsData))] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void Telemetry_BasicTest(string inputArgs, int expectedRemovedSymbolsCount, Dictionary expectedRemovedSymbols) { var (_, resultTelemetry) = CmdArgsSanitizer.SanitizeArguments(inputArgs); @@ -69,6 +77,8 @@ public SanitizerTelemetryTestsData() [InlineData("")] [InlineData("123")] [InlineData("1 ^; ^&")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void Telemetry_ReturnsNull(string inputArgs) { var (_, resultTelemetry) = CmdArgsSanitizer.SanitizeArguments(inputArgs); diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 392c3a8e09..1f5a1a039c 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -10,6 +10,8 @@ namespace Test.L0.Worker.Handlers public sealed class ProcessHandlerHelperL0 { [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void EmptyLineTest() { string argsLine = ""; @@ -21,6 +23,8 @@ public void EmptyLineTest() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void BasicTest() { string argsLine = "%VAR1% 2"; @@ -35,6 +39,8 @@ public void BasicTest() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void TestWithMultipleVars() { string argsLine = "1 %VAR1% %VAR2%"; @@ -54,6 +60,8 @@ public void TestWithMultipleVars() [InlineData("%VAR1% %VAR2%%VAR3%", "1 23")] [InlineData("%VAR1% %VAR2%_%VAR3%", "1 2_3")] [InlineData("%VAR1%%VAR2%%VAR3%", "123")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void TestWithCloseVars(string inputArgs, string expectedArgs) { var testEnv = new Dictionary() @@ -69,6 +77,8 @@ public void TestWithCloseVars(string inputArgs, string expectedArgs) } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NestedVariablesNotExpands() { string argsLine = "%VAR1% %VAR2%"; @@ -86,6 +96,8 @@ public void NestedVariablesNotExpands() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void SkipsInvalidEnv() { string argsLine = "%VAR1% 2"; @@ -103,6 +115,7 @@ public void SkipsInvalidEnv() [Fact] [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] [Trait("Category", "Worker")] [Trait("SkipOn", "darwin")] [Trait("SkipOn", "linux")] diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index a12f8696a5..d9fea13cbe 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -9,6 +9,8 @@ namespace Test.L0.Worker.Handlers public sealed class ProcessHandlerHelperTelemetryL0 { [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void FoundPrefixesTest() { string argsLine = "% % %"; @@ -18,6 +20,8 @@ public void FoundPrefixesTest() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NotClosedEnv() { string argsLine = "%1"; @@ -28,6 +32,8 @@ public void NotClosedEnv() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NotClosedEnv2() { string argsLine = "\"%\" %"; @@ -38,6 +44,8 @@ public void NotClosedEnv2() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NotClosedQuotes() { string argsLine = "\" %var%"; @@ -48,6 +56,8 @@ public void NotClosedQuotes() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NotClosedQuotes_Ignore_if_no_envVar() { string argsLine = "\" 1"; @@ -58,6 +68,8 @@ public void NotClosedQuotes_Ignore_if_no_envVar() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void QuotedBlocksCount() { // We're ignoring quote blocks where no any env variables @@ -69,6 +81,8 @@ public void QuotedBlocksCount() } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void CountsVariablesStartFromEscSymbol() { string argsLine = "%^VAR1% \"%^VAR2%\" %^VAR3%"; From 4568202c8868d8e62d22814e63eaf0e028519a59 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 19:55:02 +0400 Subject: [PATCH 12/25] Fix env incorrect expanding --- .../Handlers/Helpers/ProcessHandlerHelper.cs | 6 +++++- .../Worker/Handlers/ProcessHandlerHelperL0.cs | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 97c4f99aac..66a61742c9 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -35,7 +35,8 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary 0 && result[prefixIndex - 1] == escapingSymbol) { - if (result[prefixIndex - 2] == 0 || result[prefixIndex - 2] != escapingSymbol) + int checkInx = prefixIndex - 2; + if (checkInx > 2 && (result[checkInx] == 0 || result[checkInx] != escapingSymbol)) { startIndex++; result = result[..(prefixIndex - 1)] + result[prefixIndex..]; @@ -45,7 +46,10 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary if just no close quote, then break diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 1f5a1a039c..2c80332d03 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -113,6 +113,23 @@ public void SkipsInvalidEnv() Assert.Equal(expectedArgs, actualArgs); } + [Theory] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + [InlineData("^%var")] + [InlineData("%var")] + [InlineData("^%var%")] + public void TestNoChanges(string input) + { + var testEnv = new Dictionary + { + { "var", "value" } + }; + var (output, _) = ProcessHandlerHelper.ExpandCmdEnv(input, testEnv); + + Assert.Equal(input, output); + } + [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] From c2549b40a68660ec5b61ab2b63ffd175d824c42e Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 20:04:15 +0400 Subject: [PATCH 13/25] Add more test cases --- src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 2c80332d03..29fb7c931b 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -116,9 +116,12 @@ public void SkipsInvalidEnv() [Theory] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] - [InlineData("^%var")] [InlineData("%var")] + [InlineData("%someothervar%")] + [InlineData("^%var")] [InlineData("^%var%")] + [InlineData("^^%var%")] // we'll keep this case as a pessimistic for now. + [InlineData("^^^%var%")] public void TestNoChanges(string input) { var testEnv = new Dictionary From 4c8eb5e4701348fac713b97acafd191da6bc298a Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 22 Sep 2023 21:26:05 +0400 Subject: [PATCH 14/25] Fix exception condition --- src/Agent.Worker/Handlers/ProcessHandler.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 02baa265d9..f3577c58c1 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -236,13 +236,16 @@ private void ValidateInputArguments(string inputArgs) PublishTelemetry(telemetry, "ProcessHandler"); } - if (enableAudit && !enableValidation) + if (sanitizedArgs != expandedArgs) { - ExecutionContext.Warning(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); - } - if (enableValidation) - { - throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + if (enableAudit && !enableValidation) + { + ExecutionContext.Warning(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } + if (enableValidation) + { + throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } } } } From 1ad825f122923f63ac104998ea527d59472577cc Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Sat, 23 Sep 2023 01:04:48 +0400 Subject: [PATCH 15/25] Move validation args to public --- .../Handlers/Helpers/ProcessHandlerHelper.cs | 75 ++++++++++++++++--- src/Agent.Worker/Handlers/ProcessHandler.cs | 60 +++------------ .../Worker/Handlers/ProcessHandlerHelperL0.cs | 54 +++++++++++++ 3 files changed, 129 insertions(+), 60 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 66a61742c9..821401f4b9 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -3,7 +3,10 @@ using System; using System.Collections.Generic; +using Agent.Sdk.Knob; using Microsoft.VisualStudio.Services.Agent.Util; +using Microsoft.VisualStudio.Services.Agent.Worker; +using Microsoft.VisualStudio.Services.Common; namespace Agent.Worker.Handlers.Helpers { @@ -35,20 +38,18 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary 0 && result[prefixIndex - 1] == escapingSymbol) { - int checkInx = prefixIndex - 2; - if (checkInx > 2 && (result[checkInx] == 0 || result[checkInx] != escapingSymbol)) + int beforeBeforePrefix = prefixIndex - 2; + if (beforeBeforePrefix < 0 || result[beforeBeforePrefix] != escapingSymbol) { - startIndex++; - result = result[..(prefixIndex - 1)] + result[prefixIndex..]; - telemetry.EscapedVariables++; - - continue; + } + else + { + telemetry.EscapedEscapingSymbols++; } startIndex++; - telemetry.EscapedEscapingSymbols++; continue; } @@ -84,7 +85,6 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary) ValidateInputArguments( + string inputArgs, + Dictionary environment, + IExecutionContext context) + { + var enableValidation = AgentKnobs.ProcessHandlerSecureArguments.GetValue(context).AsBoolean(); + context.Debug($"Enable args validation: '{enableValidation}'"); + var enableAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(context).AsBoolean(); + context.Debug($"Enable args validation audit: '{enableAudit}'"); + var enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(context).AsBoolean(); + context.Debug($"Enable telemetry: '{enableTelemetry}'"); + + if (enableValidation || enableAudit || enableTelemetry) + { + context.Debug("Starting args env expansion"); + var (expandedArgs, envExpandTelemetry) = ExpandCmdEnv(inputArgs, environment); + context.Debug($"Expanded args={expandedArgs}"); + + context.Debug("Starting args sanitization"); + var (sanitizedArgs, sanitizeTelemetry) = CmdArgsSanitizer.SanitizeArguments(expandedArgs); + + Dictionary telemetry = null; + if (sanitizedArgs != inputArgs) + { + if (enableTelemetry) + { + telemetry = envExpandTelemetry.ToDictionary(); + if (sanitizeTelemetry != null) + { + telemetry.AddRange(sanitizeTelemetry.ToDictionary()); + } + } + if (sanitizedArgs != expandedArgs) + { + if (enableAudit && !enableValidation) + { + context.Warning(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } + if (enableValidation) + { + return (false, telemetry); + } + + return (true, telemetry); + } + } + + return (true, null); + } + else + { + context.Debug("Args sanitization skipped."); + return (true, null); + } + } + } public class CmdTelemetry { public int FoundPrefixes { get; set; } = 0; diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index f3577c58c1..f624d7e45a 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -134,7 +134,16 @@ public async Task RunAsync() try { - ValidateInputArguments(arguments); + var (isValid, telemetry) = ProcessHandlerHelper.ValidateInputArguments(arguments, Environment, ExecutionContext); + + if (telemetry != null) + { + PublishTelemetry(telemetry, "ProcessHandler"); + } + if (!isValid) + { + throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } } catch (ArgsSanitizedException) { @@ -206,55 +215,6 @@ public async Task RunAsync() } } - private void ValidateInputArguments(string inputArgs) - { - var enableValidation = AgentKnobs.ProcessHandlerSecureArguments.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable args validation: '{enableValidation}'"); - var enableAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable args validation audit: '{enableAudit}'"); - var enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable telemetry: '{enableTelemetry}'"); - - if (enableValidation || enableAudit || enableTelemetry) - { - ExecutionContext.Debug("Starting args env expansion"); - var (expandedArgs, envExpandTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, Environment); - ExecutionContext.Debug($"Expanded args={expandedArgs}"); - - ExecutionContext.Debug("Starting args sanitization"); - var (sanitizedArgs, sanitizeTelemetry) = CmdArgsSanitizer.SanitizeArguments(expandedArgs); - - if (sanitizedArgs != inputArgs) - { - if (enableTelemetry) - { - var telemetry = envExpandTelemetry.ToDictionary(); - if (sanitizeTelemetry != null) - { - telemetry.AddRange(sanitizeTelemetry.ToDictionary()); - } - - PublishTelemetry(telemetry, "ProcessHandler"); - } - if (sanitizedArgs != expandedArgs) - { - if (enableAudit && !enableValidation) - { - ExecutionContext.Warning(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); - } - if (enableValidation) - { - throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); - } - } - } - } - else - { - ExecutionContext.Debug("Args sanitization skipped."); - } - } - private void FlushErrorData() { if (_errorBuffer.Length > 0) diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 29fb7c931b..15a7946f00 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -4,6 +4,8 @@ using Xunit; using Agent.Worker.Handlers.Helpers; using System.Collections.Generic; +using Microsoft.VisualStudio.Services.Agent.Worker; +using Moq; namespace Test.L0.Worker.Handlers { @@ -152,5 +154,57 @@ public void WindowsCaseInsensetiveTest() var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expandedArgs, actualArgs); } + + // TODO: Code smell. Refactor, remove specific logic from here. + [Theory] + [InlineData("%var%", "1 & echo 23")] + [InlineData("%var%%", "1 & echo 23")] + [InlineData("%%%var%", "1 & echo 23")] + [InlineData("1 & echo 23", "")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void ArgsValidation_Failes(string inputArgs, string envVarValue) + { + var testEnv = new Dictionary + { + {"var", envVarValue}, + }; + + var mockContext = CreateMockExecContext(); + + var (isValid, _) = ProcessHandlerHelper.ValidateInputArguments(inputArgs, testEnv, mockContext.Object); + + Assert.False(isValid); + } + + // TODO: Code smell. Refactor, remove specific logic from here. + [Theory] + [InlineData("", "")] + [InlineData("%%var%", "1 & whoami")] + [InlineData("%%%%var%", "1 & whoami")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void ArgsValidation_Passes(string inputArgs, string envVarValue) + { + var testEnv = new Dictionary + { + {"var", envVarValue}, + }; + + var mockContext = CreateMockExecContext(); + + var (isValid, _) = ProcessHandlerHelper.ValidateInputArguments(inputArgs, testEnv, mockContext.Object); + + Assert.True(isValid); + } + + + private Mock CreateMockExecContext() + { + var mockContext = new Mock(); + mockContext.Setup(x => x.GetVariableValueOrDefault(It.IsAny())).Returns("true"); + + return mockContext; + } } } From b6fd90c73560f97bb31c68bf614e2f81d31658d3 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Tue, 3 Oct 2023 19:01:37 +0400 Subject: [PATCH 16/25] Update validation logic --- .../Handlers/Helpers/CmdArgsSanitizer.cs | 2 +- .../Handlers/Helpers/ProcessHandlerHelper.cs | 55 ++------------- .../Worker/Handlers/ProcessHandlerHelperL0.cs | 16 ++--- .../ProcessHandlerHelperTelemetryL0.cs | 67 +++++-------------- 4 files changed, 31 insertions(+), 109 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs index 696d6020e4..c127cfa63b 100644 --- a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -23,7 +23,7 @@ public static (string, CmdArgsSanitizingTelemetry) SanitizeArguments(string inpu var argsChunks = inputArgs.Split(argsSplitSymbols); var matchesChunks = new List(); - var saniziteRegExp = new Regex("(? 0 && result[prefixIndex - 1] == escapingSymbol) { - int beforeBeforePrefix = prefixIndex - 2; - if (beforeBeforePrefix < 0 || result[beforeBeforePrefix] != escapingSymbol) - { - telemetry.EscapedVariables++; - } - else - { - telemetry.EscapedEscapingSymbols++; - } - - startIndex++; - - continue; - } - - // We possibly should simplify that part -> if just no close quote, then break - int quoteIndex = result.IndexOf(quote, startIndex); - if (quoteIndex >= 0 && prefixIndex > quoteIndex) - { - int nextQuoteIndex = result.IndexOf(quote, quoteIndex + 1); - if (nextQuoteIndex < 0) - { - telemetry.QuotesNotEnclosed = 1; - break; - } - - startIndex = nextQuoteIndex + 1; - - telemetry.QuottedBlocks++; - - continue; + telemetry.EscapingSymbolBeforeVar++; } int envStartIndex = prefixIndex + envPrefix.Length; @@ -80,24 +49,14 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary) ValidateInputArguments( public class CmdTelemetry { public int FoundPrefixes { get; set; } = 0; - public int QuottedBlocks { get; set; } = 0; public int VariablesExpanded { get; set; } = 0; - public int EscapedVariables { get; set; } = 0; - public int EscapedEscapingSymbols { get; set; } = 0; + public int EscapingSymbolBeforeVar { get; set; } = 0; public int VariablesStartsFromES { get; set; } = 0; - public int BraceSyntaxEntries { get; set; } = 0; public int VariablesWithESInside { get; set; } = 0; public int QuotesNotEnclosed { get; set; } = 0; public int NotClosedEnvSyntaxPosition { get; set; } = 0; @@ -213,12 +169,9 @@ public Dictionary ToDictionary() return new Dictionary { ["foundPrefixes"] = FoundPrefixes, - ["quottedBlocks"] = QuottedBlocks, ["variablesExpanded"] = VariablesExpanded, - ["escapedVariables"] = EscapedVariables, - ["escapedEscapingSymbols"] = EscapedEscapingSymbols, + ["escapedVariables"] = EscapingSymbolBeforeVar, ["variablesStartsFromES"] = VariablesStartsFromES, - ["braceSyntaxEntries"] = BraceSyntaxEntries, ["bariablesWithESInside"] = VariablesWithESInside, ["quotesNotEnclosed"] = QuotesNotEnclosed, ["notClosedBraceSyntaxPosition"] = NotClosedEnvSyntaxPosition, diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 15a7946f00..80fc942094 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -120,10 +120,6 @@ public void SkipsInvalidEnv() [Trait("Category", "Worker.Handlers")] [InlineData("%var")] [InlineData("%someothervar%")] - [InlineData("^%var")] - [InlineData("^%var%")] - [InlineData("^^%var%")] // we'll keep this case as a pessimistic for now. - [InlineData("^^^%var%")] public void TestNoChanges(string input) { var testEnv = new Dictionary @@ -159,8 +155,10 @@ public void WindowsCaseInsensetiveTest() [Theory] [InlineData("%var%", "1 & echo 23")] [InlineData("%var%%", "1 & echo 23")] - [InlineData("%%%var%", "1 & echo 23")] + [InlineData("%%var%", "1 & echo 23")] [InlineData("1 & echo 23", "")] + [InlineData("1 ; whoami", "")] + [InlineData("1 | whoami", "")] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] public void ArgsValidation_Failes(string inputArgs, string envVarValue) @@ -177,11 +175,13 @@ public void ArgsValidation_Failes(string inputArgs, string envVarValue) Assert.False(isValid); } - // TODO: Code smell. Refactor, remove specific logic from here. [Theory] [InlineData("", "")] - [InlineData("%%var%", "1 & whoami")] - [InlineData("%%%%var%", "1 & whoami")] + [InlineData("%", "")] + [InlineData("1 2", "")] + [InlineData("1 %var%", "2")] + [InlineData("1 \"2\"", "")] + [InlineData("%%var%%", "1")] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] public void ArgsValidation_Passes(string inputArgs, string envVarValue) diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index d9fea13cbe..63cf36660a 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -3,56 +3,38 @@ using Xunit; using Agent.Worker.Handlers.Helpers; +using System.Collections.Generic; namespace Test.L0.Worker.Handlers { public sealed class ProcessHandlerHelperTelemetryL0 { - [Fact] - [Trait("Level", "L0")] - [Trait("Category", "Worker.Handlers")] - public void FoundPrefixesTest() - { - string argsLine = "% % %"; - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); - - Assert.Equal(2, resultTelemetry.FoundPrefixes); - } - - [Fact] - [Trait("Level", "L0")] - [Trait("Category", "Worker.Handlers")] - public void NotClosedEnv() - { - string argsLine = "%1"; - - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); - - Assert.Equal(0, resultTelemetry.NotClosedEnvSyntaxPosition); - } - - [Fact] + [Theory] + [InlineData("% % %", 3)] + [InlineData("%var% %", 2)] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] - public void NotClosedEnv2() + public void FoundPrefixesTest(string inputArgs, int expectedCount) { - string argsLine = "\"%\" %"; - - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); + var env = new Dictionary + { + { "var", "test" } + }; + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, env); - Assert.Equal(4, resultTelemetry.NotClosedEnvSyntaxPosition); + Assert.Equal(expectedCount, resultTelemetry.FoundPrefixes); } - [Fact] + [Theory] + [InlineData("%1", 0)] + [InlineData(" %1", 2)] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] - public void NotClosedQuotes() + public void NotClosedEnv(string inputArgs, int expectedPosition) { - string argsLine = "\" %var%"; - - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, new()); - Assert.Equal(1, resultTelemetry.QuotesNotEnclosed); + Assert.Equal(expectedPosition, resultTelemetry.NotClosedEnvSyntaxPosition); } [Fact] @@ -67,19 +49,6 @@ public void NotClosedQuotes_Ignore_if_no_envVar() Assert.Equal(0, resultTelemetry.QuotesNotEnclosed); } - [Fact] - [Trait("Level", "L0")] - [Trait("Category", "Worker.Handlers")] - public void QuotedBlocksCount() - { - // We're ignoring quote blocks where no any env variables - string argsLine = "\"%VAR1%\" \"%VAR2%\" \"3\""; - - var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); - - Assert.Equal(2, resultTelemetry.QuottedBlocks); - } - [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] @@ -89,7 +58,7 @@ public void CountsVariablesStartFromEscSymbol() var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); - Assert.Equal(2, resultTelemetry.VariablesStartsFromES); + Assert.Equal(3, resultTelemetry.VariablesStartsFromES); } } } From 8004fc6a3dd626951bb6f0532a70b954d854c338 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Tue, 3 Oct 2023 19:03:51 +0400 Subject: [PATCH 17/25] Add % to test --- src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs index ae18d3c38f..b676f4c6d3 100644 --- a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs +++ b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs @@ -40,7 +40,7 @@ public void SanitizeTest(string inputArgs, string expectedArgs) [InlineData("1 2")] [InlineData("1 ^; 2")] [InlineData("1 ^; 2 ^&^& 3 ^< ^> ^| ^^")] - [InlineData(", / \\ aA zZ 09 ' \" - = : . * + ? ^")] + [InlineData(", / \\ aA zZ 09 ' \" - = : . * + ? ^ %")] [Trait("Level", "L0")] [Trait("Category", "Worker.Handlers")] public void SanitizeSkipTest(string inputArgs) From c881b5b0c3ce2c22fd5331ecfb15c8cfce2832af Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Thu, 12 Oct 2023 13:09:45 +0400 Subject: [PATCH 18/25] Code cleanup --- src/Agent.Worker/Handlers/ProcessHandler.cs | 3 --- src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs | 1 - 2 files changed, 4 deletions(-) diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index f624d7e45a..28b0002b69 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -1,12 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -using Agent.Sdk.Knob; using Agent.Worker.Handlers.Helpers; using Microsoft.VisualStudio.Services.Agent.Util; -using Microsoft.VisualStudio.Services.Common; using System; -using System.Collections.Generic; using System.IO; using System.Text; using System.Threading.Tasks; diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index 80fc942094..8a99769c74 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -151,7 +151,6 @@ public void WindowsCaseInsensetiveTest() Assert.Equal(expandedArgs, actualArgs); } - // TODO: Code smell. Refactor, remove specific logic from here. [Theory] [InlineData("%var%", "1 & echo 23")] [InlineData("%var%%", "1 & echo 23")] From cd3826070135f4a0f8ff1e3ef5bf593389a0bdc3 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Fri, 13 Oct 2023 23:27:23 +0400 Subject: [PATCH 19/25] Hide new logic under knob --- src/Agent.Sdk/Knob/AgentKnobs.cs | 7 ++ src/Agent.Worker/Handlers/NodeHandler.cs | 1 - src/Agent.Worker/Handlers/ProcessHandler.cs | 129 ++++++++++++++++---- 3 files changed, 111 insertions(+), 26 deletions(-) diff --git a/src/Agent.Sdk/Knob/AgentKnobs.cs b/src/Agent.Sdk/Knob/AgentKnobs.cs index cbe5fdab95..b0dfa5f248 100644 --- a/src/Agent.Sdk/Knob/AgentKnobs.cs +++ b/src/Agent.Sdk/Knob/AgentKnobs.cs @@ -478,6 +478,13 @@ public class AgentKnobs new RuntimeKnobSource("AZP_75787_ENABLE_COLLECT"), new EnvironmentKnobSource("AZP_75787_ENABLE_COLLECT"), new BuiltInDefaultKnobSource("false")); + + public static readonly Knob ProcessHandlerEnableNewLogic = new Knob( + nameof(ProcessHandlerEnableNewLogic), + "Enables new sanitization logic for process handler", + new RuntimeKnobSource("AZP_75787_ENABLE_NEW_PH_LOGIC"), + new EnvironmentKnobSource("AZP_75787_ENABLE_NEW_PH_LOGIC"), + new BuiltInDefaultKnobSource("false")); public static readonly Knob DisableDrainQueuesAfterTask = new Knob( nameof(DisableDrainQueuesAfterTask), diff --git a/src/Agent.Worker/Handlers/NodeHandler.cs b/src/Agent.Worker/Handlers/NodeHandler.cs index 1e64043662..abcea4acd9 100644 --- a/src/Agent.Worker/Handlers/NodeHandler.cs +++ b/src/Agent.Worker/Handlers/NodeHandler.cs @@ -156,7 +156,6 @@ public async Task RunAsync() { Trace.Info("AZP_AGENT_IGNORE_VSTSTASKLIB enabled, ignoring fix"); } - StepHost.OutputDataReceived += OnDataReceived; StepHost.ErrorDataReceived += OnDataReceived; diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 28b0002b69..233bdf1856 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -1,8 +1,10 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using Agent.Sdk.Knob; using Agent.Worker.Handlers.Helpers; using Microsoft.VisualStudio.Services.Agent.Util; +using Newtonsoft.Json; using System; using System.IO; using System.Text; @@ -24,6 +26,7 @@ public sealed class ProcessHandler : Handler, IProcessHandler private volatile int _errorCount; private bool _foundDelimiter; private bool _modifyEnvironment; + private string _generatedScriptPath; public ProcessHandlerData Data { get; set; } @@ -60,12 +63,12 @@ public async Task RunAsync() Trace.Info($"Command is rooted: {isCommandRooted}"); - var disableInlineExecution = StringUtil.ConvertToBoolean(Data.DisableInlineExecution); + bool disableInlineExecution = StringUtil.ConvertToBoolean(Data.DisableInlineExecution); ExecutionContext.Debug($"Disable inline execution: '{disableInlineExecution}'"); if (disableInlineExecution && !File.Exists(command)) { - throw new Exception(StringUtil.Loc("FileNotFound", command)); + throw new FileNotFoundException(StringUtil.Loc("FileNotFound", command)); } // Determine the working directory. @@ -129,39 +132,72 @@ public async Task RunAsync() cmdExe = "cmd.exe"; } - try + bool enableSecureArguments = AgentKnobs.ProcessHandlerSecureArguments.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable secure arguments: '{enableSecureArguments}'"); + bool enableNewPHLogic = AgentKnobs.ProcessHandlerEnableNewLogic.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable new PH sanitizing logic: '{enableNewPHLogic}'"); + + bool enableFileArgs = disableInlineExecution && enableSecureArguments && !enableNewPHLogic; + if (enableFileArgs) { - var (isValid, telemetry) = ProcessHandlerHelper.ValidateInputArguments(arguments, Environment, ExecutionContext); + bool enableSecureArgumentsAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable secure arguments audit: '{enableSecureArgumentsAudit}'"); + bool enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable telemetry: '{enableTelemetry}'"); - if (telemetry != null) + if ((disableInlineExecution && (enableSecureArgumentsAudit || enableSecureArguments)) || enableTelemetry) { - PublishTelemetry(telemetry, "ProcessHandler"); - } - if (!isValid) - { - throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + var (processedArgs, telemetry) = ProcessHandlerHelper.ExpandCmdEnv(arguments, Environment); + + if (disableInlineExecution && enableSecureArgumentsAudit) + { + ExecutionContext.Warning($"The following arguments will be executed: '{processedArgs}'"); + } + if (enableFileArgs) + { + GenerateScriptFile(cmdExe, command, processedArgs); + } + if (enableTelemetry) + { + ExecutionContext.Debug($"Agent PH telemetry: {JsonConvert.SerializeObject(telemetry.ToDictionary(), Formatting.None)}"); + PublishTelemetry(telemetry.ToDictionary(), "ProcessHandler"); + } } } - catch (ArgsSanitizedException) - { - throw; - } - catch (Exception ex) + else if (enableNewPHLogic) { - Trace.Error($"Failed to validate process handler input arguments. Publishing telemetry. Ex: {ex}"); + try + { + var (isValid, telemetry) = ProcessHandlerHelper.ValidateInputArguments(arguments, Environment, ExecutionContext); - var telemetry = new + if (telemetry != null) + { + PublishTelemetry(telemetry, "ProcessHandler"); + } + if (!isValid) + { + throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } + } + catch (ArgsSanitizedException) + { + throw; + } + catch (Exception ex) { - UnexpectedError = ex.Message, - ErrorStackTrace = ex.StackTrace - }; - PublishTelemetry(telemetry, "ProcessHandler"); + Trace.Error($"Failed to validate process handler input arguments. Publishing telemetry. Ex: {ex}"); + + var telemetry = new + { + UnexpectedError = ex.Message, + ErrorStackTrace = ex.StackTrace + }; + PublishTelemetry(telemetry, "ProcessHandler"); + } + } - string cmdExeArgs = $"/c \"{command} {arguments}"; - cmdExeArgs += _modifyEnvironment - ? $" && echo {OutputDelimiter} && set \"" - : "\""; + string cmdExeArgs = PrepareCmdExeArgs(command, arguments, enableFileArgs); // Invoke the process. ExecutionContext.Debug($"{cmdExe} {cmdExeArgs}"); @@ -212,6 +248,49 @@ public async Task RunAsync() } } + private string PrepareCmdExeArgs(string command, string arguments, bool enableFileArgs) + { + string cmdExeArgs; + if (enableFileArgs) + { + cmdExeArgs = $"/c \"{_generatedScriptPath}\""; + } + else + { + // Format the input to be invoked from cmd.exe to enable built-in shell commands. For example, RMDIR. + cmdExeArgs = $"/c \"{command} {arguments}"; + cmdExeArgs += _modifyEnvironment + ? $" && echo {OutputDelimiter} && set \"" + : "\""; + } + + return cmdExeArgs; + } + + private void GenerateScriptFile(string cmdExe, string command, string arguments) + { + var scriptId = Guid.NewGuid().ToString(); + string inputArgsEnvVarName = VarUtil.ConvertToEnvVariableFormat("AGENT_PH_ARGS_" + scriptId[..8]); + + System.Environment.SetEnvironmentVariable(inputArgsEnvVarName, arguments); + + var agentTemp = ExecutionContext.GetVariableValueOrDefault(Constants.Variables.Agent.TempDirectory); + _generatedScriptPath = Path.Combine(agentTemp, $"processHandlerScript_{scriptId}.cmd"); + + var scriptArgs = $"/v:ON /c \"{command} !{inputArgsEnvVarName}!"; + + scriptArgs += _modifyEnvironment + ? $" && echo {OutputDelimiter} && set \"" + : "\""; + + using (var writer = new StreamWriter(_generatedScriptPath)) + { + writer.WriteLine($"{cmdExe} {scriptArgs}"); + } + + ExecutionContext.Debug($"Generated script file: {_generatedScriptPath}"); + } + private void FlushErrorData() { if (_errorBuffer.Length > 0) From bdabbd99bd2141be785f2d0ab4d5e2f2cc483b7c Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Mon, 16 Oct 2023 14:57:20 +0400 Subject: [PATCH 20/25] Move constants to class --- src/Agent.Sdk/Knob/AgentKnobs.cs | 2 +- .../Handlers/Helpers/CmdArgsSanitizer.cs | 17 ++++++++-------- .../Handlers/Helpers/ProcessHandlerHelper.cs | 20 +++++++++---------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/Agent.Sdk/Knob/AgentKnobs.cs b/src/Agent.Sdk/Knob/AgentKnobs.cs index b0dfa5f248..f20543542b 100644 --- a/src/Agent.Sdk/Knob/AgentKnobs.cs +++ b/src/Agent.Sdk/Knob/AgentKnobs.cs @@ -478,7 +478,7 @@ public class AgentKnobs new RuntimeKnobSource("AZP_75787_ENABLE_COLLECT"), new EnvironmentKnobSource("AZP_75787_ENABLE_COLLECT"), new BuiltInDefaultKnobSource("false")); - + public static readonly Knob ProcessHandlerEnableNewLogic = new Knob( nameof(ProcessHandlerEnableNewLogic), "Enables new sanitization logic for process handler", diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs index c127cfa63b..f83bee583a 100644 --- a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -10,6 +10,10 @@ namespace Agent.Worker.Handlers.Helpers { public static class CmdArgsSanitizer { + private const string _RemovedSymbolSign = "_#removed_d8c0672b#_"; + private const string _ArgsSplitSymbols = "^^"; + private static readonly Regex _SanitizeRegExp = new("(?(); - var saniziteRegExp = new Regex("(? 0) { matchesChunks.Add(matches); - argsChunks[i] = saniziteRegExp.Replace(argsChunks[i], removedSymbolSign); + argsChunks[i] = _SanitizeRegExp.Replace(argsChunks[i], _RemovedSymbolSign); } } - var resultArgs = string.Join(argsSplitSymbols, argsChunks); + var resultArgs = string.Join(_ArgsSplitSymbols, argsChunks); CmdArgsSanitizingTelemetry telemetry = null; diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index e0591f2ecc..2288b08477 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -12,22 +12,22 @@ namespace Agent.Worker.Handlers.Helpers { public static class ProcessHandlerHelper { + private const char _EscapingSymbol = '^'; + private const string _EnvPrefix = "%"; + private const string _EnvPostfix = "%"; + public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary environment) { ArgUtil.NotNull(inputArgs, nameof(inputArgs)); ArgUtil.NotNull(environment, nameof(environment)); - const char escapingSymbol = '^'; - const string envPrefix = "%"; - const string envPostfix = "%"; - string result = inputArgs; int startIndex = 0; var telemetry = new CmdTelemetry(); while (true) { - int prefixIndex = result.IndexOf(envPrefix, startIndex); + int prefixIndex = result.IndexOf(_EnvPrefix, startIndex); if (prefixIndex < 0) { break; @@ -35,12 +35,12 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary 0 && result[prefixIndex - 1] == escapingSymbol) + if (prefixIndex > 0 && result[prefixIndex - 1] == _EscapingSymbol) { telemetry.EscapingSymbolBeforeVar++; } - int envStartIndex = prefixIndex + envPrefix.Length; + int envStartIndex = prefixIndex + _EnvPrefix.Length; int envEndIndex = FindEnclosingIndex(result, prefixIndex); if (envEndIndex == 0) { @@ -49,13 +49,13 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary Date: Mon, 16 Oct 2023 16:02:20 +0400 Subject: [PATCH 21/25] move const value back --- src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs index f83bee583a..d01c9e6f3c 100644 --- a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -10,7 +10,7 @@ namespace Agent.Worker.Handlers.Helpers { public static class CmdArgsSanitizer { - private const string _RemovedSymbolSign = "_#removed_d8c0672b#_"; + private const string _RemovedSymbolSign = "_#removed#_"; private const string _ArgsSplitSymbols = "^^"; private static readonly Regex _SanitizeRegExp = new("(? Date: Mon, 16 Oct 2023 19:51:15 +0400 Subject: [PATCH 22/25] Add PublishTelemetry overload --- src/Agent.Worker/Handlers/Handler.cs | 12 +++++++++++- src/Agent.Worker/Handlers/ProcessHandler.cs | 7 ++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Agent.Worker/Handlers/Handler.cs b/src/Agent.Worker/Handlers/Handler.cs index 8c73d78c4f..ccacef12e0 100644 --- a/src/Agent.Worker/Handlers/Handler.cs +++ b/src/Agent.Worker/Handlers/Handler.cs @@ -308,7 +308,17 @@ protected void RemovePSModulePathFromEnvironment() } } - protected void PublishTelemetry( + // This overload is to handle specific types some other way. + protected void PublishTelemetry( + Dictionary telemetryData, + string feature = "TaskHandler" + ) + { + // JsonConvert.SerializeObject always converts to base object. + PublishTelemetry((object)telemetryData, feature); + } + + private void PublishTelemetry( object telemetryData, string feature = "TaskHandler" ) diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 233bdf1856..d572657180 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -6,6 +6,7 @@ using Microsoft.VisualStudio.Services.Agent.Util; using Newtonsoft.Json; using System; +using System.Collections.Generic; using System.IO; using System.Text; using System.Threading.Tasks; @@ -187,10 +188,10 @@ public async Task RunAsync() { Trace.Error($"Failed to validate process handler input arguments. Publishing telemetry. Ex: {ex}"); - var telemetry = new + var telemetry = new Dictionary { - UnexpectedError = ex.Message, - ErrorStackTrace = ex.StackTrace + ["UnexpectedError"] = ex.Message, + ["ErrorStackTrace"] = ex.StackTrace }; PublishTelemetry(telemetry, "ProcessHandler"); } From fc08d0ba115189d22d0cdff89b6bf8469eb523ad Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Mon, 16 Oct 2023 20:13:18 +0400 Subject: [PATCH 23/25] Update throw logic --- src/Agent.Worker/Handlers/ProcessHandler.cs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index d572657180..7cc18ae62c 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -167,22 +167,18 @@ public async Task RunAsync() } else if (enableNewPHLogic) { + bool shouldThrow = false; try { var (isValid, telemetry) = ProcessHandlerHelper.ValidateInputArguments(arguments, Environment, ExecutionContext); + // If args are not valid - we'll throw exception. + shouldThrow = !isValid; + if (telemetry != null) { PublishTelemetry(telemetry, "ProcessHandler"); } - if (!isValid) - { - throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); - } - } - catch (ArgsSanitizedException) - { - throw; } catch (Exception ex) { @@ -194,8 +190,13 @@ public async Task RunAsync() ["ErrorStackTrace"] = ex.StackTrace }; PublishTelemetry(telemetry, "ProcessHandler"); - } + shouldThrow = false; + } + if (shouldThrow) + { + throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + } } string cmdExeArgs = PrepareCmdExeArgs(command, arguments, enableFileArgs); From d66169c742d0fdc5a5ffda6e02a5c7adf90288e0 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Mon, 16 Oct 2023 20:21:53 +0400 Subject: [PATCH 24/25] Update invalid args exception --- src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs | 3 ++- src/Agent.Worker/Handlers/ProcessHandler.cs | 6 +++--- src/Misc/layoutbin/en-US/strings.json | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 2288b08477..30dd642d4f 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -133,7 +133,7 @@ public static (bool, Dictionary) ValidateInputArguments( { if (enableAudit && !enableValidation) { - context.Warning(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + context.Warning(StringUtil.Loc("ProcessHandlerInvalidScriptArgs")); } if (enableValidation) { @@ -153,6 +153,7 @@ public static (bool, Dictionary) ValidateInputArguments( } } } + public class CmdTelemetry { public int FoundPrefixes { get; set; } = 0; diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 7cc18ae62c..0bb9d02236 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -195,7 +195,7 @@ public async Task RunAsync() } if (shouldThrow) { - throw new ArgsSanitizedException(StringUtil.Loc("ProcessHandlerScriptArgsSanitized")); + throw new InvalidScriptArgsException(StringUtil.Loc("ProcessHandlerInvalidScriptArgs")); } } @@ -366,9 +366,9 @@ private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs e) } } - public class ArgsSanitizedException : Exception + public class InvalidScriptArgsException : Exception { - public ArgsSanitizedException(string message) : base(message) + public InvalidScriptArgsException(string message) : base(message) { } } diff --git a/src/Misc/layoutbin/en-US/strings.json b/src/Misc/layoutbin/en-US/strings.json index 6851c08544..d7aa27fb80 100644 --- a/src/Misc/layoutbin/en-US/strings.json +++ b/src/Misc/layoutbin/en-US/strings.json @@ -461,7 +461,7 @@ "ProcessCompletedWithCode0Errors1": "Process completed with exit code {0} and had {1} error(s) written to the error stream.", "ProcessCompletedWithExitCode0": "Process completed with exit code {0}.", "ProcessExitCode": "Exit code {0} returned from process: file name '{1}', arguments '{2}'.", - "ProcessHandlerScriptArgsSanitized": "Detected characters in arguments that may not be executed correctly by the shell. Please escape special characters using circumflex (^). More information is available here: https://aka.ms/ado/75787", + "ProcessHandlerInvalidScriptArgs": "Detected characters in arguments that may not be executed correctly by the shell. More information is available here: https://aka.ms/ado/75787", "ProfileLoadFailure": "Unable to load the user profile for the user {0}\\{1} AutoLogon configuration is not possible.", "ProjectName": "Project name", "Prompt0": "Enter {0}", From 37edc1a6ed730dd34ef228468a0b02b192a6fe61 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov Date: Mon, 16 Oct 2023 20:26:51 +0400 Subject: [PATCH 25/25] Update private const letter case --- .../Handlers/Helpers/CmdArgsSanitizer.cs | 14 +++++++------- .../Handlers/Helpers/ProcessHandlerHelper.cs | 18 +++++++++--------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs index d01c9e6f3c..43c1ed4a5b 100644 --- a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -10,9 +10,9 @@ namespace Agent.Worker.Handlers.Helpers { public static class CmdArgsSanitizer { - private const string _RemovedSymbolSign = "_#removed#_"; - private const string _ArgsSplitSymbols = "^^"; - private static readonly Regex _SanitizeRegExp = new("(?(); for (int i = 0; i < argsChunks.Length; i++) { - var matches = _SanitizeRegExp.Matches(argsChunks[i]); + var matches = _sanitizeRegExp.Matches(argsChunks[i]); if (matches.Count > 0) { matchesChunks.Add(matches); - argsChunks[i] = _SanitizeRegExp.Replace(argsChunks[i], _RemovedSymbolSign); + argsChunks[i] = _sanitizeRegExp.Replace(argsChunks[i], _removedSymbolSign); } } - var resultArgs = string.Join(_ArgsSplitSymbols, argsChunks); + var resultArgs = string.Join(_argsSplitSymbols, argsChunks); CmdArgsSanitizingTelemetry telemetry = null; diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 30dd642d4f..a7f784ca5d 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -12,9 +12,9 @@ namespace Agent.Worker.Handlers.Helpers { public static class ProcessHandlerHelper { - private const char _EscapingSymbol = '^'; - private const string _EnvPrefix = "%"; - private const string _EnvPostfix = "%"; + private const char _escapingSymbol = '^'; + private const string _envPrefix = "%"; + private const string _envPostfix = "%"; public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary environment) { @@ -27,7 +27,7 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary 0 && result[prefixIndex - 1] == _EscapingSymbol) + if (prefixIndex > 0 && result[prefixIndex - 1] == _escapingSymbol) { telemetry.EscapingSymbolBeforeVar++; } - int envStartIndex = prefixIndex + _EnvPrefix.Length; + int envStartIndex = prefixIndex + _envPrefix.Length; int envEndIndex = FindEnclosingIndex(result, prefixIndex); if (envEndIndex == 0) { @@ -49,13 +49,13 @@ public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary