From c37d503bd1ec2c723242800b129e152116f2a15a Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Sat, 4 May 2024 16:51:16 +0300 Subject: [PATCH] SmartContract: catch exception on multisignature contract parsing IsMultiSigContract should return proper true/false result even if some garbage is provided as an input. Without this commit an exception is possible during IsMultiSigContract execution during subsequent public key decoding from compressed form: ``` Failed TestIsMultiSigContract [16 ms] Error Message: Test method Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract threw exception: System.FormatException: Invalid point encoding 221 Stack Trace: at Neo.Cryptography.ECC.ECPoint.DecodePoint(ReadOnlySpan`1 encoded, ECCurve curve) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/Cryptography/ECC/ECPoint.cs:line 98 at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script, Int32& m, Int32& n, List`1 points) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 189 at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 123 at Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract() in /home/anna/Documents/GitProjects/neo-project/neo/tests/Neo.UnitTests/SmartContract/UT_Helper.cs:line 65 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor) at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) ``` Note that this change is compatible wrt the callers' behaviour. We need this change to properly handle non-Secp256r1-based witness scripts, ref. https://github.com/neo-project/neo/pull/3209. Signed-off-by: Anna Shaleva --- src/Neo/SmartContract/Helper.cs | 9 +++- .../Neo.UnitTests/SmartContract/UT_Helper.cs | 43 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/Neo/SmartContract/Helper.cs b/src/Neo/SmartContract/Helper.cs index 333ef77d7b..c0c694fc4e 100644 --- a/src/Neo/SmartContract/Helper.cs +++ b/src/Neo/SmartContract/Helper.cs @@ -184,7 +184,14 @@ private static bool IsMultiSigContract(ReadOnlySpan script, out int m, out { if (script.Length <= i + 35) return false; if (script[++i] != 33) return false; - points?.Add(ECPoint.DecodePoint(script.Slice(i + 1, 33), ECCurve.Secp256r1)); + try + { + points?.Add(ECPoint.DecodePoint(script.Slice(i + 1, 33), ECCurve.Secp256r1)); + } + catch (Exception) // Script may contain any data, thus exceptions are allowed on point decoding. + { + return false; + } i += 34; ++n; } diff --git a/tests/Neo.UnitTests/SmartContract/UT_Helper.cs b/tests/Neo.UnitTests/SmartContract/UT_Helper.cs index 3d682a4ee9..ffffc70728 100644 --- a/tests/Neo.UnitTests/SmartContract/UT_Helper.cs +++ b/tests/Neo.UnitTests/SmartContract/UT_Helper.cs @@ -17,6 +17,8 @@ using Neo.VM; using Neo.Wallets; using System; +using System.Collections.Generic; +using System.Linq; using static Neo.SmartContract.Helper; namespace Neo.UnitTests.SmartContract @@ -72,6 +74,47 @@ public void TestIsMultiSigContract() Assert.IsFalse(IsMultiSigContract(case2)); } + [TestMethod] + // TestIsMultiSigContract_WrongCurve checks that multisignature verification script based on points + // not from Secp256r1 curve fails IsMultiSigContract check without any exception. + public void TestIsMultiSigContract_WrongCurve() + { + // A set of points on Koblitz curve in uncompressed representation. One of the points + // (the first one) is specially selected, this point can't be restored on Secp256r1 + // from compressed form, whereas three other points can be restored on both Secp256r1 + // and Koblitz curves. + var pubs = new List() + { + ECPoint.Parse("047b4e72ae854b6a0955b3e02d92651ab7fa641a936066776ad438f95bb674a269a63ff98544691663d91a6cfcd215831f01bfb7a226363a6c5c67ef14541dba07", ECCurve.Secp256k1), + ECPoint.Parse("040486468683c112125978ffe876245b2006bfe739aca8539b67335079262cb27ad0dedc9e5583f99b61c6f46bf80b97eaec3654b87add0e5bd7106c69922a229d", ECCurve.Secp256k1), + ECPoint.Parse("040d26fc2ad3b1aae20f040b5f83380670f8ef5c2b2ac921ba3bdd79fd0af0525177715fd4370b1012ddd10579698d186ab342c223da3e884ece9cab9b6638c7bb", ECCurve.Secp256k1), + ECPoint.Parse("04a114d72fe2997cdac67427b6f39ea08ed46213c8bb6a461bbac2a6212cf43fb510f8adf59b0b087a7859f96d0288e5e94800eab8388f30f03f92b2e4d807dfce", ECCurve.Secp256k1) + }; + const int m = 3; + + var badScript = Contract.CreateMultiSigRedeemScript(m, pubs); + Assert.IsFalse(IsMultiSigContract(badScript, out _, out ECPoint[] _)); // enforce runtime point decoding by specifying ECPoint[] out variable. + Assert.IsTrue(IsMultiSigContract(badScript)); // this overload is unlucky since it doesn't perform ECPoint decoding. + + // Exclude the first special point and check one more time, both methods should return true. + var goodScript = Contract.CreateMultiSigRedeemScript(m, pubs.Skip(1).ToArray()); + Assert.IsTrue(IsMultiSigContract(goodScript, out _, out ECPoint[] _)); // enforce runtime point decoding by specifying ECPoint[] out variable. + Assert.IsTrue(IsMultiSigContract(goodScript)); // this overload is unlucky since it doesn't perform ECPoint decoding. + } + + [TestMethod] + // TestIsSignatureContract_WrongCurve checks that signature verification script based on point + // not from Secp256r1 curve passes IsSignatureContract check without any exception. + public void TestIsSignatureContract_WrongCurve() + { + // A special point on Koblitz curve that can't be restored at Secp256r1 from compressed form. + var pub = ECPoint.Parse("047b4e72ae854b6a0955b3e02d92651ab7fa641a936066776ad438f95bb674a269a63ff98544691663d91a6cfcd215831f01bfb7a226363a6c5c67ef14541dba07", ECCurve.Secp256k1); + var script = Contract.CreateSignatureRedeemScript(pub); + + // IsSignatureContract should pass since it doesn't perform ECPoint decoding. + Assert.IsTrue(IsSignatureContract(script)); + } + [TestMethod] public void TestSignatureContractCost() {