From 3a5b83ec7a8b8116510d70a478762d4118ba8d66 Mon Sep 17 00:00:00 2001 From: Ben McCallum Date: Mon, 31 Jan 2022 11:03:38 +0100 Subject: [PATCH 1/8] Implement IDisposal for RequestValidator Relates to #466 Turns out this may just be for correctness, as these algos may not hold onto any unmanaged resources. https://github.com/dotnet/aspnetcore/issues/32871 --- src/Twilio/Security/RequestValidator.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index aa94ada48..abe6da79a 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -9,7 +9,7 @@ namespace Twilio.Security /// /// Twilio request validator /// - public class RequestValidator + public class RequestValidator : IDisposable { private readonly HMACSHA1 _hmac; private readonly SHA256 _sha; @@ -160,5 +160,11 @@ private string PreserveCase(string url, string replacementString) var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); } + + public void Dispose() + { + _hmac.Dispose() + _sha.Dispose(); + } } } From c3c01661cec332b986417e3ae08a1b6755c49d51 Mon Sep 17 00:00:00 2001 From: Ben McCallum Date: Mon, 7 Feb 2022 11:53:35 +0100 Subject: [PATCH 2/8] fix: Make RequestValidator thread safe --- src/Twilio/Security/RequestValidator.cs | 36 +++++++++++-------- .../Security/RequestValidatorTest.cs | 29 ++++++++++++++- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index abe6da79a..5e4188acc 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -11,8 +11,7 @@ namespace Twilio.Security /// public class RequestValidator : IDisposable { - private readonly HMACSHA1 _hmac; - private readonly SHA256 _sha; + private readonly string _secret; /// /// Create a new RequestValidator @@ -20,8 +19,7 @@ public class RequestValidator : IDisposable /// Signing secret public RequestValidator(string secret) { - _hmac = new HMACSHA1(Encoding.UTF8.GetBytes(secret)); - _sha = SHA256.Create(); + _secret = secret; } /// @@ -68,10 +66,15 @@ public bool Validate(string url, string body, string expected) return Validate(url, new Dictionary(), expected) && ValidateBody(body, bodyHash); } - public bool ValidateBody(string rawBody, string expected) + public static bool ValidateBody(string rawBody, string expected) { - var signature = _sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); - return SecureCompare(BitConverter.ToString(signature).Replace("-","").ToLower(), expected); + // TODO: In future, use net6's one-shot methods. + // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 + using (var sha = SHA256.Create()) + { + var signature = sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); + return SecureCompare(BitConverter.ToString(signature).Replace("-", "").ToLower(), expected); + } } private static IDictionary ToDictionary(NameValueCollection col) @@ -96,10 +99,15 @@ private string GetValidationSignature(string url, IDictionary pa { b.Append(key).Append(parameters[key] ?? ""); } + } + + // TODO: In future, use net6's one-shot methods. + // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 + using (var hmac = new HMACSHA1(Encoding.UTF8.GetBytes(_secret))) + { + var hash = hmac.ComputeHash(Encoding.UTF8.GetBytes(b.ToString())); + return Convert.ToBase64String(hash); } - - var hash = _hmac.ComputeHash(Encoding.UTF8.GetBytes(b.ToString())); - return Convert.ToBase64String(hash); } private static bool SecureCompare(string a, string b) @@ -124,18 +132,18 @@ private static bool SecureCompare(string a, string b) return mismatch == 0; } - private string RemovePort(string url) + private static string RemovePort(string url) { return SetPort(url, -1); } - private string AddPort(string url) + private static string AddPort(string url) { var uri = new UriBuilder(url); return SetPort(url, uri.Port); } - private string SetPort(string url, int port) + private static string SetPort(string url, int port) { var uri = new UriBuilder(url); uri.Host = PreserveCase(url, uri.Host); @@ -155,7 +163,7 @@ private string SetPort(string url, int port) return uri.Uri.OriginalString.Replace(uri.Scheme, scheme); } - private string PreserveCase(string url, string replacementString) + private static string PreserveCase(string url, string replacementString) { var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); diff --git a/test/Twilio.Test/Security/RequestValidatorTest.cs b/test/Twilio.Test/Security/RequestValidatorTest.cs index 22dd894f3..0d3b8646d 100644 --- a/test/Twilio.Test/Security/RequestValidatorTest.cs +++ b/test/Twilio.Test/Security/RequestValidatorTest.cs @@ -1,6 +1,8 @@ using System; using System.Collections.Generic; using System.Collections.Specialized; +using System.Diagnostics; +using System.Threading; using NUnit.Framework; using Twilio.Security; @@ -121,7 +123,7 @@ public void TestValidateCollection() [Test] public void TestValidateBody() { - Assert.IsTrue(_validator.ValidateBody(Body, BodyHash), "Request body validation failed"); + Assert.IsTrue(RequestValidator.ValidateBody(Body, BodyHash), "Request body validation failed"); } [Test] @@ -183,6 +185,31 @@ public void TestValidateAddsCustomPortHttp() { const string url = "http://mycompany.com:1234/myapp.php?foo=1&bar=2"; Assert.IsTrue(_validator.Validate(url, _parameters, "Zmvh+3yNM1Phv2jhDCwEM3q5ebU="), "Request does not match provided signature"); + } + + [Test] + public void TestIsThreadSafe() + { + var validator = new RequestValidator("secret"); + var thread1 = new Thread(Validate); + var thread2 = new Thread(Validate); + + Assert.DoesNotThrow(() => + { + thread1.Start(validator); + thread2.Start(validator); + thread1.Join(); + thread2.Join(); + }); + } + + private static void Validate(object obj) + { + var sw = Stopwatch.StartNew(); + while (sw.ElapsedMilliseconds < 5000) + { + ((RequestValidator)obj).Validate("https://foo.com", "123", "foo"); + } } } } From 48e66dfc269bd8c9c68b14b2d32d77fcf8aff654 Mon Sep 17 00:00:00 2001 From: Ben McCallum Date: Mon, 7 Feb 2022 11:57:18 +0100 Subject: [PATCH 3/8] Remove IDisposable interface as not needed anymore --- src/Twilio/Security/RequestValidator.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index 5e4188acc..bd197af6a 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -9,7 +9,7 @@ namespace Twilio.Security /// /// Twilio request validator /// - public class RequestValidator : IDisposable + public class RequestValidator { private readonly string _secret; @@ -168,11 +168,5 @@ private static string PreserveCase(string url, string replacementString) var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); } - - public void Dispose() - { - _hmac.Dispose() - _sha.Dispose(); - } } } From 813a64b86fdef8d17d873be6fe211376b8722dd8 Mon Sep 17 00:00:00 2001 From: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com> Date: Wed, 28 Dec 2022 13:59:46 -0500 Subject: [PATCH 4/8] Optimize request validator but keep backwards compatible --- .gitignore | 1 + Twilio.sln | 19 +- src/Twilio/Security/RequestValidator.cs | 137 ++++++++++----- test/Twilio.Benchmark/Program.cs | 92 ++++++++++ .../RequestValidatorOriginal.cs | 164 ++++++++++++++++++ test/Twilio.Benchmark/Twilio.Benchmark.csproj | 17 ++ 6 files changed, 385 insertions(+), 45 deletions(-) create mode 100644 test/Twilio.Benchmark/Program.cs create mode 100644 test/Twilio.Benchmark/RequestValidatorOriginal.cs create mode 100644 test/Twilio.Benchmark/Twilio.Benchmark.csproj diff --git a/.gitignore b/.gitignore index 57907c574..7dd56f4a2 100644 --- a/.gitignore +++ b/.gitignore @@ -235,3 +235,4 @@ html/ # sonar cloud stuff .sonarqube +/test/Twilio.Benchmark/BenchmarkDotNet.Artifacts diff --git a/Twilio.sln b/Twilio.sln index f75efdaab..f11fa4220 100644 --- a/Twilio.sln +++ b/Twilio.sln @@ -1,7 +1,7 @@  Microsoft Visual Studio Solution File, Format Version 12.00 -# Visual Studio 15 -VisualStudioVersion = 15.0.26206.0 +# Visual Studio Version 17 +VisualStudioVersion = 17.4.33205.214 MinimumVisualStudioVersion = 10.0.40219.1 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{36585F38-8C30-49A9-BDA1-9A0DC61C288B}" EndProject @@ -11,6 +11,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Twilio", "src\Twilio\Twilio EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Twilio.Test", "test\Twilio.Test\Twilio.Test.csproj", "{DC35107A-F987-47A3-B0BC-7110BA15943C}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Twilio.Benchmark", "test\Twilio.Benchmark\Twilio.Benchmark.csproj", "{80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -45,6 +47,18 @@ Global {DC35107A-F987-47A3-B0BC-7110BA15943C}.Release|x64.Build.0 = Release|Any CPU {DC35107A-F987-47A3-B0BC-7110BA15943C}.Release|x86.ActiveCfg = Release|Any CPU {DC35107A-F987-47A3-B0BC-7110BA15943C}.Release|x86.Build.0 = Release|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Debug|Any CPU.Build.0 = Debug|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Debug|x64.ActiveCfg = Debug|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Debug|x64.Build.0 = Debug|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Debug|x86.ActiveCfg = Debug|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Debug|x86.Build.0 = Debug|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Release|Any CPU.ActiveCfg = Release|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Release|Any CPU.Build.0 = Release|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Release|x64.ActiveCfg = Release|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Release|x64.Build.0 = Release|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Release|x86.ActiveCfg = Release|Any CPU + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -52,6 +66,7 @@ Global GlobalSection(NestedProjects) = preSolution {62BB8FE9-99DD-475D-80EB-D2E53C380754} = {36585F38-8C30-49A9-BDA1-9A0DC61C288B} {DC35107A-F987-47A3-B0BC-7110BA15943C} = {FE04FB2E-73FB-4E45-AAEE-EE04754A5E9C} + {80DEE3E3-5F8E-40C2-A68C-15463C4CFAB1} = {FE04FB2E-73FB-4E45-AAEE-EE04754A5E9C} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {75638FC3-0E0B-4D79-8BEB-8CC499BF98C5} diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index aa94ada48..6eb9ab933 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using System.Collections.Generic; using System.Collections.Specialized; using System.Security.Cryptography; @@ -45,16 +46,63 @@ public bool Validate(string url, NameValueCollection parameters, string expected /// true if the signature matches the result; false otherwise public bool Validate(string url, IDictionary parameters, string expected) { - // check signature of url with and without port, since sig generation on back end is inconsistent - var signatureWithoutPort = GetValidationSignature(RemovePort(url), parameters); - var signatureWithPort = GetValidationSignature(AddPort(url), parameters); - // If either url produces a valid signature, we accept the request as valid - return SecureCompare(signatureWithoutPort, expected) || SecureCompare(signatureWithPort, expected); - } - + if (string.IsNullOrEmpty(url)) + throw new ArgumentException("Parameter 'url' cannot be null or empty."); + if (string.IsNullOrEmpty(expected)) + throw new ArgumentException("Parameter 'expected' cannot be null or empty."); + + if(parameters == null || parameters.Count == 0) + { + var signature = GetValidationSignature(url); + if (SecureCompare(signature, expected)) return true; + + // check signature of url with and without port, since sig generation on back end is inconsistent + // If either url produces a valid signature, we accept the request as valid + url = GetUriVariation(url); + signature = GetValidationSignature(url); + if (SecureCompare(signature, expected)) return true; + return false; + } + else + { + var parameterStringBuilder = GetJoinedParametersStringBuilder(parameters); + parameterStringBuilder.Insert(0, url); + var signature = GetValidationSignature(parameterStringBuilder.ToString()); + if (SecureCompare(signature, expected)) return true; + parameterStringBuilder.Remove(0, url.Length); + + // check signature of url with and without port, since sig generation on back end is inconsistent + // If either url produces a valid signature, we accept the request as valid + url = GetUriVariation(url); + parameterStringBuilder.Insert(0, url); + signature = GetValidationSignature(parameterStringBuilder.ToString()); + if (SecureCompare(signature, expected)) return true; + + return false; + } + } + + private StringBuilder GetJoinedParametersStringBuilder(IDictionary parameters) + { + var keys = parameters.Keys.ToArray(); + Array.Sort(keys, StringComparer.Ordinal); + + var b = new StringBuilder(); + foreach (var key in keys) + { + b.Append(key).Append(parameters[key] ?? ""); + } + return b; + } + public bool Validate(string url, string body, string expected) { - var paramString = new UriBuilder(url).Query.TrimStart('?'); + if (string.IsNullOrEmpty(url)) + throw new ArgumentException("Parameter 'url' cannot be null or empty."); + if (string.IsNullOrEmpty(expected)) + throw new ArgumentException("Parameter 'expected' cannot be null or empty."); + + var paramString = new Uri(url, UriKind.Absolute).Query.TrimStart('?'); var bodyHash = ""; foreach (var param in paramString.Split('&')) { @@ -65,13 +113,13 @@ public bool Validate(string url, string body, string expected) } } - return Validate(url, new Dictionary(), expected) && ValidateBody(body, bodyHash); + return Validate(url, (IDictionary) null, expected) && ValidateBody(body, bodyHash); } public bool ValidateBody(string rawBody, string expected) { var signature = _sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); - return SecureCompare(BitConverter.ToString(signature).Replace("-","").ToLower(), expected); + return SecureCompare(BitConverter.ToString(signature).Replace("-", "").ToLower(), expected); } private static IDictionary ToDictionary(NameValueCollection col) @@ -81,27 +129,16 @@ private static IDictionary ToDictionary(NameValueCollection col) { dict.Add(k, col[k]); } + return dict; } - private string GetValidationSignature(string url, IDictionary parameters) + private string GetValidationSignature(string urlWithParameters) { - var b = new StringBuilder(url); - if (parameters != null) - { - var sortedKeys = new List(parameters.Keys); - sortedKeys.Sort(StringComparer.Ordinal); - - foreach (var key in sortedKeys) - { - b.Append(key).Append(parameters[key] ?? ""); - } - } - - var hash = _hmac.ComputeHash(Encoding.UTF8.GetBytes(b.ToString())); + byte[] hash = _hmac.ComputeHash(Encoding.UTF8.GetBytes(urlWithParameters)); return Convert.ToBase64String(hash); - } - + } + private static bool SecureCompare(string a, string b) { if (a == null || b == null) @@ -124,41 +161,55 @@ private static bool SecureCompare(string a, string b) return mismatch == 0; } - private string RemovePort(string url) + /// + /// Returns URL without port if given URL has port, returns URL with port if given URL has no port + /// + /// + /// + private string GetUriVariation(string url) { - return SetPort(url, -1); - } + var uri = new Uri(url); + var uriBuilder = new UriBuilder(uri); + var port = uri.GetComponents(UriComponents.Port, UriFormat.UriEscaped); + // if port already removed + if (port == "") + { + return SetPort(url, uriBuilder, uriBuilder.Port); + } - private string AddPort(string url) - { - var uri = new UriBuilder(url); - return SetPort(url, uri.Port); + return SetPort(url, uriBuilder, -1); } - private string SetPort(string url, int port) + private string SetPort(string url, UriBuilder uri, int newPort) { - var uri = new UriBuilder(url); - uri.Host = PreserveCase(url, uri.Host); - if (port == -1) + if (newPort == -1) { - uri.Port = port; + uri.Port = newPort; } - else if ((port != 443) && (port != 80)) + else if (newPort != 443 && newPort != 80) { - uri.Port = port; + uri.Port = newPort; } else { uri.Port = uri.Scheme == "https" ? 443 : 80; } + + var uriStringBuilder = new StringBuilder(uri.ToString()); + + var host = PreserveCase(url, uri.Host); + uriStringBuilder.Replace(uri.Host, host); + var scheme = PreserveCase(url, uri.Scheme); - return uri.Uri.OriginalString.Replace(uri.Scheme, scheme); - } + uriStringBuilder.Replace(uri.Scheme, scheme); + return uriStringBuilder.ToString(); + } + private string PreserveCase(string url, string replacementString) { var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); return url.Substring(startIndex, replacementString.Length); } } -} +} \ No newline at end of file diff --git a/test/Twilio.Benchmark/Program.cs b/test/Twilio.Benchmark/Program.cs new file mode 100644 index 000000000..8c8e2dcd2 --- /dev/null +++ b/test/Twilio.Benchmark/Program.cs @@ -0,0 +1,92 @@ +using System.Collections.Specialized; +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Running; +using Twilio.Security; + +var summary = BenchmarkRunner.Run(); +Console.Write(summary); + +[MemoryDiagnoser] +public class RequestValidationBenchmark +{ + private const string Secret = "12345"; + private const string UnhappyPathUrl = "HTTP://MyCompany.com:8080/myapp.php?foo=1&bar=2"; + private const string UnhappyPathSignature = "eYYN9fMlxrQMXOsr7bIzoPTrbxA="; + private const string HappyPathUrl = "https://mycompany.com/myapp.php?foo=1&bar=2"; + private const string HappyPathSignature = "3LL3BFKOcn80artVM5inMPFpmtU="; + private static readonly NameValueCollection UnhappyPathParameters = new() + { + {"ToCountry", "US"}, + {"ToState", "OH"}, + {"SmsMessageSid", "SMcea2a3bd6f50296f8fab60f377db03eb"}, + {"NumMedia", "0"}, + {"ToCity", "UTICA"}, + {"FromZip", "20705"}, + {"SmsSid", "SMcea2a3bd6f50296f8fab60f377db03eb"}, + {"FromState", "DC"}, + {"SmsStatus", "received"}, + {"FromCity", "BELTSVILLE"}, + {"Body", "Ahoy!"}, + {"FromCountry", "US"}, + {"To", "+10123456789"}, + {"ToZip", "43037"}, + {"NumSegments", "1"}, + {"ReferralNumMedia", "0"}, + {"MessageSid", "SMcea2a3bd6f50296f8fab60f377db03eb"}, + {"AccountSid", "ACe718619887aac3ee5b21edafbvsdf6h7fgb"}, + {"From", "+10123456789"}, + {"ApiVersion", "2010-04-01"} + }; + private static readonly Dictionary HappyPathParameters = new() + { + {"ToCountry", "US"}, + {"ToState", "OH"}, + {"SmsMessageSid", "SMcea2a3bd6f50296f8fab60f377db03eb"}, + {"NumMedia", "0"}, + {"ToCity", "UTICA"}, + {"FromZip", "20705"}, + {"SmsSid", "SMcea2a3bd6f50296f8fab60f377db03eb"}, + {"FromState", "DC"}, + {"SmsStatus", "received"}, + {"FromCity", "BELTSVILLE"}, + {"Body", "Ahoy!"}, + {"FromCountry", "US"}, + {"To", "+10123456789"}, + {"ToZip", "43037"}, + {"NumSegments", "1"}, + {"ReferralNumMedia", "0"}, + {"MessageSid", "SMcea2a3bd6f50296f8fab60f377db03eb"}, + {"AccountSid", "ACe718619887aac3ee5b21edafbvsdf6h7fgb"}, + {"From", "+10123456789"}, + {"ApiVersion", "2010-04-01"} + }; + + + [Benchmark] + public void OriginalUnhappyPath() + { + var requestValidator = new RequestValidatorOriginal(Secret); + requestValidator.Validate(UnhappyPathUrl, UnhappyPathParameters, UnhappyPathSignature); + } + + [Benchmark] + public void CurrentUnhappyPath() + { + var requestValidator = new RequestValidator(Secret); + requestValidator.Validate(UnhappyPathUrl, UnhappyPathParameters, UnhappyPathSignature); + } + + [Benchmark] + public void OriginalHappyPath() + { + var requestValidator = new RequestValidatorOriginal(Secret); + requestValidator.Validate(HappyPathUrl, HappyPathParameters, HappyPathSignature); + } + + [Benchmark] + public void CurrentHappyPath() + { + var requestValidator = new RequestValidator(Secret); + requestValidator.Validate(HappyPathUrl, HappyPathParameters, HappyPathSignature); + } +} \ No newline at end of file diff --git a/test/Twilio.Benchmark/RequestValidatorOriginal.cs b/test/Twilio.Benchmark/RequestValidatorOriginal.cs new file mode 100644 index 000000000..9395a7500 --- /dev/null +++ b/test/Twilio.Benchmark/RequestValidatorOriginal.cs @@ -0,0 +1,164 @@ +using System; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Security.Cryptography; +using System.Text; + +namespace Twilio.Security +{ + /// + /// Twilio request validator + /// + public class RequestValidatorOriginal + { + private readonly HMACSHA1 _hmac; + private readonly SHA256 _sha; + + /// + /// Create a new RequestValidator + /// + /// Signing secret + public RequestValidatorOriginal(string secret) + { + _hmac = new HMACSHA1(Encoding.UTF8.GetBytes(secret)); + _sha = SHA256.Create(); + } + + /// + /// Validate against a request + /// + /// Request URL + /// Request parameters + /// Expected result + /// true if the signature matches the result; false otherwise + public bool Validate(string url, NameValueCollection parameters, string expected) + { + return Validate(url, ToDictionary(parameters), expected); + } + + /// + /// Validate against a request + /// + /// Request URL + /// Request parameters + /// Expected result + /// true if the signature matches the result; false otherwise + public bool Validate(string url, IDictionary parameters, string expected) + { + // check signature of url with and without port, since sig generation on back end is inconsistent + var signatureWithoutPort = GetValidationSignature(RemovePort(url), parameters); + var signatureWithPort = GetValidationSignature(AddPort(url), parameters); + // If either url produces a valid signature, we accept the request as valid + return SecureCompare(signatureWithoutPort, expected) || SecureCompare(signatureWithPort, expected); + } + + public bool Validate(string url, string body, string expected) + { + var paramString = new UriBuilder(url).Query.TrimStart('?'); + var bodyHash = ""; + foreach (var param in paramString.Split('&')) + { + var split = param.Split('='); + if (split[0] == "bodySHA256") + { + bodyHash = Uri.UnescapeDataString(split[1]); + } + } + + return Validate(url, new Dictionary(), expected) && ValidateBody(body, bodyHash); + } + + public bool ValidateBody(string rawBody, string expected) + { + var signature = _sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); + return SecureCompare(BitConverter.ToString(signature).Replace("-", "").ToLower(), expected); + } + + private static IDictionary ToDictionary(NameValueCollection col) + { + var dict = new Dictionary(); + foreach (var k in col.AllKeys) + { + dict.Add(k, col[k]); + } + return dict; + } + + private string GetValidationSignature(string url, IDictionary parameters) + { + var b = new StringBuilder(url); + if (parameters != null) + { + var sortedKeys = new List(parameters.Keys); + sortedKeys.Sort(StringComparer.Ordinal); + + foreach (var key in sortedKeys) + { + b.Append(key).Append(parameters[key] ?? ""); + } + } + + var hash = _hmac.ComputeHash(Encoding.UTF8.GetBytes(b.ToString())); + return Convert.ToBase64String(hash); + } + + private static bool SecureCompare(string a, string b) + { + if (a == null || b == null) + { + return false; + } + + var n = a.Length; + if (n != b.Length) + { + return false; + } + + var mismatch = 0; + for (var i = 0; i < n; i++) + { + mismatch |= a[i] ^ b[i]; + } + + return mismatch == 0; + } + + private string RemovePort(string url) + { + return SetPort(url, -1); + } + + private string AddPort(string url) + { + var uri = new UriBuilder(url); + return SetPort(url, uri.Port); + } + + private string SetPort(string url, int port) + { + var uri = new UriBuilder(url); + uri.Host = PreserveCase(url, uri.Host); + if (port == -1) + { + uri.Port = port; + } + else if ((port != 443) && (port != 80)) + { + uri.Port = port; + } + else + { + uri.Port = uri.Scheme == "https" ? 443 : 80; + } + var scheme = PreserveCase(url, uri.Scheme); + return uri.Uri.OriginalString.Replace(uri.Scheme, scheme); + } + + private string PreserveCase(string url, string replacementString) + { + var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); + return url.Substring(startIndex, replacementString.Length); + } + } +} diff --git a/test/Twilio.Benchmark/Twilio.Benchmark.csproj b/test/Twilio.Benchmark/Twilio.Benchmark.csproj new file mode 100644 index 000000000..81b91b034 --- /dev/null +++ b/test/Twilio.Benchmark/Twilio.Benchmark.csproj @@ -0,0 +1,17 @@ + + + + Exe + net7.0 + enable + disable + + + + + + + + + + From 43e433c820e9cdffe15984acc3c19f3035e60708 Mon Sep 17 00:00:00 2001 From: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com> Date: Tue, 3 Jan 2023 13:15:13 -0500 Subject: [PATCH 5/8] Reapply optimizations --- src/Twilio/Security/RequestValidator.cs | 410 ++++++++++++------------ 1 file changed, 208 insertions(+), 202 deletions(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index a05e54f0b..1c02affbf 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -1,83 +1,92 @@ -using System; -using System.Linq; -using System.Collections.Generic; -using System.Collections.Specialized; -using System.Security.Cryptography; -using System.Text; - -namespace Twilio.Security -{ - /// - /// Twilio request validator - /// - public class RequestValidator - { - private readonly string _secret; - - /// - /// Create a new RequestValidator - /// - /// Signing secret - public RequestValidator(string secret) - { - _secret = secret; - } - - /// - /// Validate against a request - /// - /// Request URL - /// Request parameters - /// Expected result - /// true if the signature matches the result; false otherwise - public bool Validate(string url, NameValueCollection parameters, string expected) - { - return Validate(url, ToDictionary(parameters), expected); - } - - /// - /// Validate against a request - /// - /// Request URL - /// Request parameters - /// Expected result - /// true if the signature matches the result; false otherwise - public bool Validate(string url, IDictionary parameters, string expected) - { - if (string.IsNullOrEmpty(url)) - throw new ArgumentException("Parameter 'url' cannot be null or empty."); - if (string.IsNullOrEmpty(expected)) - throw new ArgumentException("Parameter 'expected' cannot be null or empty."); - - if(parameters == null || parameters.Count == 0) +using System; +using System.Linq; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Security.Cryptography; +using System.Text; + +namespace Twilio.Security +{ + /// + /// Twilio request validator + /// + public class RequestValidator + { + private readonly byte[] _secret; + + /// + /// Create a new RequestValidator + /// + /// Signing secret + public RequestValidator(string secret) + { + _secret = Encoding.UTF8.GetBytes(secret); + } + + /// + /// Validate against a request + /// + /// Request URL + /// Request parameters + /// Expected result + /// true if the signature matches the result; false otherwise + public bool Validate(string url, NameValueCollection parameters, string expected) + { + return Validate(url, ToDictionary(parameters), expected); + } + + /// + /// Validate against a request + /// + /// Request URL + /// Request parameters + /// Expected result + /// true if the signature matches the result; false otherwise + public bool Validate(string url, IDictionary parameters, string expected) + { + if (string.IsNullOrEmpty(url)) + throw new ArgumentException("Parameter 'url' cannot be null or empty."); + if (string.IsNullOrEmpty(expected)) + throw new ArgumentException("Parameter 'expected' cannot be null or empty."); + +#if NET6_0_OR_GREATER { - var signature = GetValidationSignature(url); - if (SecureCompare(signature, expected)) return true; - - // check signature of url with and without port, since sig generation on back end is inconsistent - // If either url produces a valid signature, we accept the request as valid - url = GetUriVariation(url); - signature = GetValidationSignature(url); - if (SecureCompare(signature, expected)) return true; - return false; - } - else + byte[] computeHash(byte[] buffer) => HMACSHA1.HashData(_secret, buffer); +#else + using (var hmac = new HMACSHA1(_secret)) { - var parameterStringBuilder = GetJoinedParametersStringBuilder(parameters); - parameterStringBuilder.Insert(0, url); - var signature = GetValidationSignature(parameterStringBuilder.ToString()); - if (SecureCompare(signature, expected)) return true; - parameterStringBuilder.Remove(0, url.Length); - - // check signature of url with and without port, since sig generation on back end is inconsistent - // If either url produces a valid signature, we accept the request as valid - url = GetUriVariation(url); - parameterStringBuilder.Insert(0, url); - signature = GetValidationSignature(parameterStringBuilder.ToString()); - if (SecureCompare(signature, expected)) return true; + Func computeHash = hmac.ComputeHash; +#endif + if (parameters == null || parameters.Count == 0) + { + var signature = GetValidationSignature(url, computeHash); + if (SecureCompare(signature, expected)) return true; - return false; - } + // check signature of url with and without port, since sig generation on back end is inconsistent + // If either url produces a valid signature, we accept the request as valid + url = GetUriVariation(url); + signature = GetValidationSignature(url, computeHash); + if (SecureCompare(signature, expected)) return true; + return false; + } + else + { + var parameterStringBuilder = GetJoinedParametersStringBuilder(parameters); + parameterStringBuilder.Insert(0, url); + var signature = GetValidationSignature(parameterStringBuilder.ToString(), computeHash); + if (SecureCompare(signature, expected)) return true; + parameterStringBuilder.Remove(0, url.Length); + + // check signature of url with and without port, since sig generation on back end is inconsistent + // If either url produces a valid signature, we accept the request as valid + url = GetUriVariation(url); + parameterStringBuilder.Insert(0, url); + signature = GetValidationSignature(parameterStringBuilder.ToString(), computeHash); + if (SecureCompare(signature, expected)) return true; + + return false; + } + } } private StringBuilder GetJoinedParametersStringBuilder(IDictionary parameters) @@ -85,139 +94,136 @@ private StringBuilder GetJoinedParametersStringBuilder(IDictionary) null, expected) && ValidateBody(body, bodyHash); - } - - public static bool ValidateBody(string rawBody, string expected) - { - // TODO: In future, use net6's one-shot methods. - // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 + public bool Validate(string url, string body, string expected) + { + if (string.IsNullOrEmpty(url)) + throw new ArgumentException("Parameter 'url' cannot be null or empty."); + if (string.IsNullOrEmpty(expected)) + throw new ArgumentException("Parameter 'expected' cannot be null or empty."); + + var paramString = new Uri(url, UriKind.Absolute).Query.TrimStart('?'); + var bodyHash = ""; + foreach (var param in paramString.Split('&')) + { + var split = param.Split('='); + if (split[0] == "bodySHA256") + { + bodyHash = Uri.UnescapeDataString(split[1]); + } + } + + return Validate(url, (IDictionary)null, expected) && ValidateBody(body, bodyHash); + } + + public static bool ValidateBody(string rawBody, string expected) + { + // TODO: In future, use net6's one-shot methods. + // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 using (var sha = SHA256.Create()) { var signature = sha.ComputeHash(Encoding.UTF8.GetBytes(rawBody)); return SecureCompare(BitConverter.ToString(signature).Replace("-", "").ToLower(), expected); - } - } - - private static IDictionary ToDictionary(NameValueCollection col) - { - var dict = new Dictionary(); - foreach (var k in col.AllKeys) - { - dict.Add(k, col[k]); - } - - return dict; - } - - private string GetValidationSignature(string urlWithParameters) - { - // TODO: In future, use net6's one-shot methods. + } + } + + private static IDictionary ToDictionary(NameValueCollection col) + { + var dict = new Dictionary(); + foreach (var k in col.AllKeys) + { + dict.Add(k, col[k]); + } + + return dict; + } + + private string GetValidationSignature(string urlWithParameters, Func computeHash) + { + // TODO: In future, use net6's one-shot methods. // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 - using (var hmac = new HMACSHA1(Encoding.UTF8.GetBytes(_secret))) + var hash = computeHash(Encoding.UTF8.GetBytes(urlWithParameters)); + return Convert.ToBase64String(hash); + } + + private static bool SecureCompare(string a, string b) + { + if (a == null || b == null) + { + return false; + } + + var n = a.Length; + if (n != b.Length) + { + return false; + } + + var mismatch = 0; + for (var i = 0; i < n; i++) + { + mismatch |= a[i] ^ b[i]; + } + + return mismatch == 0; + } + + /// + /// Returns URL without port if given URL has port, returns URL with port if given URL has no port + /// + /// + /// + private static string GetUriVariation(string url) + { + var uri = new Uri(url); + var uriBuilder = new UriBuilder(uri); + var port = uri.GetComponents(UriComponents.Port, UriFormat.UriEscaped); + // if port already removed + if (port == "") + { + return SetPort(url, uriBuilder, uriBuilder.Port); + } + + return SetPort(url, uriBuilder, -1); + } + + private static string SetPort(string url, UriBuilder uri, int newPort) + { + if (newPort == -1) + { + uri.Port = newPort; + } + else if (newPort != 443 && newPort != 80) + { + uri.Port = newPort; + } + else { - var hash = hmac.ComputeHash(Encoding.UTF8.GetBytes(urlWithParameters)); - return Convert.ToBase64String(hash); - } - } - - private static bool SecureCompare(string a, string b) - { - if (a == null || b == null) - { - return false; - } - - var n = a.Length; - if (n != b.Length) - { - return false; - } - - var mismatch = 0; - for (var i = 0; i < n; i++) - { - mismatch |= a[i] ^ b[i]; - } - - return mismatch == 0; - } - - /// - /// Returns URL without port if given URL has port, returns URL with port if given URL has no port - /// - /// - /// - private static string GetUriVariation(string url) - { - var uri = new Uri(url); - var uriBuilder = new UriBuilder(uri); - var port = uri.GetComponents(UriComponents.Port, UriFormat.UriEscaped); - // if port already removed - if (port == "") - { - return SetPort(url, uriBuilder, uriBuilder.Port); - } - - return SetPort(url, uriBuilder, -1); - } - - private static string SetPort(string url, UriBuilder uri, int newPort) - { - if (newPort == -1) - { - uri.Port = newPort; - } - else if (newPort != 443 && newPort != 80) - { - uri.Port = newPort; - } - else - { - uri.Port = uri.Scheme == "https" ? 443 : 80; - } - - var uriStringBuilder = new StringBuilder(uri.ToString()); - - var host = PreserveCase(url, uri.Host); - uriStringBuilder.Replace(uri.Host, host); - - var scheme = PreserveCase(url, uri.Scheme); - uriStringBuilder.Replace(uri.Scheme, scheme); - - return uriStringBuilder.ToString(); - } - - private static string PreserveCase(string url, string replacementString) - { - var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); - return url.Substring(startIndex, replacementString.Length); - } - } + uri.Port = uri.Scheme == "https" ? 443 : 80; + } + + var uriStringBuilder = new StringBuilder(uri.ToString()); + + var host = PreserveCase(url, uri.Host); + uriStringBuilder.Replace(uri.Host, host); + + var scheme = PreserveCase(url, uri.Scheme); + uriStringBuilder.Replace(uri.Scheme, scheme); + + return uriStringBuilder.ToString(); + } + + private static string PreserveCase(string url, string replacementString) + { + var startIndex = url.IndexOf(replacementString, StringComparison.OrdinalIgnoreCase); + return url.Substring(startIndex, replacementString.Length); + } + } } \ No newline at end of file From c22e4afdb50be6ff2efd3b470cfcb293d6bd672b Mon Sep 17 00:00:00 2001 From: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com> Date: Tue, 3 Jan 2023 14:25:58 -0500 Subject: [PATCH 6/8] Add 6.0 TFM, empty to fix solution, update test packages --- src/Twilio/Twilio.csproj | 119 +++++++++++++++------------- test/Twilio.Test/Twilio.Test.csproj | 15 ++-- 2 files changed, 70 insertions(+), 64 deletions(-) diff --git a/src/Twilio/Twilio.csproj b/src/Twilio/Twilio.csproj index 1774b35ab..7434cfd67 100644 --- a/src/Twilio/Twilio.csproj +++ b/src/Twilio/Twilio.csproj @@ -1,59 +1,64 @@  - - netstandard1.4;netstandard2.0;net451;net35 - true - Twilio - Twilio REST API helper library - Copyright © Twilio - Twilio - en-US - 6.2.0 - - - Twilio - $(NoWarn);CS1591 - true - true - Twilio - REST;SMS;voice;telephony;phone;twilio;twiml;video;wireless;api - https://www.twilio.com/docs/static/company/img/logos/red/twilio-mark-red.898073bba.png - http://github.com/twilio/twilio-csharp - https://github.com/twilio/twilio-csharp/blob/HEAD/LICENSE - http://github.com/twilio/twilio-csharp - git - 1.6.1 - 2.0.0 - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + net6.0;netstandard1.4;netstandard2.0;net451;net35 + true + Twilio + Twilio REST API helper library + Copyright © Twilio + Twilio + en-US + 6.2.0 + + Twilio + $(NoWarn);CS1591 + true + true + Twilio + REST;SMS;voice;telephony;phone;twilio;twiml;video;wireless;api + https://www.twilio.com/docs/static/company/img/logos/red/twilio-mark-red.898073bba.png + http://github.com/twilio/twilio-csharp + https://github.com/twilio/twilio-csharp/blob/HEAD/LICENSE + http://github.com/twilio/twilio-csharp + git + 1.6.1 + 2.0.0 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/Twilio.Test/Twilio.Test.csproj b/test/Twilio.Test/Twilio.Test.csproj index 3b44e7ec9..84e3d6cd0 100644 --- a/test/Twilio.Test/Twilio.Test.csproj +++ b/test/Twilio.Test/Twilio.Test.csproj @@ -2,21 +2,22 @@ Exe Twilio.Tests - netcoreapp2.0;net451;net35 + net6.0;net451;net35 win7-x86 win7-x86 false + false - + runtime; build; native; contentfiles; analyzers; buildtransitive all - - - - - + + + + + From 7bf745b05a440a1ddae9dbf7b2f5f10211e7fda0 Mon Sep 17 00:00:00 2001 From: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com> Date: Tue, 3 Jan 2023 14:26:17 -0500 Subject: [PATCH 7/8] Stop SecureCompare from being optimized --- src/Twilio/Security/RequestValidator.cs | 2 ++ test/Twilio.Benchmark/RequestValidatorOriginal.cs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index 1c02affbf..8afdebc57 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -4,6 +4,7 @@ using System.Collections.Specialized; using System.Security.Cryptography; using System.Text; +using System.Runtime.CompilerServices; namespace Twilio.Security { @@ -153,6 +154,7 @@ private string GetValidationSignature(string urlWithParameters, Func pa return Convert.ToBase64String(hash); } + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] private static bool SecureCompare(string a, string b) { if (a == null || b == null) From 7181df520eca52bb18f7322eb396f908f6a9c2d6 Mon Sep 17 00:00:00 2001 From: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com> Date: Fri, 3 Feb 2023 11:54:07 -0500 Subject: [PATCH 8/8] Use oneshot hash method for ValidateBody --- src/Twilio/Security/RequestValidator.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/Twilio/Security/RequestValidator.cs b/src/Twilio/Security/RequestValidator.cs index 8afdebc57..8a1dcca1c 100644 --- a/src/Twilio/Security/RequestValidator.cs +++ b/src/Twilio/Security/RequestValidator.cs @@ -46,9 +46,9 @@ public bool Validate(string url, NameValueCollection parameters, string expected public bool Validate(string url, IDictionary parameters, string expected) { if (string.IsNullOrEmpty(url)) - throw new ArgumentException("Parameter 'url' cannot be null or empty."); + throw new ArgumentException("Parameter 'url' cannot be null or empty.", nameof(url)); if (string.IsNullOrEmpty(expected)) - throw new ArgumentException("Parameter 'expected' cannot be null or empty."); + throw new ArgumentException("Parameter 'expected' cannot be null or empty.", nameof(url)); #if NET6_0_OR_GREATER { @@ -100,15 +100,16 @@ private StringBuilder GetJoinedParametersStringBuilder(IDictionary ToDictionary(NameValueCollection col) private string GetValidationSignature(string urlWithParameters, Func computeHash) { - // TODO: In future, use net6's one-shot methods. - // See: https://github.com/twilio/twilio-csharp/issues/466#issuecomment-1028091370 var hash = computeHash(Encoding.UTF8.GetBytes(urlWithParameters)); return Convert.ToBase64String(hash); }