diff --git a/src/Admin/Program.cs b/src/Admin/Program.cs index 80a1ae058ca5..84aa1fa846d0 100644 --- a/src/Admin/Program.cs +++ b/src/Admin/Program.cs @@ -6,6 +6,8 @@ public class Program { public static void Main(string[] args) { + RegexDefaults.ConfigureDefaultMatchTimeout(); + Host .CreateDefaultBuilder(args) .UseBitwardenSdk() diff --git a/src/Api/Program.cs b/src/Api/Program.cs index 51096693b544..c3ccb4959574 100644 --- a/src/Api/Program.cs +++ b/src/Api/Program.cs @@ -6,6 +6,8 @@ public class Program { public static void Main(string[] args) { + RegexDefaults.ConfigureDefaultMatchTimeout(); + CreateHostBuilder(args).Build().Run(); } diff --git a/src/Api/SecretsManager/Models/Request/RequestSMAccessRequestModel.cs b/src/Api/SecretsManager/Models/Request/RequestSMAccessRequestModel.cs index b3a9e2a14080..a61fcfeb7093 100644 --- a/src/Api/SecretsManager/Models/Request/RequestSMAccessRequestModel.cs +++ b/src/Api/SecretsManager/Models/Request/RequestSMAccessRequestModel.cs @@ -10,5 +10,12 @@ public class RequestSMAccessRequestModel [Required] public Guid OrganizationId { get; set; } [Required(ErrorMessage = "Add a note is a required field")] + [StringLength(MaxEmailContentLength)] public string EmailContent { get; set; } + + /// + /// The note is interpolated into an email sent to every organization admin, so it is bounded + /// to keep both the email and the server-side sanitization work proportionate. + /// + public const int MaxEmailContentLength = 1000; } diff --git a/src/Billing/Program.cs b/src/Billing/Program.cs index 334dc49368dc..9556fb45e19e 100644 --- a/src/Billing/Program.cs +++ b/src/Billing/Program.cs @@ -6,6 +6,8 @@ public class Program { public static void Main(string[] args) { + RegexDefaults.ConfigureDefaultMatchTimeout(); + Host .CreateDefaultBuilder(args) .UseBitwardenSdk() diff --git a/src/Core/Utilities/CoreHelpers.cs b/src/Core/Utilities/CoreHelpers.cs index 50e55333e2ef..7a76b74e8a91 100644 --- a/src/Core/Utilities/CoreHelpers.cs +++ b/src/Core/Utilities/CoreHelpers.cs @@ -508,24 +508,94 @@ public static string PreventEmailAutoLinking(string value) /// /// Sanitizes a value for display in an email by neutralizing anything that looks like an - /// address or link (e.g. "@" and "scheme://"). It deliberately does NOT HTML-encode the + /// address or link (e.g. "@", ".x" and "scheme://"). It deliberately does NOT HTML-encode the /// result: the mail templates are rendered by Handlebars, which HTML-encodes interpolated /// values ({{ }}) by default. Encoding here as well produced double-encoded output. /// + /// + /// This is a single left-to-right pass and runs in time linear to the input length, so it is + /// safe to call on caller-supplied input. A "scheme://" is any run of word characters + /// immediately followed by "://", or a "://" at the very start of the string. If stripping + /// one exposes another (e.g. "a:/b:///"), that one is stripped as well. + /// public static string SanitizeForEmail(string value) { - var cleanedValue = value.Replace("@", "[at]"); - var regexOptions = RegexOptions.CultureInvariant | - RegexOptions.Singleline | - RegexOptions.IgnoreCase; - cleanedValue = Regex.Replace(cleanedValue, @"(\.\w)", - m => string.Concat("[dot]", m.ToString().Last()), regexOptions); - while (Regex.IsMatch(cleanedValue, @"((^|\b)(\w*)://)", regexOptions)) + if (string.IsNullOrEmpty(value)) + { + return value; + } + + var builder = new StringBuilder(value.Length); + for (var i = 0; i < value.Length; i++) + { + var c = value[i]; + switch (c) + { + case '@': + builder.Append("[at]"); + break; + case '.' when i + 1 < value.Length && IsWordChar(value[i + 1]): + builder.Append("[dot]"); + break; + default: + builder.Append(c); + break; + } + + StripTrailingUrlScheme(builder); + } + + return builder.ToString(); + } + + /// + /// If ends with "://" that is either at the start of the buffer or + /// preceded by a word character, removes the "://" together with the word run before it. + /// + private static void StripTrailingUrlScheme(StringBuilder builder) + { + const string separator = "://"; + var separatorStart = builder.Length - separator.Length; + if (separatorStart < 0) + { + return; + } + + for (var j = 0; j < separator.Length; j++) + { + if (builder[separatorStart + j] != separator[j]) + { + return; + } + } + + if (separatorStart > 0 && !IsWordChar(builder[separatorStart - 1])) + { + return; + } + + var schemeStart = separatorStart; + while (schemeStart > 0 && IsWordChar(builder[schemeStart - 1])) + { + schemeStart--; + } + + builder.Length = schemeStart; + } + + /// + /// Mirrors the regular expression \w class: letters, decimal digits, non-spacing marks + /// and connector punctuation (which includes "_"). + /// + private static bool IsWordChar(char c) + { + if (char.IsLetterOrDigit(c)) { - cleanedValue = Regex.Replace(cleanedValue, @"((^|\b)(\w*)://)", - string.Empty, regexOptions); + return true; } - return cleanedValue; + + var category = CharUnicodeInfo.GetUnicodeCategory(c); + return category is UnicodeCategory.NonSpacingMark or UnicodeCategory.ConnectorPunctuation; } public static string DateTimeToTableStorageKey(DateTime? date = null) diff --git a/src/Core/Utilities/RegexDefaults.cs b/src/Core/Utilities/RegexDefaults.cs new file mode 100644 index 000000000000..02a0b88f1c7c --- /dev/null +++ b/src/Core/Utilities/RegexDefaults.cs @@ -0,0 +1,27 @@ +namespace Bit.Core.Utilities; + +/// +/// Process-wide defaults for . +/// +/// +/// Deliberately kept free of any static Regex fields: the default match timeout is read once, +/// when the Regex type is first initialized, so it must be configured before any regular +/// expression is constructed anywhere in the process. +/// +public static class RegexDefaults +{ + /// + /// The timeout applied to every Regex that does not specify its own. + /// + public static readonly TimeSpan DefaultMatchTimeout = TimeSpan.FromSeconds(5); + + /// + /// Configures a process-wide default Regex match timeout as defence in depth against + /// catastrophic backtracking on untrusted input (CWE-1333). Call this as the very first + /// statement of Main, before anything else has a chance to construct a Regex. + /// + public static void ConfigureDefaultMatchTimeout() + { + AppContext.SetData("REGEX_DEFAULT_MATCH_TIMEOUT", DefaultMatchTimeout); + } +} diff --git a/src/Events/Program.cs b/src/Events/Program.cs index 78a3cfcdc09b..b868156777a5 100644 --- a/src/Events/Program.cs +++ b/src/Events/Program.cs @@ -6,6 +6,8 @@ public class Program { public static void Main(string[] args) { + RegexDefaults.ConfigureDefaultMatchTimeout(); + Host .CreateDefaultBuilder(args) .UseBitwardenSdk() diff --git a/src/EventsProcessor/Program.cs b/src/EventsProcessor/Program.cs index 45f8b620b18b..471adb643079 100644 --- a/src/EventsProcessor/Program.cs +++ b/src/EventsProcessor/Program.cs @@ -6,6 +6,8 @@ public class Program { public static void Main(string[] args) { + RegexDefaults.ConfigureDefaultMatchTimeout(); + Host .CreateDefaultBuilder(args) .UseBitwardenSdk() diff --git a/src/Icons/Program.cs b/src/Icons/Program.cs index 5ca5723a520c..d37996e01fd1 100644 --- a/src/Icons/Program.cs +++ b/src/Icons/Program.cs @@ -6,6 +6,11 @@ public class Program { public static void Main(string[] args) { + // This project does not reference Core, so this mirrors Bit.Core.Utilities.RegexDefaults: + // a process-wide Regex match timeout as defence in depth against catastrophic + // backtracking (CWE-1333). It must run before any Regex is constructed. + AppContext.SetData("REGEX_DEFAULT_MATCH_TIMEOUT", TimeSpan.FromSeconds(5)); + Host .CreateDefaultBuilder(args) .UseBitwardenSdk() diff --git a/src/Identity/Program.cs b/src/Identity/Program.cs index ae284c86f28e..a3e20f1dafad 100644 --- a/src/Identity/Program.cs +++ b/src/Identity/Program.cs @@ -6,6 +6,8 @@ public class Program { public static void Main(string[] args) { + RegexDefaults.ConfigureDefaultMatchTimeout(); + CreateHostBuilder(args) .Build() .Run(); diff --git a/src/Notifications/Program.cs b/src/Notifications/Program.cs index ec7ea67fdaf2..f242f2ca00b7 100644 --- a/src/Notifications/Program.cs +++ b/src/Notifications/Program.cs @@ -6,6 +6,11 @@ public class Program { public static void Main(string[] args) { + // This project does not reference Core, so this mirrors Bit.Core.Utilities.RegexDefaults: + // a process-wide Regex match timeout as defence in depth against catastrophic + // backtracking (CWE-1333). It must run before any Regex is constructed. + AppContext.SetData("REGEX_DEFAULT_MATCH_TIMEOUT", TimeSpan.FromSeconds(5)); + Host .CreateDefaultBuilder(args) .UseBitwardenSdk() diff --git a/test/Api.Test/SecretsManager/Models/Request/RequestSMAccessRequestModelTests.cs b/test/Api.Test/SecretsManager/Models/Request/RequestSMAccessRequestModelTests.cs new file mode 100644 index 000000000000..6f38459626bd --- /dev/null +++ b/test/Api.Test/SecretsManager/Models/Request/RequestSMAccessRequestModelTests.cs @@ -0,0 +1,59 @@ +using System.ComponentModel.DataAnnotations; +using Bit.Api.SecretsManager.Models.Request; +using Xunit; + +namespace Bit.Api.Test.SecretsManager.Models.Request; + +public class RequestSMAccessRequestModelTests +{ + [Theory] + [InlineData("I need access to the deployment secrets.")] + [InlineData("x")] + public void Validate_WithReasonableNote_ReturnsNoErrors(string emailContent) + { + var results = Validate(BuildModel(emailContent)); + + Assert.Empty(results); + } + + [Fact] + public void Validate_WithNoteAtMaxLength_ReturnsNoErrors() + { + var results = Validate(BuildModel(new string('a', RequestSMAccessRequestModel.MaxEmailContentLength))); + + Assert.Empty(results); + } + + [Fact] + public void Validate_WithNoteOverMaxLength_ReturnsError() + { + var results = Validate(BuildModel(new string('a', RequestSMAccessRequestModel.MaxEmailContentLength + 1))); + + var error = Assert.Single(results); + Assert.Contains(nameof(RequestSMAccessRequestModel.EmailContent), error.MemberNames); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void Validate_WithMissingNote_ReturnsError(string? emailContent) + { + var results = Validate(BuildModel(emailContent)); + + var error = Assert.Single(results); + Assert.Contains(nameof(RequestSMAccessRequestModel.EmailContent), error.MemberNames); + } + + private static RequestSMAccessRequestModel BuildModel(string? emailContent) => new() + { + OrganizationId = Guid.NewGuid(), + EmailContent = emailContent!, + }; + + private static List Validate(RequestSMAccessRequestModel model) + { + var results = new List(); + Validator.TryValidateObject(model, new ValidationContext(model), results, true); + return results; + } +} diff --git a/test/Core.Test/Utilities/CoreHelpersTests.cs b/test/Core.Test/Utilities/CoreHelpersTests.cs index a2985eb88f2f..f8edbb8cba9f 100644 --- a/test/Core.Test/Utilities/CoreHelpersTests.cs +++ b/test/Core.Test/Utilities/CoreHelpersTests.cs @@ -1,4 +1,5 @@ -using System.Text; +using System.Diagnostics; +using System.Text; using AutoFixture; using Bit.Core.AdminConsole.Context; using Bit.Core.AdminConsole.Enums.Provider; @@ -486,11 +487,64 @@ public void ReplaceWhiteSpace_Success(string email) [InlineData("user@example.com", "user[at]example[dot]com")] [InlineData("https://bitwarden.com", "bitwarden[dot]com")] [InlineData("ftp://files.example.org", "files[dot]example[dot]org")] + [InlineData("HTTPS://BITWARDEN.COM", "BITWARDEN[dot]COM")] + [InlineData("see https://a.io and http://b.io", "see a[dot]io and b[dot]io")] + [InlineData("my-app://open", "my-open")] + [InlineData("under_score://x", "x")] + [InlineData("://leading", "leading")] + [InlineData("a ://b", "a ://b")] + [InlineData("trailing.", "trailing.")] + [InlineData("a..b", "a.[dot]b")] + [InlineData("a@://", "a[at]://")] + [InlineData("Acme Corp", "Acme Corp")] + [InlineData("Ünïcødé://näme", "näme")] + [InlineData("", "")] public void SanitizeForEmail_NeutralizesAddressesAndLinks(string value, string expected) { Assert.Equal(expected, CoreHelpers.SanitizeForEmail(value)); } + [Theory] + [InlineData("http://://bitwarden.com", "bitwarden[dot]com")] + [InlineData("a:/b:///", "")] + [InlineData("xc://ab://", "")] + public void SanitizeForEmail_StripsSchemesExposedByEarlierStripping(string value, string expected) + { + Assert.Equal(expected, CoreHelpers.SanitizeForEmail(value)); + } + + [Fact] + public void SanitizeForEmail_LargeInput_CompletesInLinearTime() + { + // Before the rewrite this shape of input required one full regex pass per "://", + // i.e. quadratic time; at this size that took minutes. Linear processing takes + // milliseconds, so a generous bound still cleanly separates the two. + var value = "a://" + string.Concat(Enumerable.Repeat("://", 100_000)); + var stopwatch = Stopwatch.StartNew(); + + var result = CoreHelpers.SanitizeForEmail(value); + + stopwatch.Stop(); + Assert.Equal(string.Empty, result); + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(2), + $"SanitizeForEmail took {stopwatch.Elapsed} on {value.Length} characters"); + } + + [Fact] + public void SanitizeForEmail_LargeMixedInput_CompletesInLinearTime() + { + var value = string.Concat(Enumerable.Repeat("user@example.com https://bitwarden.com/x ", 25_000)); + var stopwatch = Stopwatch.StartNew(); + + var result = CoreHelpers.SanitizeForEmail(value); + + stopwatch.Stop(); + Assert.DoesNotContain("@", result); + Assert.DoesNotContain("://", result); + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(2), + $"SanitizeForEmail took {stopwatch.Elapsed} on {value.Length} characters"); + } + [Theory] [InlineData("")] [InlineData("Org & Co \"quoted\"")]