SmartContract: catch exception on multisignature contract parsing#3211
SmartContract: catch exception on multisignature contract parsing#3211
Conversation
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.
#3209.
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
vncoelho
left a comment
There was a problem hiding this comment.
I think it is not "(non-breaking change which fixes an issue)", @AnnaShaleva .
Perhaps that without a proper version control an attacker could try to use this function and change behavior of past executions.
But I did not test yet.
cschuchardt88
left a comment
There was a problem hiding this comment.
Please write test for IVerifable.VerifyStateIndependent and IVeriable.VerifyWitnesses just to make sure they work with changes.
neo/src/Neo/SmartContract/Helper.cs
Line 283 in 429a081
vncoelho
left a comment
There was a problem hiding this comment.
I double checked here,
perhaps it does not need version control. In fact, it looks like it is just internal for fee calculations and transaction creation. Thus, it would not affect the understanding of previous transactions.
roman-khimov
left a comment
There was a problem hiding this comment.
Correct fix (and likely more important than just to "properly handle non-Secp256r1-based witness scripts"), but I fear we may have more cases like this in the codebase (like VerifySignature).
Hints my tests for |
Description
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:
Note that this change is compatible wrt the callers' behaviour. We need this change to properly handle non-Secp256r1-based witness scripts, ref. #3209.
Type of change
How Has This Been Tested?
TestIsMultiSigContract_WrongCurveTestIsSignatureContract_WrongCurveChecklist: