From f0add817059e3aea3b8eee174136e7c7c47e1a7e Mon Sep 17 00:00:00 2001 From: nan01ab Date: Fri, 4 Oct 2024 22:21:21 +0800 Subject: [PATCH 1/4] fix: pass compare should use contant time compare to avoid timing attack --- src/Neo.Extensions/StringExtensions.cs | 25 +++++++++++++++++++ src/Plugins/RpcServer/RpcServer.cs | 3 ++- .../UT_StringExtensions.cs | 24 ++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/Neo.Extensions/StringExtensions.cs b/src/Neo.Extensions/StringExtensions.cs index b84869b2e1..bac4267252 100644 --- a/src/Neo.Extensions/StringExtensions.cs +++ b/src/Neo.Extensions/StringExtensions.cs @@ -43,5 +43,30 @@ public static int GetVarSize(this string value) var size = Utility.StrictUTF8.GetByteCount(value); return UnsafeData.GetVarSize(size) + size; } + + /// + /// Compares two s for equality in constant time. + /// + /// The left . + /// The right . + /// True if the two s are equal, false otherwise. + public static bool ConstantTimeEquals(this string left, string right) + { + if (left == null && right == null) + return true; + + if (left == null || right == null) + return false; + + if (left.Length != right.Length) + return false; + + var lhs = left.AsSpan(); + var rhs = right.AsSpan(); + var result = 0; + for (var i = 0; i < lhs.Length; i++) + result |= lhs[i] ^ rhs[i]; + return result == 0; + } } } diff --git a/src/Plugins/RpcServer/RpcServer.cs b/src/Plugins/RpcServer/RpcServer.cs index c90bbad539..1e0f3c5e6c 100644 --- a/src/Plugins/RpcServer/RpcServer.cs +++ b/src/Plugins/RpcServer/RpcServer.cs @@ -16,6 +16,7 @@ using Microsoft.AspNetCore.ResponseCompression; using Microsoft.AspNetCore.Server.Kestrel.Https; using Microsoft.Extensions.DependencyInjection; +using Neo.Extensions; using Neo.Json; using Neo.Network.P2P; using System; @@ -79,7 +80,7 @@ internal bool CheckAuth(HttpContext context) if (authvalues.Length < 2) return false; - return authvalues[0] == settings.RpcUser && authvalues[1] == settings.RpcPass; + return authvalues[0].ConstantTimeEquals(settings.RpcUser) && authvalues[1].ConstantTimeEquals(settings.RpcPass); } private static JObject CreateErrorResponse(JToken id, RpcError rpcError) diff --git a/tests/Neo.Extensions.Tests/UT_StringExtensions.cs b/tests/Neo.Extensions.Tests/UT_StringExtensions.cs index 6e823d2493..6c2c81b388 100644 --- a/tests/Neo.Extensions.Tests/UT_StringExtensions.cs +++ b/tests/Neo.Extensions.Tests/UT_StringExtensions.cs @@ -42,5 +42,29 @@ public void TestGetVarSizeString() int result = "AA".GetVarSize(); Assert.AreEqual(3, result); } + + [TestMethod] + public void TestConstantTimeEquals() + { + string str1 = "AA"; + string str2 = "AA"; + str1.ConstantTimeEquals(str2).Should().BeTrue(); + + str1 = "AB"; + str2 = "AA"; + str1.ConstantTimeEquals(str2).Should().BeFalse(); + + str1 = "AB"; + str2 = null; + str1.ConstantTimeEquals(str2).Should().BeFalse(); + + str1 = null; + str2 = "AA"; + str1.ConstantTimeEquals(str2).Should().BeFalse(); + + str1 = null; + str2 = null; + str1.ConstantTimeEquals(str2).Should().BeTrue(); + } } } From 0a5e52cf14d9cee60822b278c80516cd0898bb56 Mon Sep 17 00:00:00 2001 From: nan01ab Date: Sun, 6 Oct 2024 10:36:37 +0800 Subject: [PATCH 2/4] fix: pass compare should use contant time compare to avoid timing attack --- src/Neo.Extensions/StringExtensions.cs | 25 ----------- src/Plugins/RpcServer/RpcServer.cs | 22 +++++++--- .../UT_StringExtensions.cs | 24 ----------- .../UT_RpcServer.cs | 42 +++++++++++++++++++ 4 files changed, 59 insertions(+), 54 deletions(-) diff --git a/src/Neo.Extensions/StringExtensions.cs b/src/Neo.Extensions/StringExtensions.cs index bac4267252..b84869b2e1 100644 --- a/src/Neo.Extensions/StringExtensions.cs +++ b/src/Neo.Extensions/StringExtensions.cs @@ -43,30 +43,5 @@ public static int GetVarSize(this string value) var size = Utility.StrictUTF8.GetByteCount(value); return UnsafeData.GetVarSize(size) + size; } - - /// - /// Compares two s for equality in constant time. - /// - /// The left . - /// The right . - /// True if the two s are equal, false otherwise. - public static bool ConstantTimeEquals(this string left, string right) - { - if (left == null && right == null) - return true; - - if (left == null || right == null) - return false; - - if (left.Length != right.Length) - return false; - - var lhs = left.AsSpan(); - var rhs = right.AsSpan(); - var result = 0; - for (var i = 0; i < lhs.Length; i++) - result |= lhs[i] ^ rhs[i]; - return result == 0; - } } } diff --git a/src/Plugins/RpcServer/RpcServer.cs b/src/Plugins/RpcServer/RpcServer.cs index 1e0f3c5e6c..46d04f4168 100644 --- a/src/Plugins/RpcServer/RpcServer.cs +++ b/src/Plugins/RpcServer/RpcServer.cs @@ -27,6 +27,7 @@ using System.Linq.Expressions; using System.Net.Security; using System.Reflection; +using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Text; using System.Threading.Tasks; @@ -45,10 +46,18 @@ public partial class RpcServer : IDisposable private readonly NeoSystem system; private readonly LocalNode localNode; + // avoid GetBytes every time + private readonly byte[] _rpcUser; + private readonly byte[] _rpcPass; + public RpcServer(NeoSystem system, RpcServerSettings settings) { this.system = system; this.settings = settings; + + _rpcUser = settings.RpcUser is not null ? Encoding.UTF8.GetBytes(settings.RpcUser) : []; + _rpcPass = settings.RpcPass is not null ? Encoding.UTF8.GetBytes(settings.RpcPass) : []; + localNode = system.LocalNode.Ask(new LocalNode.GetInstance()).Result; RegisterMethods(this); Initialize_SmartContract(); @@ -66,21 +75,24 @@ internal bool CheckAuth(HttpContext context) return false; } - string authstring; + byte[] auths; try { - authstring = Encoding.UTF8.GetString(Convert.FromBase64String(reqauth.Replace("Basic ", "").Trim())); + auths = Convert.FromBase64String(reqauth.Replace("Basic ", "").Trim()); } catch { return false; } - string[] authvalues = authstring.Split(new string[] { ":" }, StringSplitOptions.RemoveEmptyEntries); - if (authvalues.Length < 2) + int colonIndex = Array.IndexOf(auths, (byte)':'); + if (colonIndex == -1) return false; - return authvalues[0].ConstantTimeEquals(settings.RpcUser) && authvalues[1].ConstantTimeEquals(settings.RpcPass); + byte[] user = auths[..colonIndex]; + byte[] pass = auths[(colonIndex + 1)..]; + + return CryptographicOperations.FixedTimeEquals(user, _rpcUser) && CryptographicOperations.FixedTimeEquals(pass, _rpcPass); } private static JObject CreateErrorResponse(JToken id, RpcError rpcError) diff --git a/tests/Neo.Extensions.Tests/UT_StringExtensions.cs b/tests/Neo.Extensions.Tests/UT_StringExtensions.cs index 6c2c81b388..6e823d2493 100644 --- a/tests/Neo.Extensions.Tests/UT_StringExtensions.cs +++ b/tests/Neo.Extensions.Tests/UT_StringExtensions.cs @@ -42,29 +42,5 @@ public void TestGetVarSizeString() int result = "AA".GetVarSize(); Assert.AreEqual(3, result); } - - [TestMethod] - public void TestConstantTimeEquals() - { - string str1 = "AA"; - string str2 = "AA"; - str1.ConstantTimeEquals(str2).Should().BeTrue(); - - str1 = "AB"; - str2 = "AA"; - str1.ConstantTimeEquals(str2).Should().BeFalse(); - - str1 = "AB"; - str2 = null; - str1.ConstantTimeEquals(str2).Should().BeFalse(); - - str1 = null; - str2 = "AA"; - str1.ConstantTimeEquals(str2).Should().BeFalse(); - - str1 = null; - str2 = null; - str1.ConstantTimeEquals(str2).Should().BeTrue(); - } } } diff --git a/tests/Neo.Plugins.RpcServer.Tests/UT_RpcServer.cs b/tests/Neo.Plugins.RpcServer.Tests/UT_RpcServer.cs index b63380f803..b8425ee0a9 100644 --- a/tests/Neo.Plugins.RpcServer.Tests/UT_RpcServer.cs +++ b/tests/Neo.Plugins.RpcServer.Tests/UT_RpcServer.cs @@ -87,5 +87,47 @@ public void TestCheckAuth_ValidCredentials_ReturnsTrue() // Assert Assert.IsTrue(result); } + + [TestMethod] + public void TestCheckAuth() + { + var memoryStoreProvider = new TestMemoryStoreProvider(new MemoryStore()); + var neoSystem = new NeoSystem(TestProtocolSettings.SoleNode, memoryStoreProvider); + var rpcServerSettings = RpcServerSettings.Default with + { + SessionEnabled = true, + SessionExpirationTime = TimeSpan.FromSeconds(0.3), + MaxGasInvoke = 1500_0000_0000, + Network = TestProtocolSettings.SoleNode.Network, + RpcUser = "testuser", + RpcPass = "testpass", + }; + var rpcServer = new RpcServer(neoSystem, rpcServerSettings); + + var context = new DefaultHttpContext(); + context.Request.Headers["Authorization"] = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes("testuser:testpass")); + var result = rpcServer.CheckAuth(context); + Assert.IsTrue(result); + + context.Request.Headers["Authorization"] = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes("testuser:wrongpass")); + result = rpcServer.CheckAuth(context); + Assert.IsFalse(result); + + context.Request.Headers["Authorization"] = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes("wronguser:testpass")); + result = rpcServer.CheckAuth(context); + Assert.IsFalse(result); + + context.Request.Headers["Authorization"] = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes("testuser:")); + result = rpcServer.CheckAuth(context); + Assert.IsFalse(result); + + context.Request.Headers["Authorization"] = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes(":testpass")); + result = rpcServer.CheckAuth(context); + Assert.IsFalse(result); + + context.Request.Headers["Authorization"] = "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes("")); + result = rpcServer.CheckAuth(context); + Assert.IsFalse(result); + } } } From 1f3483f9b5b6baf6247d8e02369a6a8b23bdbcb3 Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 8 Oct 2024 01:14:51 -0700 Subject: [PATCH 3/4] Update src/Plugins/RpcServer/RpcServer.cs --- src/Plugins/RpcServer/RpcServer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Plugins/RpcServer/RpcServer.cs b/src/Plugins/RpcServer/RpcServer.cs index 46d04f4168..5b6e3e82ee 100644 --- a/src/Plugins/RpcServer/RpcServer.cs +++ b/src/Plugins/RpcServer/RpcServer.cs @@ -92,7 +92,8 @@ internal bool CheckAuth(HttpContext context) byte[] user = auths[..colonIndex]; byte[] pass = auths[(colonIndex + 1)..]; - return CryptographicOperations.FixedTimeEquals(user, _rpcUser) && CryptographicOperations.FixedTimeEquals(pass, _rpcPass); + // Execute both checks always but both must be true + return CryptographicOperations.FixedTimeEquals(user, _rpcUser) & CryptographicOperations.FixedTimeEquals(pass, _rpcPass); } private static JObject CreateErrorResponse(JToken id, RpcError rpcError) From cccd7fc4c7bb40ed3776381b2f3cee1bbcaa9add Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 8 Oct 2024 01:15:35 -0700 Subject: [PATCH 4/4] Update src/Plugins/RpcServer/RpcServer.cs --- src/Plugins/RpcServer/RpcServer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Plugins/RpcServer/RpcServer.cs b/src/Plugins/RpcServer/RpcServer.cs index 5b6e3e82ee..0dc3d467bf 100644 --- a/src/Plugins/RpcServer/RpcServer.cs +++ b/src/Plugins/RpcServer/RpcServer.cs @@ -92,7 +92,7 @@ internal bool CheckAuth(HttpContext context) byte[] user = auths[..colonIndex]; byte[] pass = auths[(colonIndex + 1)..]; - // Execute both checks always but both must be true + // Always execute both checks, but both must evaluate to true return CryptographicOperations.FixedTimeEquals(user, _rpcUser) & CryptographicOperations.FixedTimeEquals(pass, _rpcPass); }