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

RAUdefender Research #2693

Closed
dusmart opened this issue Apr 11, 2022 · 52 comments · Fixed by #2749
Closed

RAUdefender Research #2693

dusmart opened this issue Apr 11, 2022 · 52 comments · Fixed by #2749

Comments

@dusmart
Copy link

dusmart commented Apr 11, 2022

Introducing this predictable random sequence into contract can be vulnerable. Contract attackers will call GetRandom repeatedly until the next GetRandom's result is profitable. This has also been integrated into coz's props toolchain here which can be exploited if the contract developer can't do it carefully.

protected internal BigInteger GetRandom()
{
nonceData = Cryptography.Helper.Murmur128(nonceData, ProtocolSettings.Network);
return new BigInteger(nonceData, isUnsigned: true);
}

As seen above, the sequence is generated in a chain way. The first random number can be used to predicate the second one. Although Murmur128 is difficulty to produce in neovm for now, this is still a risk. Whether implementing Murmur128 on NEOVM is possible is controversial for now. I'd like to leave this issue here with a condition and don't waste more time to try to implement it. I've implemented a Murmur128 on chain.

The detail exploit method can be seen in #2693 (comment). Another subtle attack by roman-khimov can be seen in #2693 (comment) which bypasses the size checking. Based on the discussion, I composed a POC here #2693 (comment).

Thanks for Liaojinghui's paper, I learned an attack method called revert-after-use which is more dangerous and common. I'd like to call the issue mentioned here pick-before-use. The protection (size checking) for revert-after-use by Liaojinghui may also protect contracts from pick-before-use. The protection is quoted below.

BCR_{AND}{RAUD} verifies whether C is the first smart contract invoked by T. Then BCR_{AND}{RAUD} verifies whether the calling script of T contains extra script logic to validate the execution of C.

Anyway, I call for choosing random_i = Hash(random_{i-1}, counter_i) for sequence generating instead of random_i = Hash(random_{i-1}).

Inspired by the paper's Irrevocable Execution, there maybe one more scence to be considered. GAS manipulating #2693 (comment)

To defend RAU, we'd better notice the developer do it carefully. Unknown code excuting should be avoided including transfering NEP-17 to some contracts because that will call the OnNep17Payment method.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

For a series of such numbers, a trivial solution would be to add some constant amount and hashing the result.

reference in ethereum's yellow paper VERSION 934279c section 12.2 (Random Numbers for Implementing Contract)

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Well, the answer is no. Currently the only secure (or considered secure) random solutions for smart contracts is Chainlink VRF, which leverages TEE and VRF to generate random values in a callback transaction. We are implementing a solution with BLS and BFT, but it is still user going, currently this random value is not suggested to be used in productive environment, because it is potentially vulnerable, (Currently all random solutions for smart contract are vulnerable to a certain degree) but not in the kind as you mentioned.

Ethereum is working with IPFS on a verifiable delay function (VDF) random number solution, but not much message can be found.

The attack you mentioned is called revert-after-use (yes, we found it and we named it since the moment we implement this function, if everything goes well, you will see it in our coming paper) and we proposed solutions to defend against such attack.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Buuuuuut, thank you very much for your reminding, i need to write a manual on this random number stuff, maybe even add a warning when user calls this function.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Random number generator for blockchain smart contract is a very tough topic, currently no real secure (decentralized) solution available, you gonna have to trust something else (centralized) aside from the blockchain.
However, thanks to @doubiliu and @Qiao-Jin and other members of the neo random number team (a team with 10 members) lead by @steven1227, we made some progress on it.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

The attack you mentioned is called revert-after-use (yes, we found it and we named it since the moment we implement this function, if everything goes well, you will see it in our coming paper) and we proposed solutions to defend against such attack.

Yes, random number generator for blockchain is hard and I've read your random-related documents. It's very helpful to me. Is there any more infomation I can read about revert-after-use? I'm not sure whether it's the point I want to deliver.

