From 98e834bfd1409915dd288db85818c6cd64bef6d7 Mon Sep 17 00:00:00 2001 From: nan01ab Date: Sat, 21 Sep 2024 22:44:47 +0800 Subject: [PATCH 1/9] fix: int overflow in SubStr --- .gitignore | 4 ++ src/Neo.VM/JumpTable/JumpTable.Splice.cs | 17 +++++-- .../Tests/OpCodes/Splice/SUBSTR.json | 44 +++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index 1358af3bde..e589e69dbb 100644 --- a/.gitignore +++ b/.gitignore @@ -257,3 +257,7 @@ paket-files/ PublishProfiles /.vscode launchSettings.json + +# UT coverage report +/coverages +**/.DS_Store \ No newline at end of file diff --git a/src/Neo.VM/JumpTable/JumpTable.Splice.cs b/src/Neo.VM/JumpTable/JumpTable.Splice.cs index f04d045988..53c4d44074 100644 --- a/src/Neo.VM/JumpTable/JumpTable.Splice.cs +++ b/src/Neo.VM/JumpTable/JumpTable.Splice.cs @@ -95,13 +95,22 @@ public virtual void SubStr(ExecutionEngine engine, Instruction instruction) { int count = (int)engine.Pop().GetInteger(); if (count < 0) - throw new InvalidOperationException($"The value {count} is out of range."); + throw new InvalidOperationException($"The value of count {count} is out of range."); + int index = (int)engine.Pop().GetInteger(); if (index < 0) - throw new InvalidOperationException($"The value {index} is out of range."); + throw new InvalidOperationException($"The value of index {index} is out of range."); + var x = engine.Pop().GetSpan(); - if (index + count > x.Length) - throw new InvalidOperationException($"The value {count} is out of range."); + if (count > x.Length) + throw new InvalidOperationException($"The value of count {count} is out of range."); + + if (index >= x.Length) + throw new InvalidOperationException($"The value of index {index} is out of range."); + + if (checked(index + count) > x.Length) + throw new InvalidOperationException($"The value of index + count {index + count} is out of range."); + Types.Buffer result = new(count, false); x.Slice(index, count).CopyTo(result.InnerBuffer.Span); engine.Push(result); diff --git a/tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json b/tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json index 7b78d7e053..13bf8b9b59 100644 --- a/tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json +++ b/tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json @@ -473,6 +473,50 @@ } } ] + }, + { + "name": "Count Exceed Range Test", + "script": [ + "PUSHDATA1", + "0x0a", + "0x00010203040506070809", + "PUSH2", + "PUSHINT32", + "0x7FFFFFFF", + "SUBSTR" + ], + "steps": [ + { + "actions": [ + "execute" + ], + "result": { + "state": "FAULT" + } + } + ] + }, + { + "name": "Index Exceed Range Test", + "script": [ + "PUSHDATA1", + "0x0a", + "0x00010203040506070809", + "PUSHINT32", + "0x7FFFFFFF", + "PUSH2", + "SUBSTR" + ], + "steps": [ + { + "actions": [ + "execute" + ], + "result": { + "state": "FAULT" + } + } + ] } ] } From 4a3c207021be009b58a82420dd78db40f80b6335 Mon Sep 17 00:00:00 2001 From: nan01ab Date: Mon, 23 Sep 2024 20:46:09 +0800 Subject: [PATCH 2/9] fix: int overflow in SubStr --- src/Neo.VM/JumpTable/JumpTable.Splice.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Neo.VM/JumpTable/JumpTable.Splice.cs b/src/Neo.VM/JumpTable/JumpTable.Splice.cs index 53c4d44074..472c9c4c4f 100644 --- a/src/Neo.VM/JumpTable/JumpTable.Splice.cs +++ b/src/Neo.VM/JumpTable/JumpTable.Splice.cs @@ -102,14 +102,8 @@ public virtual void SubStr(ExecutionEngine engine, Instruction instruction) throw new InvalidOperationException($"The value of index {index} is out of range."); var x = engine.Pop().GetSpan(); - if (count > x.Length) - throw new InvalidOperationException($"The value of count {count} is out of range."); - - if (index >= x.Length) - throw new InvalidOperationException($"The value of index {index} is out of range."); - if (checked(index + count) > x.Length) - throw new InvalidOperationException($"The value of index + count {index + count} is out of range."); + throw new InvalidOperationException($"The value of index {index} + count {count} is out of range."); Types.Buffer result = new(count, false); x.Slice(index, count).CopyTo(result.InnerBuffer.Span); From 40e5c1fe275a418df2eabe3f4faf744dd2b13c37 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Wed, 20 Nov 2024 10:30:17 +0800 Subject: [PATCH 3/9] format --- src/Neo.VM/JumpTable/JumpTable.Splice.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Neo.VM/JumpTable/JumpTable.Splice.cs b/src/Neo.VM/JumpTable/JumpTable.Splice.cs index 2d4370bb7a..0a56099b2f 100644 --- a/src/Neo.VM/JumpTable/JumpTable.Splice.cs +++ b/src/Neo.VM/JumpTable/JumpTable.Splice.cs @@ -101,7 +101,7 @@ public virtual void SubStr(ExecutionEngine engine, Instruction instruction) if (index < 0) throw new InvalidOperationException($"The index can not be negative for {nameof(OpCode.SUBSTR)}, index: {index}."); var x = engine.Pop().GetSpan(); - if (checked(index + count) > x.Length) + if (checked(index + count) > x.Length) throw new InvalidOperationException($"The index + count is out of range for {nameof(OpCode.SUBSTR)}, index: {index}, count: {count}, {index + count}/[0, {x.Length}]."); Types.Buffer result = new(count, false); x.Slice(index, count).CopyTo(result.InnerBuffer.Span); From 4772329cd7fc7e010f46b3b4981db82c553ef3d8 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Thu, 21 Nov 2024 19:33:26 +0100 Subject: [PATCH 4/9] Versioning change --- src/Neo/SmartContract/ApplicationEngine.cs | 46 +++++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs index 7808baae5e..b87314cf72 100644 --- a/src/Neo/SmartContract/ApplicationEngine.cs +++ b/src/Neo/SmartContract/ApplicationEngine.cs @@ -399,13 +399,55 @@ internal override void UnloadContext(ExecutionContext context) /// The engine instance created. public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas, IDiagnostic diagnostic = null) { - // Adjust jump table according persistingBlock - var jumpTable = ApplicationEngine.DefaultJumpTable; + var jumpTable = GetJumpTable(settings, persistingBlock?.Index ?? NativeContract.Ledger.CurrentIndex(snapshot)); return Provider?.Create(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable) ?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable); } + /// + /// Adjust jump table according persistingBlock + /// + /// The used by the engine. + /// Block index + /// + public static JumpTable GetJumpTable(ProtocolSettings settings, uint index) + { + if (settings.IsHardforkEnabled(Hardfork.HF_Echidna, index)) + { + var jumpTable = ComposeDefaultJumpTable(); + jumpTable[OpCode.SUBSTR] = VulnerableSubStr; + return jumpTable; + } + + return ApplicationEngine.DefaultJumpTable; + } + + /// + /// Extracts a substring from the specified buffer and pushes it onto the evaluation stack. + /// + /// + /// The execution engine. + /// The instruction being executed. + /// Pop 3, Push 1 + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static void VulnerableSubStr(ExecutionEngine engine, Instruction instruction) + { + var count = (int)engine.Pop().GetInteger(); + if (count < 0) + throw new InvalidOperationException($"The count can not be negative for {nameof(OpCode.SUBSTR)}, count: {count}."); + var index = (int)engine.Pop().GetInteger(); + if (index < 0) + throw new InvalidOperationException($"The index can not be negative for {nameof(OpCode.SUBSTR)}, index: {index}."); + var x = engine.Pop().GetSpan(); + if (index + count > x.Length) + throw new InvalidOperationException($"The index + count is out of range for {nameof(OpCode.SUBSTR)}, index: {index}, count: {count}, {index + count}/[0, {x.Length}]."); + + VM.Types.Buffer result = new(count, false); + x.Slice(index, count).CopyTo(result.InnerBuffer.Span); + engine.Push(result); + } + public override void LoadContext(ExecutionContext context) { // Set default execution context state From 5f159d80c4c7a8e3da617f4a140570d48588ac35 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Thu, 21 Nov 2024 19:38:37 +0100 Subject: [PATCH 5/9] Clean --- src/Neo/SmartContract/ApplicationEngine.cs | 33 +++++++++------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs index b87314cf72..7fdf9e2f5c 100644 --- a/src/Neo/SmartContract/ApplicationEngine.cs +++ b/src/Neo/SmartContract/ApplicationEngine.cs @@ -34,6 +34,7 @@ namespace Neo.SmartContract public partial class ApplicationEngine : ExecutionEngine { protected static readonly JumpTable DefaultJumpTable = ComposeDefaultJumpTable(); + protected static readonly JumpTable NotEchidnaJumpTable = ComposeEchidnaJumpTable(); /// /// The maximum cost that can be spent when a contract is executed in test mode. @@ -214,6 +215,13 @@ private static JumpTable ComposeDefaultJumpTable() return table; } + public static JumpTable ComposeEchidnaJumpTable() + { + var jumpTable = ComposeDefaultJumpTable(); + jumpTable[OpCode.SUBSTR] = VulnerableSubStr; + return jumpTable; + } + protected static void OnCallT(ExecutionEngine engine, Instruction instruction) { if (engine is ApplicationEngine app) @@ -399,30 +407,15 @@ internal override void UnloadContext(ExecutionContext context) /// The engine instance created. public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas, IDiagnostic diagnostic = null) { - var jumpTable = GetJumpTable(settings, persistingBlock?.Index ?? NativeContract.Ledger.CurrentIndex(snapshot)); + var index = persistingBlock?.Index ?? NativeContract.Ledger.CurrentIndex(snapshot); - return Provider?.Create(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable) - ?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable); - } + // Adjust jump table according persistingBlock - /// - /// Adjust jump table according persistingBlock - /// - /// The used by the engine. - /// Block index - /// - public static JumpTable GetJumpTable(ProtocolSettings settings, uint index) - { - if (settings.IsHardforkEnabled(Hardfork.HF_Echidna, index)) - { - var jumpTable = ComposeDefaultJumpTable(); - jumpTable[OpCode.SUBSTR] = VulnerableSubStr; - return jumpTable; - } + var jumpTable = settings.IsHardforkEnabled(Hardfork.HF_Echidna, index) ? DefaultJumpTable : NotEchidnaJumpTable; - return ApplicationEngine.DefaultJumpTable; + return Provider?.Create(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable) + ?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable); } - /// /// Extracts a substring from the specified buffer and pushes it onto the evaluation stack. /// From 7864e0aac6f736e8f6b4533e2a61fc2418488e18 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Thu, 21 Nov 2024 19:39:16 +0100 Subject: [PATCH 6/9] Rename --- src/Neo/SmartContract/ApplicationEngine.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs index 7fdf9e2f5c..b85cb6ca4a 100644 --- a/src/Neo/SmartContract/ApplicationEngine.cs +++ b/src/Neo/SmartContract/ApplicationEngine.cs @@ -34,7 +34,7 @@ namespace Neo.SmartContract public partial class ApplicationEngine : ExecutionEngine { protected static readonly JumpTable DefaultJumpTable = ComposeDefaultJumpTable(); - protected static readonly JumpTable NotEchidnaJumpTable = ComposeEchidnaJumpTable(); + protected static readonly JumpTable NotEchidnaJumpTable = ComposeNotEchidnaJumpTable(); /// /// The maximum cost that can be spent when a contract is executed in test mode. @@ -215,7 +215,7 @@ private static JumpTable ComposeDefaultJumpTable() return table; } - public static JumpTable ComposeEchidnaJumpTable() + public static JumpTable ComposeNotEchidnaJumpTable() { var jumpTable = ComposeDefaultJumpTable(); jumpTable[OpCode.SUBSTR] = VulnerableSubStr; From 20daa54d09957867bc2a0cd5d5cbdbb00eb01952 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Thu, 21 Nov 2024 19:39:59 +0100 Subject: [PATCH 7/9] Show change --- src/Neo/SmartContract/ApplicationEngine.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs index b85cb6ca4a..e0047b03ff 100644 --- a/src/Neo/SmartContract/ApplicationEngine.cs +++ b/src/Neo/SmartContract/ApplicationEngine.cs @@ -433,6 +433,7 @@ private static void VulnerableSubStr(ExecutionEngine engine, Instruction instruc if (index < 0) throw new InvalidOperationException($"The index can not be negative for {nameof(OpCode.SUBSTR)}, index: {index}."); var x = engine.Pop().GetSpan(); + // Note: here it's the main change if (index + count > x.Length) throw new InvalidOperationException($"The index + count is out of range for {nameof(OpCode.SUBSTR)}, index: {index}, count: {count}, {index + count}/[0, {x.Length}]."); From 2bf5125a7be7afa9392909dcee9b9c56e4877c04 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Thu, 21 Nov 2024 19:41:33 +0100 Subject: [PATCH 8/9] Space --- src/Neo/SmartContract/ApplicationEngine.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Neo/SmartContract/ApplicationEngine.cs b/src/Neo/SmartContract/ApplicationEngine.cs index e0047b03ff..64d9dd7f9c 100644 --- a/src/Neo/SmartContract/ApplicationEngine.cs +++ b/src/Neo/SmartContract/ApplicationEngine.cs @@ -416,6 +416,7 @@ public static ApplicationEngine Create(TriggerType trigger, IVerifiable containe return Provider?.Create(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable) ?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable); } + /// /// Extracts a substring from the specified buffer and pushes it onto the evaluation stack. /// From da9db730448756c2e3cd89a635be2bd115d93b1c Mon Sep 17 00:00:00 2001 From: nan01ab Date: Wed, 4 Dec 2024 21:19:00 +0800 Subject: [PATCH 9/9] remove duplicated lines in gitignroe --- .gitignore | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index 5cedf890b1..16342efe06 100644 --- a/.gitignore +++ b/.gitignore @@ -260,9 +260,5 @@ launchSettings.json /coverages **/.DS_Store -# UT coverage report -/coverages -**/.DS_Store - # Benchmarks **/BenchmarkDotNet.Artifacts/