Fix execution of scripts in variables (#4016)

* Update vars expanding * Pass task name to expanding function * Add check in RecalculateExpanded for target vars * Refactor vars expanding impl * Correct cmdline postfix * Fix recursive matching of next variables * Add check for script tasks * Move & rename lists to constants * Rename dict with variables and add description * Add test for non-expanding of vulnerable vars * Add tests for varutil * Add new tests for variables expanding * Make prefixes suffixes maps per shell * Add release environment variables * Fix enum naming * Update variables expanding for cmd shell * Add & update tests for cmd * Add test case for cmd * Use ConvertToEnvVariableFormat util * Shorten variables names * Add tracing logs * Ignore case for task & variable names * Remove unused import * Fix test with ignoring case * Add new test cases * Fix grammar * Add & update tracing logs * Change cmd replacing to regex * Move shell var constructs to separate const class * Move suffixes and prefixes to structure * Add test to check if wrong task version specified
microsoft · Nov 24, 2022 · af5328b · af5328b
1 parent 321c73b
commit af5328b
Show file tree

Hide file tree

Showing 8 changed files with 399 additions and 39 deletions.
diff --git a/src/Agent.Worker/TaskRunner.cs b/src/Agent.Worker/TaskRunner.cs
@@ -219,7 +219,7 @@ public async Task RunAsync()
 
                 // Expand the inputs.
                 Trace.Verbose("Expanding inputs.");
-                runtimeVariables.ExpandValues(target: inputs);
+                runtimeVariables.ExpandValues(target: inputs, Task.Reference.Name);
 
                 // We need to verify inputs of the tasks that were injected by decorators, to check if they contain secrets,
                 // for security reasons execution of tasks in this case should be skipped.

diff --git a/src/Agent.Worker/Variables.cs b/src/Agent.Worker/Variables.cs
@@ -10,7 +10,6 @@
 using System.Collections.Generic;
 using System.Linq;
 using BuildWebApi = Microsoft.TeamFoundation.Build.WebApi;
-using Microsoft.TeamFoundation.DistributedTask.Logging;
 using Newtonsoft.Json.Linq;
 using Agent.Sdk.Util;
 
@@ -260,22 +259,7 @@ public bool Retain_Default_Encoding
             "RequestedFor"
         };
 
-        public static readonly List<string> VariablesVulnerableToExecution = new List<string>
-        {
-            Constants.Variables.Build.SourceVersionMessage,
-            Constants.Variables.Build.DefinitionName,
-            Constants.Variables.System.SourceVersionMessage,
-            Constants.Variables.System.DefinitionName,
-            Constants.Variables.System.JobDisplayName,
-            Constants.Variables.System.PhaseDisplayName,
-            Constants.Variables.System.StageDisplayName,
-            Constants.Variables.Release.ReleaseDefinitionName,
-            Constants.Variables.Release.ReleaseEnvironmentName,
-            Constants.Variables.Agent.MachineName,
-            Constants.Variables.Agent.Name,
-        };
-
-        public void ExpandValues(IDictionary<string, string> target)
+        public void ExpandValues(IDictionary<string, string> target, string taskName = null)
         {
             ArgUtil.NotNull(target, nameof(target));
             _trace.Entering();
@@ -286,7 +270,7 @@ public void ExpandValues(IDictionary<string, string> target)
                 source[variable.Name] = value;
             }
 
-            VarUtil.ExpandValues(_hostContext, source, target);
+            VarUtil.ExpandValues(_hostContext, source, target, taskName);
         }
 
         public string ExpandValue(string name, string value)
@@ -398,7 +382,7 @@ public void Unset(string name)
             lock (_setLock)
             {
                 Variable dummy;
-                 _expanded.Remove(name, out dummy);
+                _expanded.Remove(name, out dummy);
                 _nonexpanded.Remove(name, out dummy);
                 _trace.Verbose($"Unset '{name}'");
             }
@@ -447,7 +431,8 @@ public void Set(string name, string val, bool secret = false, bool readOnly = fa
         public bool IsReadOnly(string name)
         {
             Variable existingVariable = null;
-            if (!_expanded.TryGetValue(name, out existingVariable)) {
+            if (!_expanded.TryGetValue(name, out existingVariable))
+            {
                 _nonexpanded.TryGetValue(name, out existingVariable);
             }
 
@@ -487,6 +472,12 @@ public void RecalculateExpanded(out List<string> warnings)
                 // Process each variable in the dictionary.
                 foreach (string name in _nonexpanded.Keys)
                 {
+                    if (Constants.Variables.VariablesVulnerableToExecution.Contains(name, StringComparer.OrdinalIgnoreCase))
+                    {
+                        _trace.Verbose($"Skipping expansion for variable: '{name}'");
+                        continue;
+                    }
+
                     bool secret = _nonexpanded[name].Secret;
                     bool readOnly = _nonexpanded[name].ReadOnly;
                     _trace.Verbose($"Processing expansion for variable: '{name}'");

diff --git a/src/Agent.Worker/WorkerUtilties.cs b/src/Agent.Worker/WorkerUtilties.cs
@@ -105,7 +105,7 @@ public static Pipelines.AgentJobRequestMessage DeactivateVsoCommandsFromJobMessa
 
             var deactivatedVariables = new Dictionary<string, VariableValue>(message.Variables, StringComparer.OrdinalIgnoreCase);
 
-            foreach (var variableName in Variables.VariablesVulnerableToExecution)
+            foreach (var variableName in Constants.Variables.VariablesVulnerableToExecution)
             {
                 if (deactivatedVariables.TryGetValue(variableName, out var variable))
                 {

diff --git a/src/Microsoft.VisualStudio.Services.Agent/Constants.cs b/src/Microsoft.VisualStudio.Services.Agent/Constants.cs
@@ -41,6 +41,13 @@ public enum WellKnownConfigFile
         TaskExceptionList // We need to remove this config file - once Node 6 handler is dropped
     }
 
+    public enum WellKnownScriptShell
+    {
+        Bash,
+        Cmd,
+        PowerShell
+    }
+
     public static class Constants
     {
         /// <summary>Name of environment variable holding the path.</summary>
@@ -272,6 +279,26 @@ public static class Variables
             public static readonly string MacroPrefix = "$(";
             public static readonly string MacroSuffix = ")";
 
+            /// <summary>
+            /// These variables potentially may be used to execute scripts without the knowledge of the owner of the pipelines.
+            /// We want to prevent this by not expanding them and replacing these variables in user scripts with environment variables.
+            /// Note that the replacement will only take place for inline scripts.
+            /// </summary>
+            public static readonly List<string> VariablesVulnerableToExecution = new List<string>
+            {
+                Agent.MachineName,
+                Agent.Name,
+                Build.DefinitionName,
+                Build.SourceVersionMessage,
+                Release.ReleaseDefinitionName,
+                Release.ReleaseEnvironmentName,
+                System.DefinitionName,
+                System.JobDisplayName,
+                System.PhaseDisplayName,
+                System.SourceVersionMessage,
+                System.StageDisplayName
+            };
+
             public static class Agent
             {
                 //
@@ -460,6 +487,13 @@ public static class Task
                 public static readonly string SkipTranslatorForCheckout = "task.skipTranslatorForCheckout";
             }
 
+            public static readonly Dictionary<string, WellKnownScriptShell> ScriptShellsPerTasks = new Dictionary<string, WellKnownScriptShell>(StringComparer.OrdinalIgnoreCase)
+            {
+                ["PowerShell"] = WellKnownScriptShell.PowerShell,
+                ["Bash"] = WellKnownScriptShell.Bash,
+                ["CmdLine"] = WellKnownScriptShell.Cmd
+            };
+
             public static List<string> ReadOnlyVariables = new List<string>(){
                 // Agent variables
                 Agent.AcceptTeeEula,
@@ -588,5 +622,15 @@ public static class Task
                 Task.SkipTranslatorForCheckout
             };
         }
+
+        public static class ScriptShells
+        {
+            public static Dictionary<WellKnownScriptShell, EnvVariableParts> EnvVariablePartsPerShell = new Dictionary<WellKnownScriptShell, EnvVariableParts>
+            {
+                [WellKnownScriptShell.PowerShell] = new EnvVariableParts { Prefix = "$env:", Suffix = "" },
+                [WellKnownScriptShell.Bash] = new EnvVariableParts { Prefix = "$", Suffix = "" },
+                [WellKnownScriptShell.Cmd] = new EnvVariableParts { Prefix = "%", Suffix = "" }
+            };
+        }
     }
 }
diff --git a/src/Microsoft.VisualStudio.Services.Agent/EnvVariableParts.cs b/src/Microsoft.VisualStudio.Services.Agent/EnvVariableParts.cs
@@ -0,0 +1,11 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+namespace Microsoft.VisualStudio.Services.Agent
+{
+    public class EnvVariableParts
+    {
+        public string Prefix { get; set; }
+        public string Suffix { get; set; }
+    }
+}
diff --git a/src/Microsoft.VisualStudio.Services.Agent/Util/VarUtil.cs b/src/Microsoft.VisualStudio.Services.Agent/Util/VarUtil.cs
@@ -9,10 +9,11 @@
 using System.Runtime.InteropServices;
 using Microsoft.VisualStudio.Services.WebApi;
 using Newtonsoft.Json.Linq;
+using System.Text.RegularExpressions;
 
 namespace Microsoft.VisualStudio.Services.Agent.Util
 {
-  public static class VarUtil
+    public static class VarUtil
     {
         public static StringComparer EnvironmentVariableKeyComparer
         {
@@ -107,6 +108,7 @@ public static void ExpandEnvironmentVariables(IHostContext context, IDictionary<
             // Copy the environment variables into a dictionary that uses the correct comparer.
             var source = new Dictionary<string, string>(EnvironmentVariableKeyComparer);
             IDictionary environment = Environment.GetEnvironmentVariables();
+
             foreach (DictionaryEntry entry in environment)
             {
                 string key = entry.Key as string ?? string.Empty;
@@ -140,13 +142,18 @@ public static JToken ExpandValues(IHostContext context, IDictionary<string, stri
             return target.Map(mapFuncs);
         }
 
-        public static void ExpandValues(IHostContext context, IDictionary<string, string> source, IDictionary<string, string> target)
+        public static void ExpandValues(
+            IHostContext context,
+            IDictionary<string, string> source,
+            IDictionary<string, string> target,
+            string taskName = null
+            )
         {
             ArgUtil.NotNull(context, nameof(context));
             ArgUtil.NotNull(source, nameof(source));
             Tracing trace = context.GetTrace(nameof(VarUtil));
             trace.Entering();
-            target = target ?? new Dictionary<string, string>();
+            target ??= new Dictionary<string, string>();
 
             // This algorithm does not perform recursive replacement.
 
@@ -169,17 +176,51 @@ public static void ExpandValues(IHostContext context, IDictionary<string, string
                         startIndex: prefixIndex + Constants.Variables.MacroPrefix.Length,
                         length: suffixIndex - prefixIndex - Constants.Variables.MacroPrefix.Length);
                     trace.Verbose($"Found macro candidate: '{variableKey}'");
-                    string variableValue;
-                    if (!string.IsNullOrEmpty(variableKey) &&
-                        TryGetValue(trace, source, variableKey, out variableValue))
+
+                    var isVariableKeyPresent = !string.IsNullOrEmpty(variableKey);
+                    WellKnownScriptShell shellName;
+
+                    if (isVariableKeyPresent &&
+                        !string.IsNullOrEmpty(taskName) &&
+                        Constants.Variables.ScriptShellsPerTasks.TryGetValue(taskName, out shellName) &&
+                        shellName != WellKnownScriptShell.Cmd &&
+                        Constants.Variables.VariablesVulnerableToExecution.Contains(variableKey, StringComparer.OrdinalIgnoreCase))
+                    {
+                        trace.Verbose($"Found a macro with vulnerable variables. Replacing with env variables for the {shellName} shell.");
+
+                        var envVariableParts = Constants.ScriptShells.EnvVariablePartsPerShell[shellName];
+                        var envVariableName = ConvertToEnvVariableFormat(variableKey);
+                        var envVariable = envVariableParts.Prefix + envVariableName + envVariableParts.Suffix;
+
+                        targetValue =
+                            targetValue[..prefixIndex]
+                            + envVariable
+                            + targetValue[(suffixIndex + Constants.Variables.MacroSuffix.Length)..];
+
+                        startIndex = prefixIndex + envVariable.Length;
+                    }
+                    else if (isVariableKeyPresent &&
+                        TryGetValue(trace, source, variableKey, out string variableValue))
                     {
                         // A matching variable was found.
                         // Update the target value.
                         trace.Verbose("Macro found.");
+
+                        if (!string.IsNullOrEmpty(taskName) &&
+                            Constants.Variables.ScriptShellsPerTasks.TryGetValue(taskName, out shellName) &&
+                            shellName == WellKnownScriptShell.Cmd)
+                        {
+                            trace.Verbose("CMD shell found. Custom macro processing.");
+
+                            // When matching "&", "|", "<" and ">" cmd commands adds "^" before them.
+                            var cmdCommandsRegex = new Regex(@"[&|\||<|>]");
+                            variableValue = cmdCommandsRegex.Replace(variableValue, "^$&");
+                        }
+
                         targetValue = string.Concat(
-                            targetValue.Substring(0, prefixIndex),
-                            variableValue ?? string.Empty,
-                            targetValue.Substring(suffixIndex + Constants.Variables.MacroSuffix.Length));
+                            targetValue[..prefixIndex],
+                            variableValue,
+                            targetValue[(suffixIndex + Constants.Variables.MacroSuffix.Length)..]);
 
                         // Bump the start index to prevent recursive replacement.
                         startIndex = prefixIndex + (variableValue ?? string.Empty).Length;