But I'm not talking about the random itself, I want to point the way that the random sequence generating is not good enough. Even if the contract author trust the block miner as the secure source, this chain-link way can be still vulnerable. Because next-Random = Hash(pre-Random) makes the next random predictable, any user can then manipulate the contract's Random result because he/she can put any code before calling the contract.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Yes, we are 100% talking about the same thing.

The basic idea of revert-after-use defender is to disable users from calling the target contract from an attack contract and makes sure that the attack transaction could not verify the random value from the script.

And different transactions will generate totally different random values, therefore,,,,,,,, no one could attack getrandom by predicting the next-random,,,,,

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void FAUDRequire(int size)
{
    // Disable call from another contract
    ExecutionEngine.Assert(Runtime.EntryScriptHash == Runtime.CallingScriptHash, "FAUD: Contract call is not allowed.");
    // Prevent malicious transaction script
    ExecutionEngine.Assert((Transaction)Runtime.ScriptContainer).Script.Length <= size), "FAUD:Transaction script length error.");
}

This is how we generate different random seeds for different transactions:

            this.nonceData = container is Transaction tx ? tx.Hash.ToArray()[..16] : new byte[16];
            if (persistingBlock is not null)
            {
                fixed (byte* p = nonceData)
                {
                    *(ulong*)p ^= persistingBlock.Nonce;
                }
            }

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

I was going to show you how neoverse prevent the revert-after-use, ,,,but they banned my access to the repository~~~~~mercyless

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

I was going to show you how neoverse prevent the revert-after-use, ,,,but they banned my access to the repository~~~~~mercyless

I've read the bytecode instead of source code and saw those checks. It checks not only the call is from EOA, but also the script size. It indeed can prevent the attack. Others like coz's PROPS toolchain can not do it well.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

My problem, i focus too much on doing research with it, will write more blogs and manuals explaining it.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

After the VRF way is implemented. I wonder if it can only generate one random number a time or a sequence.
If it's a sequence, maybe using random_i = Hash(random_0, counter_i) is better than random1 = Hash(random0).

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

I've read the bytecode instead of source code and saw those checks.

Wow, you read the bytecode~~~~~~~amazing......i might update my neo disassembly tool to make such things easier (currently its still very shabby).

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

I've read the bytecode instead of source code and saw those checks.

i might update my neo disassembly tool to make such things easier (currently its still very shabby).

That will be great. Where can I find that?

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

After the VRF way is implemented. I wonder if it can only generate one random number a time or a sequence. If it's a sequence, maybe using random_i = Hash(random_0, counter_i) is better than random1 = Hash(random0).

Well, we are using random_i = Hash(random_{i-1})....its basically random_i = Hash(random_0, counter_i) and can generate any number of random values you want

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

Actually, I mean that random_i = Hash(random_{i-1}) will leave the revert-after-use issue there. Because user can add any code before calling the contract in NEO (not from a attack contract but with some heading code). Then he will take as much random number as possible so that he find the next will be good.

But if we choose random_i = Hash(random_0, counter_i), the problem above will disappear.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

i might update my neo disassembly tool to make such things easier (currently its still very shabby).

That will be great. Where can I find that?

When i say shabby, i really mean it,,,,, haven't update it for 3 years... i though no one is gonna need a disassembly tool, so i stopped working on it~~~ will pick it up and update it. Hope this time i can make it more useful since i have being study the neo source code for 4 years and have read every single line of the compiler and virtual machine.

here is the project link:
https://github.com/Liaojinghui/NEODecompiler

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

But if we choose random_i = Hash(random_0, counter_i), the problem above will disappear.

i dont see any difference between random_i = Hash(random_{i-1}) and random_i = Hash(random_0, counter_i),

From a security view, they provide the same level of security promise

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

In this way random_i = Hash(random_0, counter_i), even if the attacker can get the random_{i-1}, he can not calculate the random_i on chain. So that he can not predicate the random_i that will be taken by the contract.

While if it is random_i = Hash(random_{i-1}), attacker can write this kind of bytecode as script and concat it before calling the contract.

this = get_random()
for i in range(0, 10):
    next = hash(this)
    if profitable(next):
        break
    this = get_random()
