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

nan01ab · 2024-09-21T15:01:12Z

Description

Fix integer overflow in JumpTable.SubStr

Fixes #3495

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

cschuchardt88

@shargon why isn't there a vm limit in this

src/Neo.VM/JumpTable/JumpTable.Splice.cs

tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json

roman-khimov · 2024-09-23T12:20:45Z

tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json

+        "0x0a",
+        "0x00010203040506070809",
+        "PUSHINT32",
+        "0x7FFFFFFF",


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.

src/Neo.VM/JumpTable/JumpTable.Splice.cs

shargon · 2024-09-27T07:12:07Z

Rebase needed

shargon

I think we don't need a HF, previously could be a DoS but not difference in the execution. Isn't it? @roman-khimov

roman-khimov · 2024-11-07T14:07:19Z

That's the question of "can we arrange a set of parameters that would fail with the new code, but succeed with the old one". This requires some probing. I'm not exactly sure of Rent details and how this can be related to machine memory available (I can malloc() more memory than I have physically), also how Slice() reacts to negative count.

I'd include it into Echidna for safety reasons, but if we can prove it can't be exploited to change execution result then OK, it can go without a HF.

nan01ab · 2024-11-14T15:44:03Z

I think we don't need a HF, previously could be a DoS but not difference in the execution. Isn't it? @roman-khimov

I agree. Don't need a HF

The base branch was changed.

shargon

If doesn't use HF should go to master

Jim8y · 2024-11-21T00:53:46Z

If doesn't use HF should go to master

it was merged with hardfork prs,,,,lets discuss it in the meeting.

cschuchardt88 · 2024-11-21T13:25:34Z

This is the already the default behavior in dotnet https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/language#checkforoverflowunderflow

roman-khimov · 2024-11-21T13:44:13Z

The default value for this option is false, that is, overflow checking is disabled.

https://dotnetfiddle.net/I7RINu

cschuchardt88 · 2024-11-21T17:23:17Z

The default value for this option is false, that is, overflow checking is disabled.

https://dotnetfiddle.net/I7RINu

my bad -- integral-type arithmetic operations and conversions are executed in an unchecked context and Constant expressions are evaluated by default in a checked context.

shargon · 2024-11-21T18:34:49Z

src/Neo/SmartContract/ApplicationEngine.cs

+        /// <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)


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

cschuchardt88 · 2024-11-21T22:16:07Z

src/Neo/SmartContract/ApplicationEngine.cs

@@ -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);


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

on test Neo.UnitTests.SmartContract.UT_NotifyEventArgs.TestIssue3300

Snapshot is null?

Could be snapshot or GetInteroperable<HashIndexState>()

neo/src/Neo/SmartContract/Native/LedgerContract.cs

Lines 119 to 122 in eb96d14

public uint CurrentIndex(DataCache snapshot)

{

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

}

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

Would this work for CurrentIndex?

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

We should not change the logic in native contracts for this

cschuchardt88 · 2024-12-04T07:54:12Z

.gitignore

+# UT coverage report
+/coverages
+**/.DS_Store
+


this is a dupe of the above lines. please remove it.

this is a dupe of the above lines. please remove it.

I’ll remove it later

fix: int overflow in SubStr

98e834b

nan01ab changed the title ~~fix: integer overflow in JumpTable.SubStr~~ [Fix]: integer overflow in JumpTable.SubStr Sep 21, 2024

cschuchardt88 reviewed Sep 21, 2024

View reviewed changes

src/Neo.VM/JumpTable/JumpTable.Splice.cs Show resolved Hide resolved

src/Neo.VM/JumpTable/JumpTable.Splice.cs Outdated Show resolved Hide resolved

cschuchardt88 added the Waiting for Review label Sep 21, 2024

roman-khimov mentioned this pull request Sep 23, 2024

Adapt new way of NeoVM tests delivering nspcc-dev/neo-go#3204

Open

roman-khimov reviewed Sep 23, 2024

View reviewed changes

nan01ab and others added 2 commits September 23, 2024 20:46

fix: int overflow in SubStr

4a3c207

Merge branch 'master' into fix.overflow-in-substr

2494812

Jim8y changed the base branch from master to HF_Echidna September 27, 2024 01:24

shargon added Hardfork Waiting-Hardfork Bug Used to tag confirmed bugs Critical Issues (bugs) that need to be fixed ASAP and removed Hardfork labels Sep 27, 2024

nan01ab and others added 3 commits September 27, 2024 19:17

Merge branch 'neo-project:master' into fix.overflow-in-substr

20ec1e1

Merge branch 'master' into HF_Echidna

7860aa8

Merge branch 'HF_Echidna' into fix.overflow-in-substr

f9fc6fc

shargon previously approved these changes Nov 7, 2024

View reviewed changes

cschuchardt88 previously approved these changes Nov 14, 2024

View reviewed changes

cschuchardt88 force-pushed the HF_Echidna branch from 16a9c29 to 0457ccd Compare November 17, 2024 12:41

Jim8y mentioned this pull request Nov 20, 2024

Add hardfork HF_Echidna #3454

Draft

9 tasks

Jim8y added Hardfork and removed Waiting-Hardfork labels Nov 20, 2024

Jim8y added 2 commits November 20, 2024 10:22

Merge branch 'HF_Echidna' into fix.overflow-in-substr

5e8de32

format

40e5c1f

Jim8y changed the base branch from HF_Echidna to master November 20, 2024 02:31

Jim8y changed the base branch from master to HF_Echidna November 20, 2024 02:32

shargon approved these changes Nov 20, 2024

View reviewed changes

Jim8y approved these changes Nov 20, 2024

View reviewed changes

cschuchardt88 approved these changes Nov 20, 2024

View reviewed changes

kiki18z mentioned this pull request Nov 20, 2024

【Nov.24】Core Dev Regular Meeting Proposal Discussion #3589

Closed

shargon approved these changes Nov 20, 2024

View reviewed changes

shargon added Ready to Merge and removed Waiting for Review Hardfork labels Nov 20, 2024

Jim8y assigned shargon Nov 21, 2024

Jim8y added Need Update and removed Ready to Merge labels Nov 21, 2024

Jim8y assigned cschuchardt88 Nov 21, 2024

Versioning change

4772329

shargon approved these changes Nov 21, 2024

View reviewed changes

shargon added 4 commits November 21, 2024 19:38

Clean

5f159d8

Rename

7864e0a

Show change

20daa54

Space

2bf5125

shargon added the Hardfork label Nov 21, 2024

cschuchardt88 reviewed Nov 21, 2024

View reviewed changes

cschuchardt88 reviewed Dec 4, 2024

View reviewed changes

remove duplicated lines in gitignroe

da9db73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nan01ab commented Sep 21, 2024

cschuchardt88 left a comment

roman-khimov Sep 23, 2024

shargon commented Sep 27, 2024

shargon left a comment

roman-khimov commented Nov 7, 2024

nan01ab commented Nov 14, 2024

shargon left a comment

Jim8y commented Nov 21, 2024

cschuchardt88 commented Nov 21, 2024

roman-khimov commented Nov 21, 2024

cschuchardt88 commented Nov 21, 2024

shargon Nov 21, 2024

cschuchardt88 Nov 21, 2024

shargon Nov 22, 2024

cschuchardt88 Nov 22, 2024 •

edited

Loading

shargon Nov 22, 2024

cschuchardt88 Nov 22, 2024 •

edited

Loading

shargon Nov 23, 2024

cschuchardt88 Dec 4, 2024

nan01ab Dec 4, 2024

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

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

Are you sure you want to change the base?

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

Conversation

nan01ab commented Sep 21, 2024

Description

Type of change

Checklist:

cschuchardt88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon commented Sep 27, 2024

shargon left a comment

Choose a reason for hiding this comment

roman-khimov commented Nov 7, 2024

nan01ab commented Nov 14, 2024

shargon left a comment

Choose a reason for hiding this comment

Jim8y commented Nov 21, 2024

cschuchardt88 commented Nov 21, 2024

roman-khimov commented Nov 21, 2024

cschuchardt88 commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

cschuchardt88 Nov 22, 2024 •

edited

Loading

cschuchardt88 Nov 22, 2024 •

edited

Loading