From ec5d72810f2adbeb051792e79d088c0ddda4ae41 Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Wed, 5 Apr 2023 08:27:23 +0000 Subject: [PATCH] Omit Base64 padding characters from Base64 representations of secrets. --- src/Runner.Common/HostContext.cs | 27 ++-- .../Expressions2/Sdk/ExpressionNode.cs | 4 +- src/Sdk/DTLogging/Logging/ISecretMasker.cs | 1 - src/Sdk/DTLogging/Logging/SecretMasker.cs | 81 +---------- src/Sdk/DTLogging/Logging/ValueEncoders.cs | 127 ++++++++++-------- src/Test/L0/HostContextL0.cs | 8 +- src/Test/L0/TestHostContext.cs | 9 +- 7 files changed, 102 insertions(+), 155 deletions(-) diff --git a/src/Runner.Common/HostContext.cs b/src/Runner.Common/HostContext.cs index 4d9ff24c5..d49160aa4 100644 --- a/src/Runner.Common/HostContext.cs +++ b/src/Runner.Common/HostContext.cs @@ -53,7 +53,7 @@ namespace GitHub.Runner.Common private static int[] _vssHttpCredentialEventIds = new int[] { 11, 13, 14, 15, 16, 17, 18, 20, 21, 22, 27, 29 }; private readonly ConcurrentDictionary _serviceInstances = new(); private readonly ConcurrentDictionary _serviceTypes = new(); - private readonly ISecretMasker _secretMasker = new SecretMasker(); + private readonly ISecretMasker _secretMasker; private readonly List _userAgents = new() { new ProductInfoHeaderValue($"GitHubActionsRunner-{BuildConstants.RunnerPackage.PackageName}", BuildConstants.RunnerPackage.Version) }; private CancellationTokenSource _runnerShutdownTokenSource = new(); private object _perfLock = new(); @@ -82,17 +82,20 @@ namespace GitHub.Runner.Common _loadContext = AssemblyLoadContext.GetLoadContext(typeof(HostContext).GetTypeInfo().Assembly); _loadContext.Unloading += LoadContext_Unloading; - this.SecretMasker.AddValueEncoder(ValueEncoders.Base64StringEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.Base64StringEscapeShift1); - this.SecretMasker.AddValueEncoder(ValueEncoders.Base64StringEscapeShift2); - this.SecretMasker.AddValueEncoder(ValueEncoders.CommandLineArgumentEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.ExpressionStringEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.JsonStringEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.UriDataEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.XmlDataEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.TrimDoubleQuotes); - this.SecretMasker.AddValueEncoder(ValueEncoders.PowerShellPreAmpersandEscape); - this.SecretMasker.AddValueEncoder(ValueEncoders.PowerShellPostAmpersandEscape); + var masks = new List() + { + ValueEncoders.EnumerateBase64Variations, + ValueEncoders.CommandLineArgumentEscape, + ValueEncoders.ExpressionStringEscape, + ValueEncoders.JsonStringEscape, + ValueEncoders.UriDataEscape, + ValueEncoders.XmlDataEscape, + ValueEncoders.TrimDoubleQuotes, + ValueEncoders.PowerShellPreAmpersandEscape, + ValueEncoders.PowerShellPostAmpersandEscape + }; + _secretMasker = new SecretMasker(masks); + // Create StdoutTraceListener if ENV is set StdoutTraceListener stdoutTraceListener = null; diff --git a/src/Sdk/DTExpressions2/Expressions2/Sdk/ExpressionNode.cs b/src/Sdk/DTExpressions2/Expressions2/Sdk/ExpressionNode.cs index 28e7b37ec..39f8b21e3 100644 --- a/src/Sdk/DTExpressions2/Expressions2/Sdk/ExpressionNode.cs +++ b/src/Sdk/DTExpressions2/Expressions2/Sdk/ExpressionNode.cs @@ -1,6 +1,6 @@ using System; -using System.Collections.Generic; using System.ComponentModel; +using System.Linq; using GitHub.DistributedTask.Logging; namespace GitHub.DistributedTask.Expressions2.Sdk @@ -55,7 +55,7 @@ namespace GitHub.DistributedTask.Expressions2.Sdk try { // Evaluate - secretMasker = secretMasker?.Clone() ?? new SecretMasker(); + secretMasker = secretMasker?.Clone() ?? new SecretMasker(Enumerable.Empty()); trace = new EvaluationTraceWriter(trace, secretMasker); var context = new EvaluationContext(trace, secretMasker, state, options, this); trace.Info($"Evaluating: {ConvertToExpression()}"); diff --git a/src/Sdk/DTLogging/Logging/ISecretMasker.cs b/src/Sdk/DTLogging/Logging/ISecretMasker.cs index eec40bef0..cfc8214d7 100644 --- a/src/Sdk/DTLogging/Logging/ISecretMasker.cs +++ b/src/Sdk/DTLogging/Logging/ISecretMasker.cs @@ -8,7 +8,6 @@ namespace GitHub.DistributedTask.Logging { void AddRegex(String pattern); void AddValue(String value); - void AddValueEncoder(ValueEncoder encoder); ISecretMasker Clone(); String MaskSecrets(String input); } diff --git a/src/Sdk/DTLogging/Logging/SecretMasker.cs b/src/Sdk/DTLogging/Logging/SecretMasker.cs index 430b977f5..b97019b6b 100644 --- a/src/Sdk/DTLogging/Logging/SecretMasker.cs +++ b/src/Sdk/DTLogging/Logging/SecretMasker.cs @@ -10,11 +10,11 @@ namespace GitHub.DistributedTask.Logging [EditorBrowsable(EditorBrowsableState.Never)] public sealed class SecretMasker : ISecretMasker, IDisposable { - public SecretMasker() + public SecretMasker(IEnumerable encoders) { m_originalValueSecrets = new HashSet(); m_regexSecrets = new HashSet(); - m_valueEncoders = new HashSet(); + m_valueEncoders = new HashSet(encoders); m_valueSecrets = new HashSet(); } @@ -104,15 +104,11 @@ namespace GitHub.DistributedTask.Logging } } - // Compute the encoded values. - foreach (ValueEncoder valueEncoder in valueEncoders) - { - String encodedValue = valueEncoder(value); - if (!String.IsNullOrEmpty(encodedValue)) - { - valueSecrets.Add(new ValueSecret(encodedValue)); - } - } + var secretVariations = valueEncoders.SelectMany(encoder => encoder(value)) + .Where(variation => !string.IsNullOrEmpty(variation)) + .Distinct() + .Select(variation => new ValueSecret(variation)); + valueSecrets.AddRange(secretVariations); // Write section. try @@ -135,69 +131,6 @@ namespace GitHub.DistributedTask.Logging } } - /// - /// This implementation assumes no more than one thread is adding regexes, values, or encoders at any given time. - /// - public void AddValueEncoder(ValueEncoder encoder) - { - ValueSecret[] originalSecrets; - - // Read section. - try - { - m_lock.EnterReadLock(); - - // Test whether already added. - if (m_valueEncoders.Contains(encoder)) - { - return; - } - - // Read the original value secrets. - originalSecrets = m_originalValueSecrets.ToArray(); - } - finally - { - if (m_lock.IsReadLockHeld) - { - m_lock.ExitReadLock(); - } - } - - // Compute the encoded values. - var encodedSecrets = new List(); - foreach (ValueSecret originalSecret in originalSecrets) - { - String encodedValue = encoder(originalSecret.m_value); - if (!String.IsNullOrEmpty(encodedValue)) - { - encodedSecrets.Add(new ValueSecret(encodedValue)); - } - } - - // Write section. - try - { - m_lock.EnterWriteLock(); - - // Add the encoder. - m_valueEncoders.Add(encoder); - - // Add the values. - foreach (ValueSecret encodedSecret in encodedSecrets) - { - m_valueSecrets.Add(encodedSecret); - } - } - finally - { - if (m_lock.IsWriteLockHeld) - { - m_lock.ExitWriteLock(); - } - } - } - public ISecretMasker Clone() => new SecretMasker(this); public void Dispose() diff --git a/src/Sdk/DTLogging/Logging/ValueEncoders.cs b/src/Sdk/DTLogging/Logging/ValueEncoders.cs index c3c065bc2..c4bf5fb27 100644 --- a/src/Sdk/DTLogging/Logging/ValueEncoders.cs +++ b/src/Sdk/DTLogging/Logging/ValueEncoders.cs @@ -1,73 +1,97 @@ using System; +using System.Collections.Generic; using System.ComponentModel; +using System.Linq; using System.Security; using System.Text; -using System.Text.RegularExpressions; using Newtonsoft.Json; namespace GitHub.DistributedTask.Logging { [EditorBrowsable(EditorBrowsableState.Never)] - public delegate String ValueEncoder(String value); + public delegate IEnumerable ValueEncoder(string value); [EditorBrowsable(EditorBrowsableState.Never)] public static class ValueEncoders { - public static String Base64StringEscape(String value) - { - return Convert.ToBase64String(Encoding.UTF8.GetBytes(value)); - } - // Base64 is 6 bits -> char - // A byte is 8 bits - // When end user doing somthing like base64(user:password) - // The length of the leading content will cause different base64 encoding result on the password - // So we add base64(value shifted 1 and two bytes) as secret as well. - // B1 B2 B3 B4 B5 B6 B7 - // 000000|00 0000|0000 00|000000| 000000|00 0000|0000 00|000000| - // Char1 Char2 Char3 Char4 - // See the above, the first byte has a character beginning at index 0, the second byte has a character beginning at index 4, the third byte has a character beginning at index 2 and then the pattern repeats - // We register byte offsets for all these possible values - public static String Base64StringEscapeShift1(String value) + public static IEnumerable EnumerateBase64Variations(string value) { - return Base64StringEscapeShift(value, 1); - } + if (!string.IsNullOrEmpty(value)) + { + // A byte is 8 bits. A Base64 "digit" can hold a maximum of 6 bits (2^64 - 1, or values 0 to 63). + // As a result, many Unicode characters (including single-byte letters) cannot be represented using a single Base64 digit. + // Furthermore, on average a Base64 string will be about 33% longer than the original text. + // This is because it generally requires 4 Base64 digits to represent 3 Unicode bytes. (4 / 3 ~ 1.33) + // + // Because of this 4:3 ratio (or, more precisely, 8 bits : 6 bits ratio), there's a cyclical pattern + // to when a byte boundary aligns with a Base64 digit boundary. + // The pattern repeats every 24 bits (the lowest common multiple of 8 and 6). + // + // |-----------24 bits-------------|-----------24 bits------------| + // Base64 Digits: |digit 0|digit 1|digit 2|digit 3|digit 4|digit 5|digit 6|digit7| + // Allocated Bits: aaaaaa aaBBBB BBBBcc cccccc DDDDDD DDeeee eeeeFF FFFFFF + // Unicode chars: |0th char |1st char |2nd char |3rd char |4th char |5th char | - public static String Base64StringEscapeShift2(String value) - { - return Base64StringEscapeShift(value, 2); + // Depending on alignment, the Base64-encoded secret can take any of 3 basic forms. + // For example, the Base64 digits representing "abc" could appear as any of the following: + // "YWJj" when aligned + // ".!FiYw==" when preceded by 3x + 1 bytes + // "..!hYmM=" when preceded by 3x + 2 bytes + // (where . represents an unrelated Base64 digit, ! represents a Base64 digit that should be masked, and x represents any non-negative integer) + + var rawBytes = Encoding.UTF8.GetBytes(value); + + for (var offset = 0; offset <= 2; offset++) + { + var prunedBytes = rawBytes.Skip(offset).ToArray(); + if (prunedBytes.Length > 0) + { + // Don't include Base64 padding characters (=) in Base64 representations of the secret. + // They don't represent anything interesting, so they don't need to be masked. + // (Some clients omit the padding, so we want to be sure we recognize the secret regardless of whether the padding is present or not.) + var buffer = new StringBuilder(Convert.ToBase64String(prunedBytes).TrimEnd(BASE64_PADDING_SUFFIX)); + yield return buffer.ToString(); + + // Also, yield the RFC4648-equivalent RegEx. + buffer.Replace('+', '-'); + buffer.Replace('/', '_'); + yield return buffer.ToString(); + } + } + } } // Used when we pass environment variables to docker to escape " with \" - public static String CommandLineArgumentEscape(String value) + public static IEnumerable CommandLineArgumentEscape(string value) { - return value.Replace("\"", "\\\""); + yield return value.Replace("\"", "\\\""); } - public static String ExpressionStringEscape(String value) + public static IEnumerable ExpressionStringEscape(string value) { - return Expressions2.Sdk.ExpressionUtility.StringEscape(value); + yield return Expressions2.Sdk.ExpressionUtility.StringEscape(value); } - public static String JsonStringEscape(String value) + public static IEnumerable JsonStringEscape(string value) { // Convert to a JSON string and then remove the leading/trailing double-quote. String jsonString = JsonConvert.ToString(value); String jsonEscapedValue = jsonString.Substring(startIndex: 1, length: jsonString.Length - 2); - return jsonEscapedValue; + yield return jsonEscapedValue; } - public static String UriDataEscape(String value) + public static IEnumerable UriDataEscape(string value) { - return UriDataEscape(value, 65519); + yield return UriDataEscape(value, 65519); } - public static String XmlDataEscape(String value) + public static IEnumerable XmlDataEscape(string value) { - return SecurityElement.Escape(value); + yield return SecurityElement.Escape(value); } - public static String TrimDoubleQuotes(String value) + public static IEnumerable TrimDoubleQuotes(string value) { var trimmed = string.Empty; if (!string.IsNullOrEmpty(value) && @@ -78,17 +102,17 @@ namespace GitHub.DistributedTask.Logging trimmed = value.Substring(1, value.Length - 2); } - return trimmed; + yield return trimmed; } - public static String PowerShellPreAmpersandEscape(String value) + public static IEnumerable PowerShellPreAmpersandEscape(string value) { // if the secret is passed to PS as a command and it causes an error, sections of it can be surrounded by color codes - // or printed individually. - + // or printed individually. + // The secret secretpart1&secretpart2&secretpart3 would be split into 2 sections: // 'secretpart1&secretpart2&' and 'secretpart3'. This method masks for the first section. - + // The secret secretpart1&+secretpart2&secretpart3 would be split into 2 sections: // 'secretpart1&+' and (no 's') 'ecretpart2&secretpart3'. This method masks for the first section. @@ -112,10 +136,10 @@ namespace GitHub.DistributedTask.Logging } } - return trimmed; + yield return trimmed; } - public static String PowerShellPostAmpersandEscape(String value) + public static IEnumerable PowerShellPostAmpersandEscape(string value) { var trimmed = string.Empty; if (!string.IsNullOrEmpty(value) && value.Contains("&")) @@ -137,27 +161,10 @@ namespace GitHub.DistributedTask.Logging } } - return trimmed; + yield return trimmed; } - private static string Base64StringEscapeShift(String value, int shift) - { - var bytes = Encoding.UTF8.GetBytes(value); - if (bytes.Length > shift) - { - var shiftArray = new byte[bytes.Length - shift]; - Array.Copy(bytes, shift, shiftArray, 0, bytes.Length - shift); - return Convert.ToBase64String(shiftArray); - } - else - { - return Convert.ToBase64String(bytes); - } - } - - private static String UriDataEscape( - String value, - Int32 maxSegmentSize) + private static string UriDataEscape(string value, Int32 maxSegmentSize) { if (value.Length <= maxSegmentSize) { @@ -183,5 +190,7 @@ namespace GitHub.DistributedTask.Logging return result.ToString(); } + + private const char BASE64_PADDING_SUFFIX = '='; } } diff --git a/src/Test/L0/HostContextL0.cs b/src/Test/L0/HostContextL0.cs index 488064ad8..54793bf3e 100644 --- a/src/Test/L0/HostContextL0.cs +++ b/src/Test/L0/HostContextL0.cs @@ -95,11 +95,11 @@ namespace GitHub.Runner.Common.Tests Assert.Equal("123***123", _hc.SecretMasker.MaskSecrets("123Pass%20word%20123%21123")); Assert.Equal("123***123", _hc.SecretMasker.MaskSecrets("123Pass<word>123!123")); Assert.Equal("123***123", _hc.SecretMasker.MaskSecrets("123Pass''word''123!123")); - Assert.Equal("OlBh***", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($":Password123!")))); - Assert.Equal("YTpQ***", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"a:Password123!")))); + Assert.Equal("OlBh***==", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($":Password123!")))); + Assert.Equal("YTpQ***=", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"a:Password123!")))); Assert.Equal("YWI6***", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"ab:Password123!")))); - Assert.Equal("YWJjOlBh***", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"abc:Password123!")))); - Assert.Equal("YWJjZDpQ***", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"abcd:Password123!")))); + Assert.Equal("YWJjOlBh***==", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"abc:Password123!")))); + Assert.Equal("YWJjZDpQ***=", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"abcd:Password123!")))); Assert.Equal("YWJjZGU6***", _hc.SecretMasker.MaskSecrets(Convert.ToBase64String(Encoding.UTF8.GetBytes($"abcde:Password123!")))); Assert.Equal("123***123", _hc.SecretMasker.MaskSecrets("123Password123!!123")); Assert.Equal("123short123", _hc.SecretMasker.MaskSecrets("123short123")); diff --git a/src/Test/L0/TestHostContext.cs b/src/Test/L0/TestHostContext.cs index a3e484b14..c1679ceda 100644 --- a/src/Test/L0/TestHostContext.cs +++ b/src/Test/L0/TestHostContext.cs @@ -56,9 +56,12 @@ namespace GitHub.Runner.Common.Tests } var traceListener = new HostTraceListener(TraceFileName); - _secretMasker = new SecretMasker(); - _secretMasker.AddValueEncoder(ValueEncoders.JsonStringEscape); - _secretMasker.AddValueEncoder(ValueEncoders.UriDataEscape); + var encoders = new List() + { + ValueEncoders.JsonStringEscape, + ValueEncoders.UriDataEscape + }; + _secretMasker = new SecretMasker(encoders); _traceManager = new TraceManager(traceListener, null, _secretMasker); _trace = GetTrace(nameof(TestHostContext));