if !profitable(this):
    abort()

This trailing code will make sure that the contract's random will be a profitable number for the attacker.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Then who is the one to write this code? the attacker could not add such script in the transaction, the contract developer could not have access to the hash function from the virtual machine, and i dont see any reason a contact developer would do such a thing.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

Then who is the one to write this code? the attacker could not add such script in the transaction, the contract developer could not have access to the hash function from the virtual machine, and i dont see any reason a contact developer would do such a thing.

This code will be added by the attacker. Say that if the Neoverse didn't check the script size and I'm the attacker. I'll add this bytecode in my unbox transaction. So that I will surely open a N box instead of E box.

The hash function will be implemented by me using bytecode.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

you dont have access to this hash function~~~~~its not sha256 nor any hash function you can access from the virtual machine~~~~😜~~

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

you dont have access to this hash function~~~~~

The hash function is a deterministic function and I can translate it into NEOVM's bytecode since the seed used here is the networkid.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

you dont have access to this hash function~~~~~its not sha256 nor any hash function you can access from the virtual machine~~~~😜~~

Yes. Although Murmur128 is difficulty to produce in neovm for now, this is still a risk.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

it is not difficult, it is impossible,,,,,,,,,,the calculation will exhaust your gas~~~ way more GAS than a transaction is allowed

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

And we have constrains to the size of a transaction, it is impossible to implement a Murmur128 with bytecode.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

it is not difficult, it is impossible,,,,,,,,,,the calculation will exhaust your gas~~~ way more GAS than a transaction is allowed

I think that's why I have no POC in this issue 😂.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Man, you almost got me,,,,😂😂😂😂😂

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

Man, you almost got me,,,,😂😂😂😂😂

I'd better have a try to implement a murmur128 contract in testnet.It seems that the calculation step is not that much here because the nonce need to be hashed is only 128-bit (the loop will be 16 times).

I'm not going to implement it. This issue is controversial for now. And I changed the title with a condition that if Murmur128 can be implemented in NEOVM's bytecode .

for (int i = ibStart; i < alignedLength; i += 16)
{
ulong k1 = BinaryPrimitives.ReadUInt64LittleEndian(array.AsSpan(i));
k1 *= c1;
k1 = BitOperations.RotateLeft(k1, r1);
k1 *= c2;
H1 ^= k1;
H1 = BitOperations.RotateLeft(H1, 27);
H1 += H2;
H1 = H1 * m + n1;
ulong k2 = BinaryPrimitives.ReadUInt64LittleEndian(array.AsSpan(i + 8));
k2 *= c2;
k2 = BitOperations.RotateLeft(k2, r2);
k2 *= c1;
H2 ^= k2;
H2 = BitOperations.RotateLeft(H2, 31);
H2 += H1;
H2 = H2 * m + n2;
}

https://github.com/neo-project/neo/blob/master/src/neo/Cryptography/Murmur128.cs

@shargon
Copy link
Member

shargon commented Apr 11, 2022

I think that's why I have no POC in this issue 😂.

You only need to be a primary node to avoid the murmur128 :)

@dusmart dusmart changed the title predictable RANDOM can be vulnerable predictable RANDOM can be vulnerable if Murmur128 can be implemented in NEOVM's bytecode Apr 11, 2022
@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Even if you can implement Murmur128 with bytecode,,,,,why getrandom become vulnerable?

As i told you, you have no where to put ur bytecode murmur128~~~~

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

An adversary can simply get the random value and check the execution result, why would it bother to implement a bytecode hash function~~~if we are talking about malicious contract, it has way more power to do with the random number, cause it is the one who decides how to use it anyway..... its like you assume Tencent game server is malicious,,,,,it should not be a problem for us to concern

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Threat model of getrandom is malicious consensus node, and malicious transactions~~~ smart contract is not our threat model cause contract is the one who use the random number.........

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

An adversary can simply get the random value and check the execution result, why would it bother to implement a bytecode hash function

This should have been solved by your publication Blockchain_Random_Number . I'm not sure if it can help avoiding my concern (add trailing code instead of calling from a attack contract).

