Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix]: integer overflow in JumpTable.SubStr #3496

Open
wants to merge 13 commits into
base: HF_Echidna
Choose a base branch
from
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -260,5 +260,9 @@ launchSettings.json
/coverages
**/.DS_Store

# UT coverage report
/coverages
**/.DS_Store

# Benchmarks
**/BenchmarkDotNet.Artifacts/
2 changes: 1 addition & 1 deletion src/Neo.VM/JumpTable/JumpTable.Splice.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 (index + count > x.Length)
if (checked(index + count) > x.Length)
shargon marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand Down
39 changes: 38 additions & 1 deletion src/Neo/SmartContract/ApplicationEngine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ namespace Neo.SmartContract
public partial class ApplicationEngine : ExecutionEngine
{
protected static readonly JumpTable DefaultJumpTable = ComposeDefaultJumpTable();
protected static readonly JumpTable NotEchidnaJumpTable = ComposeNotEchidnaJumpTable();

/// <summary>
/// The maximum cost that can be spent when a contract is executed in test mode.
Expand Down Expand Up @@ -214,6 +215,13 @@ private static JumpTable ComposeDefaultJumpTable()
return table;
}

public static JumpTable ComposeNotEchidnaJumpTable()
{
var jumpTable = ComposeDefaultJumpTable();
jumpTable[OpCode.SUBSTR] = VulnerableSubStr;
return jumpTable;
}

protected static void OnCallT(ExecutionEngine engine, Instruction instruction)
{
if (engine is ApplicationEngine app)
Expand Down Expand Up @@ -399,13 +407,42 @@ internal override void UnloadContext(ExecutionContext context)
/// <returns>The engine instance created.</returns>
public static ApplicationEngine Create(TriggerType trigger, IVerifiable container, DataCache snapshot, Block persistingBlock = null, ProtocolSettings settings = null, long gas = TestModeGas, IDiagnostic diagnostic = null)
{
var index = persistingBlock?.Index ?? NativeContract.Ledger.CurrentIndex(snapshot);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shargon
Object reference not set to an instance of an object.

on test Neo.UnitTests.SmartContract.UT_NotifyEventArgs.TestIssue3300

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshot is null?

Copy link
Member

@cschuchardt88 cschuchardt88 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be snapshot or GetInteroperable<HashIndexState>()

public uint CurrentIndex(DataCache snapshot)
{
return snapshot[CreateStorageKey(Prefix_CurrentBlock)].GetInteroperable<HashIndexState>().Index;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If is snapshot we can return 0, otherwise we should fix the test

Copy link
Member

@cschuchardt88 cschuchardt88 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work for CurrentIndex?

snapshot?[CreateStorageKey(Prefix_CurrentBlock)]?.GetInteroperable<HashIndexState>()?.Index ?? 0; 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not change the logic in native contracts for this


// Adjust jump table according persistingBlock
var jumpTable = ApplicationEngine.DefaultJumpTable;

var jumpTable = settings.IsHardforkEnabled(Hardfork.HF_Echidna, index) ? DefaultJumpTable : NotEchidnaJumpTable;

return Provider?.Create(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable)
?? new ApplicationEngine(trigger, container, snapshot, persistingBlock, settings, gas, diagnostic, jumpTable);
}

/// <summary>
/// Extracts a substring from the specified buffer and pushes it onto the evaluation stack.
/// <see cref="OpCode.SUBSTR"/>
/// </summary>
/// <param name="engine">The execution engine.</param>
/// <param name="instruction">The instruction being executed.</param>
/// <remarks>Pop 3, Push 1</remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void VulnerableSubStr(ExecutionEngine engine, Instruction instruction)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is not required, but you think that it is, here is the solution, jump table allow it :)

{
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();
// 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}].");

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
Expand Down
44 changes: 44 additions & 0 deletions tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,50 @@
}
}
]
},
{
"name": "Count Exceed Range Test",
roman-khimov marked this conversation as resolved.
Show resolved Hide resolved
"script": [
"PUSHDATA1",
"0x0a",
"0x00010203040506070809",
"PUSH2",
"PUSHINT32",
"0x7FFFFFFF",
"SUBSTR"
],
"steps": [
{
"actions": [
"execute"
],
"result": {
"state": "FAULT"
}
}
]
},
{
"name": "Index Exceed Range Test",
roman-khimov marked this conversation as resolved.
Show resolved Hide resolved
"script": [
"PUSHDATA1",
"0x0a",
"0x00010203040506070809",
"PUSHINT32",
"0x7FFFFFFF",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add some tests for INT64, like:

                byte(opcode.PUSHINT64), 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x7F,
                byte(opcode.PUSH2),

It'll fail (in NeoGo it's at instruction 22 (SUBSTR): not an int32), but just to make sure.

"PUSH2",
"SUBSTR"
],
"steps": [
{
"actions": [
"execute"
],
"result": {
"state": "FAULT"
}
}
]
}
]
}
Loading