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.

Sign up for GitHub

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

Jump to bottom

Check return value of contracts #1680

Merged
merged 5 commits into from
Jun 15, 2020
Merged

Check return value of contracts #1680

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dadfde4
Check return value of contracts
erikzhang Jun 3, 2020
7b01ca6
Merge branch 'master' into check-return-value
shargon Jun 3, 2020
061e61d
Merge branch 'master' into check-return-value
vncoelho Jun 9, 2020
41dec3d
Merge branch 'master' into check-return-value
erikzhang Jun 15, 2020
411cfcc
Merge branch 'master' into check-return-value
erikzhang Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/neo/Ledger/Blockchain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ static Blockchain()
using (ScriptBuilder sb = new ScriptBuilder())
{
foreach (NativeContract contract in contracts)
sb.EmitAppCall(contract.Hash, "onPersist");
sb.EmitAppCall(contract.Hash, ContractParameterType.Boolean, "onPersist");

onPersistNativeContractScript = sb.ToArray();
}
Expand Down
18 changes: 11 additions & 7 deletions src/neo/SmartContract/ApplicationEngine.Contract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,19 +100,19 @@ internal void DestroyContract()
Snapshot.Storages.Delete(key);
}

internal void CallContract(UInt160 contractHash, string method, Array args)
internal void CallContract(UInt160 contractHash, ContractParameterType returnType, string method, Array args)
{
CallContractInternal(contractHash, method, args, CallFlags.All);
CallContractInternal(contractHash, returnType, method, args, CallFlags.All);
}

internal void CallContractEx(UInt160 contractHash, string method, Array args, CallFlags callFlags)
internal void CallContractEx(UInt160 contractHash, ContractParameterType returnType, string method, Array args, CallFlags callFlags)
{
if ((callFlags & ~CallFlags.All) != 0)
throw new ArgumentOutOfRangeException(nameof(callFlags));
CallContractInternal(contractHash, method, args, callFlags);
CallContractInternal(contractHash, returnType, method, args, callFlags);
}

private void CallContractInternal(UInt160 contractHash, string method, Array args, CallFlags flags)
private void CallContractInternal(UInt160 contractHash, ContractParameterType returnType, string method, Array args, CallFlags flags)
Copy link
Member

Choose a reason for hiding this comment

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

Why the caller need to specify the return type? it should be defined in the abi.

Copy link
Member Author

Choose a reason for hiding this comment

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

For security reason. See the example:

bool verify(int a, int b)
{
    CallSomeContract();
    return a > b;
}

We expect CallSomeContract() to return no value. But if it maliciously returns 2 values, then these values will be automatically copied onto our stack, causing us to return wrong results.

//script of CallSomeContract()
PUSH 1 // this hide the parameter b of verify()
PUSH 2 // this hide the parameter a of verify()
RET

So we have to check the return values at the runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

As CallContract and CallContractEx is to call contract public methods, we can read returnType from ABI directly. Why need user to pass the value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because you can modify the abi at any time.

Copy link
Member

@shargon shargon Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we should not be able to modify the abi without change the script

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can create a canary in this methods, store the number of items previously, and check it later, if it's more than expected (by compiler), we can throw an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set rvcount = 0 for _initialize, it may have the same issue.

neo/src/neo/SmartContract/ApplicationEngine.Contract.cs

Lines 160 to 161 in fc67879

md = contract.Manifest.Abi.GetMethod("_initialize");
if (md != null) LoadClonedContext(md.Offset);

Copy link
Member Author

Choose a reason for hiding this comment

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

if it's more than expected (by compiler)

How to do it?

Should we set rvcount = 0 for _initialize, it may have the same issue.

_initialize() is not a cross-contract call, so it's okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if it maliciously returns 2 values

Now the contract can only return one value, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is just an example. One value can also harm the system.

erikzhang marked this conversation as resolved.
Show resolved Hide resolved
{
if (method.StartsWith('_')) throw new ArgumentException();

Expand All @@ -139,11 +139,15 @@ private void CallContractInternal(UInt160 contractHash, string method, Array arg

ContractMethodDescriptor md = contract.Manifest.Abi.GetMethod(method);
if (md is null) throw new InvalidOperationException();
int rvcount = md.ReturnType == ContractParameterType.Void ? 0 : 1;
ExecutionContext context_new = LoadScript(contract.Script, rvcount);
if (returnType == ContractParameterType.Void && md.ReturnType != ContractParameterType.Void)
throw new InvalidOperationException();
if (returnType != ContractParameterType.Any && md.ReturnType != returnType)
throw new InvalidOperationException();
ExecutionContext context_new = LoadScript(contract.Script);
state = context_new.GetState<ExecutionContextState>();
state.CallingScriptHash = callingScriptHash;
state.CallFlags = flags & callingFlags;
state.RVCount = returnType == ContractParameterType.Void ? 0 : 1;

if (NativeContract.IsNative(contractHash))
{
Expand Down
15 changes: 12 additions & 3 deletions src/neo/SmartContract/ApplicationEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,15 @@ internal bool AddGas(long gas)
return testMode || GasConsumed <= gas_amount;
}

protected override void ContextUnloaded(ExecutionContext context)
{
base.ContextUnloaded(context);
if (context.EvaluationStack == CurrentContext?.EvaluationStack) return;
int rvcount = context.GetState<ExecutionContextState>().RVCount;
if (rvcount != -1 && rvcount != context.EvaluationStack.Count)
throw new InvalidOperationException();
}

protected override void LoadContext(ExecutionContext context)
{
// Set default execution context state
Expand All @@ -64,9 +73,9 @@ protected override void LoadContext(ExecutionContext context)
base.LoadContext(context);
}

public ExecutionContext LoadScript(Script script, CallFlags callFlags, int rvcount = -1)
public ExecutionContext LoadScript(Script script, CallFlags callFlags)
{
ExecutionContext context = LoadScript(script, rvcount);
ExecutionContext context = LoadScript(script);
context.GetState<ExecutionContextState>().CallFlags = callFlags;
return context;
}
Expand Down Expand Up @@ -144,7 +153,7 @@ protected override bool OnSysCall(uint method)
{
value = pd.Converter(item);
if (pd.IsEnum)
value = Convert.ChangeType(value, pd.Type);
value = Enum.ToObject(pd.Type, value);
else if (pd.IsInterface)
value = ((InteropInterface)value).GetInterface<object>();
}
Expand Down
25 changes: 13 additions & 12 deletions src/neo/SmartContract/ContractParameterType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,22 @@ namespace Neo.SmartContract
{
public enum ContractParameterType : byte
{
Signature = 0x00,
Boolean = 0x01,
Integer = 0x02,
Hash160 = 0x03,
Hash256 = 0x04,
ByteArray = 0x05,
PublicKey = 0x06,
String = 0x07,
Any = 0x00,

Array = 0x10,
Map = 0x12,
Boolean = 0x10,
Integer = 0x11,
ByteArray = 0x12,
String = 0x13,
Hash160 = 0x14,
Hash256 = 0x15,
PublicKey = 0x16,
Signature = 0x17,

InteropInterface = 0xf0,
Array = 0x20,
Map = 0x22,

InteropInterface = 0x30,

Any = 0xfe,
Void = 0xff
}
}
2 changes: 2 additions & 0 deletions src/neo/SmartContract/ExecutionContextState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,7 @@ internal class ExecutionContextState
/// Execution context rights
/// </summary>
public CallFlags CallFlags { get; set; } = CallFlags.All;

public int RVCount { get; set; } = -1;
}
}
2 changes: 1 addition & 1 deletion src/neo/SmartContract/Native/NativeContract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public ApplicationEngine TestCall(string operation, params object[] args)
{
using (ScriptBuilder sb = new ScriptBuilder())
{
sb.EmitAppCall(Hash, operation, args);
sb.EmitAppCall(Hash, ContractParameterType.Any, operation, args);
return ApplicationEngine.Run(sb.ToArray(), testMode: true);
}
}
Expand Down
15 changes: 9 additions & 6 deletions src/neo/VM/Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,35 +23,38 @@ public static ScriptBuilder Emit(this ScriptBuilder sb, params OpCode[] ops)
return sb;
}

public static ScriptBuilder EmitAppCall(this ScriptBuilder sb, UInt160 scriptHash, string operation)
public static ScriptBuilder EmitAppCall(this ScriptBuilder sb, UInt160 scriptHash, ContractParameterType returnType, string operation)
{
sb.EmitPush(0);
sb.Emit(OpCode.NEWARRAY);
sb.EmitPush(operation);
sb.EmitPush(returnType);
sb.EmitPush(scriptHash);
sb.EmitSysCall(ApplicationEngine.System_Contract_Call);
return sb;
}

public static ScriptBuilder EmitAppCall(this ScriptBuilder sb, UInt160 scriptHash, string operation, params ContractParameter[] args)
public static ScriptBuilder EmitAppCall(this ScriptBuilder sb, UInt160 scriptHash, ContractParameterType returnType, string operation, params ContractParameter[] args)
{
for (int i = args.Length - 1; i >= 0; i--)
sb.EmitPush(args[i]);
sb.EmitPush(args.Length);
sb.Emit(OpCode.PACK);
sb.EmitPush(operation);
sb.EmitPush(returnType);
sb.EmitPush(scriptHash);
sb.EmitSysCall(ApplicationEngine.System_Contract_Call);
return sb;
}

public static ScriptBuilder EmitAppCall(this ScriptBuilder sb, UInt160 scriptHash, string operation, params object[] args)
public static ScriptBuilder EmitAppCall(this ScriptBuilder sb, UInt160 scriptHash, ContractParameterType returnType, string operation, params object[] args)
{
for (int i = args.Length - 1; i >= 0; i--)
sb.EmitPush(args[i]);
sb.EmitPush(args.Length);
sb.Emit(OpCode.PACK);
sb.EmitPush(operation);
sb.EmitPush(returnType);
sb.EmitPush(scriptHash);
sb.EmitSysCall(ApplicationEngine.System_Contract_Call);
return sb;
Expand Down Expand Up @@ -208,14 +211,14 @@ public static string GetString(this StackItem item)
/// <param name="operation">contract operation</param>
/// <param name="args">operation arguments</param>
/// <returns></returns>
public static byte[] MakeScript(this UInt160 scriptHash, string operation, params object[] args)
public static byte[] MakeScript(this UInt160 scriptHash, ContractParameterType returnType, string operation, params object[] args)
{
using (ScriptBuilder sb = new ScriptBuilder())
{
if (args.Length > 0)
sb.EmitAppCall(scriptHash, operation, args);
sb.EmitAppCall(scriptHash, returnType, operation, args);
else
sb.EmitAppCall(scriptHash, operation);
sb.EmitAppCall(scriptHash, returnType, operation);
return sb.ToArray();
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/neo/Wallets/AssetDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ public AssetDescriptor(UInt160 asset_id)
byte[] script;
using (ScriptBuilder sb = new ScriptBuilder())
{
sb.EmitAppCall(asset_id, "decimals");
sb.EmitAppCall(asset_id, "name");
sb.EmitAppCall(asset_id, ContractParameterType.Integer, "decimals");
sb.EmitAppCall(asset_id, ContractParameterType.String, "name");
script = sb.ToArray();
}
using ApplicationEngine engine = ApplicationEngine.Run(script, extraGAS: 3_000_000);
Expand Down
8 changes: 4 additions & 4 deletions src/neo/Wallets/Wallet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,10 @@ public BigDecimal GetBalance(UInt160 asset_id, params UInt160[] accounts)
sb.EmitPush(0);
foreach (UInt160 account in accounts)
{
sb.EmitAppCall(asset_id, "balanceOf", account);
sb.EmitAppCall(asset_id, ContractParameterType.Integer, "balanceOf", account);
sb.Emit(OpCode.ADD);
}
sb.EmitAppCall(asset_id, "decimals");
sb.EmitAppCall(asset_id, ContractParameterType.Integer, "decimals");
script = sb.ToArray();
}
using ApplicationEngine engine = ApplicationEngine.Run(script, extraGAS: 20000000L * accounts.Length);
Expand Down Expand Up @@ -246,7 +246,7 @@ public Transaction MakeTransaction(TransferOutput[] outputs, UInt160 from = null
foreach (UInt160 account in accounts)
using (ScriptBuilder sb2 = new ScriptBuilder())
{
sb2.EmitAppCall(assetId, "balanceOf", account);
sb2.EmitAppCall(assetId, ContractParameterType.Integer, "balanceOf", account);
using (ApplicationEngine engine = ApplicationEngine.Run(sb2.ToArray(), snapshot, testMode: true))
{
if (engine.State.HasFlag(VMState.FAULT))
Expand All @@ -265,7 +265,7 @@ public Transaction MakeTransaction(TransferOutput[] outputs, UInt160 from = null
cosignerList.UnionWith(balances_used.Select(p => p.Account));
foreach (var (account, value) in balances_used)
{
sb.EmitAppCall(output.AssetId, "transfer", account, output.ScriptHash, value);
sb.EmitAppCall(output.AssetId, ContractParameterType.Boolean, "transfer", account, output.ScriptHash, value);
sb.Emit(OpCode.ASSERT);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/neo/neo.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<PackageReference Include="Microsoft.AspNetCore.WebSockets" Version="2.2.1" />
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="3.1.3" />
<PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="3.1.3" />
<PackageReference Include="Neo.VM" Version="3.0.0-CI00219" />
<PackageReference Include="Neo.VM" Version="3.0.0-CI00224" />
</ItemGroup>

</Project>
Loading