if we are talking about malicious contract

nope, I'm talking about a attacker attack a normal contract that using the random

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

Threat model of getrandom is malicious consensus node, and malicious transactions~~~ smart contract is not our threat model cause contract is the one who use the random number.........

Yes of cause.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

nope, I'm talking about a attacker attack a normal contract that using the random

Again, an attacker attacks a contract from where? from another smart contract? it is disabled with

ExecutionEngine.Assert(Runtime.EntryScriptHash == Runtime.CallingScriptHash, "FAUD: Contract call is not allowed.");

from a transaction? it is disabled by:

    // Prevent malicious transaction script
    ExecutionEngine.Assert((Transaction)Runtime.ScriptContainer).Script.Length <= size), "FAUD:Transaction script length error.");

So, where is the attack surface

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

nope, I'm talking about a attacker attack a normal contract that using the random

Again, an attacker attack a contract from where? from another smart contract? it is disabled with

ExecutionEngine.Assert(Runtime.EntryScriptHash == Runtime.CallingScriptHash, "FAUD: Contract call is not allowed.");

from a transaction? it is disabled by:

    // Prevent malicious transaction script
    ExecutionEngine.Assert((Transaction)Runtime.ScriptContainer).Script.Length <= size), "FAUD:Transaction script length error.");

So, where is the attack surface

In this way, every contract developer using GetRandom will need to check the transaction size carefully. While using counter only a little more memory will be used and the developer will pay no attention to the transaction size.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

If this size checking is mentioned in the paper and will be documented in NEO's doc, then we can safely ignore my concern.

@dusmart dusmart closed this as completed Apr 11, 2022
@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

Leave the concern there.

I see no good using random_i = Hash(random_{i-1}) other than a counter's memory is saved. Why not choosing random_i = Hash(random_{i-1}, counter_i)?

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Bro, may you please keep this issue open~~~i think your concern is valuable and your discussion is also a good treasure for this getrandom function

@dusmart dusmart reopened this Apr 11, 2022
@roman-khimov
Copy link
Contributor

roman-khimov commented Apr 11, 2022

But does transaction size check completely solve the problem?

NEWARRAY0
PUSH15
PUSHDATA1 'a'
PUSHDATA1 bad_contract
SYSCALL System.Contract.Call
UNPACK // and get all the things needed to call good_contract on the stack, including its hash
SYSCALL System.Contract.Call

might as well be less than what good_contract expects in terms of size.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Gracias

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

But does transaction size check completely solve the problem?

PUSHDATA1 bad_contract
SYSCALL System.Contract.Call
UNPACK
PUSHDATA1 good_contract
SYSCALL System.Contract.Call

might as well be less than what good_contract expects in terms of size.

Simple size check can not solve this issue, that is why it has to work together with the calling script verify. Currently i dont see any situation to bypass the check, but i might be wrong. That is why i ask Mr. @dusmart to keep this issue open.

@roman-khimov
Copy link
Contributor

it has to work together with the calling script verify

It will easily pass in my case, the caller will still be the entry script.

@dusmart
Copy link
Author

dusmart commented Apr 11, 2022

But does transaction size check completely solve the problem?

PUSHDATA1 bad_contract
SYSCALL System.Contract.Call
UNPACK
PUSHDATA1 good_contract
SYSCALL System.Contract.Call

might as well be less than what good_contract expects in terms of size.

