[`Fix`]: integer overflow in `JumpTable.SubStr ` by nan01ab · Pull Request #3496 · neo-project/neo

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

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

.gitignore

* add hardofork HF_Echidna * Add entries to `Designation` event (#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * add hardofork HF_Echidna * Add entries to `Designation` event (#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * add hardofork HF_Echidna * Add entries to `Designation` event (#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * format * Fixed typo * Added back #3397 * Fixed tests * fixed global.json * Update src/Neo/Neo.csproj * Update src/Neo/Neo.csproj * [`Fix`]: integer overflow in `JumpTable.SubStr ` (#3496) * fix: int overflow in SubStr * fix: int overflow in SubStr * format * Versioning change * Clean * Rename * Show change * Space * remove duplicated lines in gitignroe --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> * Fix NEO callstates (#3599) * Allow callstates to use HF * Rename to method * Other rename * Change the way * Reduce changes * Reduce changes * Adapt name always * Avoid string when only is lower the first char * UT * Test all * Update src/Neo/ProtocolSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update src/Neo/ProtocolSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Reuse Load from stream * Unify * Fix default logic * Change ContractMethod to allowMultiple * Use LowerInvariant * Move CheckingHardfork * Remove optional arg * Fix build * Avoid file not found error --------- Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * fix tests error (#3636) * fux build error * Update src/Neo/SmartContract/ApplicationEngine.cs --------- Co-authored-by: Shargon <shargon@gmail.com> * NeoToken: accept candidate registration via onNEP17Payment (#3597) Solves two problems: * inability to estimate GAS needed for registerCandidate in a regular way because of its very high fee (more than what normal RPC servers allow) * inability to have MaxBlockSystemFee lower than the registration price which is very high on its own (more than practically possible to execute) Fixes #3552. Signed-off-by: Roman Khimov <roman@nspcc.ru> * specify the argument exception information. * Fix Ut (#3635) * NeoToken: add NEP-27 to supported standards list starting from Echidna (#3643) #3597 introduces `onNEP17Payment` handler to native NeoToke contract starting from Echidna hardfork. We need to update the list of supported standards respectively. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * ut: fix HF_Echidna unit tests (#3646) * Fix UT * Update src/Neo/ProtocolSettings.cs Co-authored-by: nan01ab <yjcc201374@outlook.com> * Update src/Neo/ProtocolSettings.cs Co-authored-by: nan01ab <yjcc201374@outlook.com> * Update src/Neo/ProtocolSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: nan01ab <yjcc201374@outlook.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * [Core Add] Add support to Ed25519 (#3507) * fix unnecessary change * Clean using --------- Co-authored-by: Fernando Diaz Toledano <shargon@gmail.com> * Fix `HF_Echidna` comments (#3679) * Fix obsolete * Fix https://github.com/neo-project/neo/pull/3454/files#r1912152270 * Fix comment * Update RoleManagement.cs * Unset HF_Echidna * Revert getTransaction * Revert verifyWithECDsa * format --------- Signed-off-by: Roman Khimov <roman@nspcc.ru> Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: nan01ab <yjcc201374@outlook.com> Co-authored-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com>

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 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

Closed

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

Merged

11 tasks

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

Jimmy 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

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

.gitignore Outdated Show resolved Hide resolved

remove duplicated lines in gitignroe

da9db73

NGDAdmin merged commit f91b680 into neo-project:HF_Echidna Dec 19, 2024

shargon mentioned this pull request Jan 8, 2025

[Bug]: integer overflow in JumpTable.SubStr #3495

Closed

nan01ab deleted the fix.overflow-in-substr branch February 5, 2025 14:06

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

Conversation

nan01ab commented Sep 21, 2024

Description

Type of change

Checklist:

Uh oh!

cschuchardt88 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roman-khimov Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shargon commented Sep 27, 2024

Uh oh!

shargon left a comment

Choose a reason for hiding this comment

Uh oh!

roman-khimov commented Nov 7, 2024

Uh oh!

nan01ab commented Nov 14, 2024

Uh oh!

shargon left a comment

Choose a reason for hiding this comment

Uh oh!

Jim8y commented Nov 21, 2024

Uh oh!

cschuchardt88 commented Nov 21, 2024

Uh oh!

roman-khimov commented Nov 21, 2024

Uh oh!

cschuchardt88 commented Nov 21, 2024

Uh oh!

shargon Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

shargon Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon Nov 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cschuchardt88 Nov 22, 2024 •

edited

Loading

cschuchardt88 Nov 22, 2024 •

edited

Loading