That's a great idea!
Actually, a contract called gleeder on neo mainnet do the check even more carefully (I checked it's bytecode). It make sure the transaction's script's size is a specific size and make sure every script byte is the same as its need.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Well~~~~~~~~emmmmmmm

PUSHDATA1 bad_contract SYSCALL System.Contract.Call

wont this change the script size and Runtime.EntryScriptHash... I need to check it

@Jim8y
Copy link
Contributor

Jim8y commented Apr 11, 2022

Anyway thank you @dusmart and @roman-khimov .

@dusmart
Copy link
Author

dusmart commented Apr 12, 2022

@Liaojinghui After reading the paper, I think there maybe one more scence to consider for Irrevocable Execution. Two scences (calling from delegate contract / running scripts validate outcome) have been mentioned. There is one more which is GAS manipulating.

If some contract author write a different path for different random cases and forget to process the GAS difference (eg, adding GAS to a same level), then the attacker may manipulate his GAS balance in his account before that transaction. This attack can only achieve one path over another. Making the non-profitable path cheap than profitable will also be OK.

Three transactions sent from one account will be needed and I've done two transactions in testnet for testing.
  • while validating in pre-excuting, one path is evaluated (switch not open) so that all transactions will be valid, because three tx's excution context will all be the previous block
  • while excuting, these three txes depends on each other, the first tx will trigger the sencond tx's GAS consuming
  • if the GAS is manipulate carefully, only one random number will not revert in tx3
  1. tx1: open a switch
  2. tx2: test the switch, if it's open, do something else expensive this time (make sure remaining GAS can pass tx3 only if the random number is profitable, in testnet I deploy a contract in this step)
  3. tx3: interact with the random contract

Here is a proof that these three can all be validated during pre-excution.

  • the block
  • first tx that open a switch
  • second tx that pass the verification while fallback because of Insufficient GAS during excution, see the Exception part of the page

There is no pre-execution in NEO. Therefore, one tx will be enough. Because RAUDefender has fibidden the ability to detect GAS by System.Runtime.GasLeft, set the minimum fee will be a good tip for ressist this manipulation.

Can I join the RAU Defender project? This topic is so attractive.

@Jim8y
Copy link
Contributor

Jim8y commented Apr 12, 2022

@dusmart I am so so so excited that you can find these issues, thank you very much.
There is no such RAU defender project, but i am willing to make one, let's keep in touch. Add me on WeChat at TheMiddleNight or we can keep chatting here.

@dusmart dusmart changed the title predictable RANDOM can be vulnerable if Murmur128 can be implemented in NEOVM's bytecode RAUdefender Research Apr 12, 2022
@ixje
Copy link
Contributor

ixje commented Apr 15, 2022

When i say shabby, i really mean it,,,,, haven't update it for 3 years... i though no one is gonna need a disassembly tool, so i stopped working on it~~~ will pick it up and update it. Hope this time i can make it more useful since i have being study the neo source code for 4 years and have read every single line of the compiler and virtual machine.

here is the project link: https://github.com/Liaojinghui/NEODecompiler

I think a disassembler + decompiler is cool and can be very useful. I do wonder why did you choose a ground-up solution? The alternative being a plugin for IDA+HexRays or Ghidra where you can leverage existing tools to further work with the data and cleanup the decompiled listings. I've had this idea for a long time but my IDA license is ancient by now and I don't have the bandwidth to learn how to do it in Ghidra (but hope some else does! 👀 )

@Jim8y
Copy link
Contributor

Jim8y commented Apr 15, 2022

@ixje i am open to all options, will search on that. I agree, bottom-up is silly, i stuck a lot.

@dusmart
Copy link
Author

dusmart commented Apr 22, 2022

@roman-khimov Based on your idea, I finally composed a POC here with a Murmur128 on chain.

0x68b9a3cc6f8993452b771f4e2c35755988c5a98ed8663a48439aaf34290fdcd9
0x76aaed555ffb371cd165232d2e51946c8015987fc8c672c7ed81477137e4d7e4

detail can be seen in https://github.com/lazynode/Tanya/pull/22/files

  • The victim is the LotteryWithDefender whose method is play. The play will give the player 1 NEO only if the random is odd number.
  • The attacker uses a pick-before-usage method called a in contract LotteryWithDefenderAttacker. This method will not only choose a good random for next usage, but also return two parameters for next usage.
  • The attacker then concat a method call to play.

@dusmart dusmart changed the title RAUdefender Research RAUdefender Research -- **present RAUDefender can not prevent all attacks** with a POC which can pick random before use Apr 22, 2022
@dusmart dusmart changed the title RAUdefender Research -- **present RAUDefender can not prevent all attacks** with a POC which can pick random before use RAUdefender Research Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants