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

Validated EVM Contracts #2348

Closed
wants to merge 12 commits into from
Closed

Validated EVM Contracts #2348

wants to merge 12 commits into from

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Nov 4, 2019

A set of contract markers and validation rules relating to those markers is proposed. These
validation rules enable forwards compatible evolution of EVM contracts and provide some assurances
to Ethereum clients allowing them to disable some runtime verification steps by moving these
validations to the deployment phase.

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
* remove 1985 references, I don't see it applying
* enumerate invalid opcodes
* embedded links to othe EIPs
* style on backticking numbers and opcodes

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
EIPS/eip-2348.md Outdated
---

<!--
HTML checker won't let me reference unpublished EIPs, namely 2327, 1707, 1712
Copy link
Member

Choose a reason for hiding this comment

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

You can still put these into a references section or into the rationale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are referenced alongside their ideas in the specification body. I would have preferred to also have them in the header.

Copy link
Member

Choose a reason for hiding this comment

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

You could also persuade #1707 and #1712 to fix the PRs for draft status 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1707 is abandoned in the PR text and getting wei to publish his EIPs in this repo is problematic at the moment, so I have links to the PRs.

There is also the issue that they both need some minor revisions. 1707 needs to have the reference to WASM removed as that is out of scope of the "minimum required changes", and 1712 does not have accomidation for future evolution and only references PUSHn in the multibyte list, as well as not accounting for BEGINDATA. So I could not reference those EIPs as the text currently stands. And getting Wei to update the texts at the moment is blocked for reasons orthogonal to the content of those EIPs.

EIPS/eip-2348.md Outdated
<!--
HTML checker won't let me reference unpublished EIPs, namely 2327, 1707, 1712
requires: 1702, 2327
replaces: 615 (in part), 1707 (unpublished, abandoned), 1712 (unpublished)
Copy link
Member

Choose a reason for hiding this comment

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

Btw strictly speaking the replaces field can only reference Final (aka. deployed) EIPs.

@axic
Copy link
Member

axic commented Nov 4, 2019

Lets merge #2327 first so that can at least be properly referenced.

@axic axic added the type: Core label Nov 4, 2019
EIPS/eip-2348.md Outdated

For purposes of EVM Program Counter calculations the first byte after the version header is location
`0`. The contract header is not part of the accessible contract data. There is no mechanism within
the EVM to retrieve the header values.
Copy link
Member

Choose a reason for hiding this comment

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

There is no mechanism within the EVM to retrieve the header values.

CODECOPY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, but how does this reconcile with the start location? Do we keep PC=0 at zero and specify the EVM should skip the first 4 bytes of such a contract? Or do we let it not match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added three options to deal with this in the EIP text. Only one will make it out of draft.

EIPS/eip-2348.md Outdated Show resolved Hide resolved
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

### Static Jump Validations

For every jump operation preceded by a `PUSHn` instruction the value of the data pushed on to the
Copy link
Contributor

Choose a reason for hiding this comment

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

What about jump operations not preceded by a PUSHn instruction?

Copy link

Choose a reason for hiding this comment

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

What about jump operations not preceded by a PUSHn instruction?

In that case we have to use data flow analysis to determine if the argument to JUMP is a constant specified by a PUSHn. I assume validation is a one time thing (before deploying the contract?) so building a data flow graph does not seem to be too expensive.

BTW, compilers only generate PUSH2 in this case because ... there is a size limit to the smart contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we should not use this relatively weak heuristics as a part of new validation rules. It is better to implement subroutines (which eliminates the most common source of dynamic jumps - which is return from the subroutine), and then the actual static jumps, and disable the dynamic jumps all together. Then we can remove JUMPDEST.

Copy link

Choose a reason for hiding this comment

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

Using subroutines is way better for validating the contract, but it is not infeasible to validate a contract without static jumps. Symbolic execution is still able to figure out which jump is dynamic and hence report it.

@holiman
Copy link
Contributor

holiman commented Nov 15, 2019

In one attack we've seen , the attacker expanded memory to a megabyte, then did CREATE on the megabyte if init-code. This caused a jumpdest validation. THe memory was mainly filed with JUMPDESTs.

Anyway, once this failed (which it did pretty cheaply), the caller could modify one byte in memory (cheap), and do it again. Since mem was already expanded, this is a pretty cheap operation.

Now, we can cheat a bit, and defer some parts of the jumpdest analysis -- we need only store a bitmap, one eight a size of the code, where we remember if a portion is a data or code. Not until the code actually perform the jump do we need to check if it's actually JUMPDEST. Also, we can defer the analysis until we have an actual jump, if we want to.

With this PR, for every such attack-create, we need to do more work up-front. The entire validation needs to be done, because if it is failing, then we need to immediately abort execution.

So that means that for that one megabyte, where previously we could just iterate over bytes (and flip some bits at pushn segments [1] ), we need to

  • detect jumps, backtrack to check the pushn preceding it, check if the dest is a jumpdest. But wait, it also needs to be a valid jumpdest -- and that we don't know, if the position is ahead of us... So we actually need to do a two-pass evaluation.
  1. Evaluate valid jumpdests
  2. Validate JUMPs

So I think this brings on a more than 2x amplification to that attack described previously.

[1] To expand a bit: it is computationally expensive to flip every bit in a jumpdest bitmap for a one-megabyte contract. Therefore, it's better to flip bits only at pushn segments: whenever we have a push32 xxx, we can flip an entire byte for the same cost as flipping an individual bit. This makes the worst-case being a contract filled with push1 01, where every second bit needs to be flipped. That's still 2x better than a contract filled with JUMPDESTs, where every single bit needs to be flipped. Add to that that execution of a JUMPDEST-only contract is essentially free -- so the attacker can actually run through those 1M jumpdests and then execute some code, whereas a PUSH1 01 -filled contract will burn a lot more gas.

@holiman
Copy link
Contributor

holiman commented Nov 15, 2019

Also, for reference: the jumpdest analysis in geth is this tiny piece: https://github.com/ethereum/go-ethereum/blob/master/core/vm/analysis.go#L38 . It would be interesting to see the corresponding implementation that would be required for this proposal.

@shemnon
Copy link
Contributor Author

shemnon commented Nov 18, 2019

Here's my conceptual code: shemnon/besu@1c7f3e6#diff-24b154f8350072f858088bcc018d1888R41

Was this attack contract done prior to Spurious Dragon? Contract sizes were cut down to 24K then, so the contract length checking would mark the contract as invalid on mainnet fairly quickly. That's check is done in a different class than what I linked but is present in besu. It's still an amplification attack but the size is significantly limited on mainnet and up to date testnets.

For this logic it's one pass, storing into two bitsets, so 8k extra memory for mainnet and only one contract walk. It is followed up with a couple of bit checks but that should all be in cache for modern processors.

As far as the cost for flipping jumpdest bits remember we are also checking opcode validity at each point, so there would be more processing at every operation already.

For jump operations not preceded by a PUSHn no validation can reasonably performed, especially since the stack value can be set by a called parameter. This would allow the VM to defer jumpdest validation until just prior to the first encounter of a dynamic jump operation. Or a client could note that there was a dynamic jump operation and store the jumpdest map somewhere useful. Future versions of the gas table could charge more for dynamic jumps to incentivize static jump usage.

… `0xefevm` as header bytesSigned-off-by: Danno Ferrin <danno.ferrin@gmail.com>

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
contract is treated exactly as through it had been deployed to an account with version `0`. For
these contracts none of the other subsections in this EIP apply.

When deploying a contract if a contract starts with `0xef` and has a length 4 or later the first
Copy link
Contributor

Choose a reason for hiding this comment

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

later => larger, greater


## Motivation

There are two major motivations: first the need to make the EVM easier to evolve, and the second is
Copy link
Contributor

Choose a reason for hiding this comment

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

Two major and one minor?

rules described in the Version Header, `BEGINDATA`, Invalid Opcode Validation, and Static Jump
Validations sections.

These EIP sections applies to contracts stored or in the process of being stored in in accounts with
Copy link
Contributor

Choose a reason for hiding this comment

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

in in accounts => in accounts

@github-actions
Copy link

github-actions bot commented Sep 8, 2020

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Sep 8, 2020
@github-actions
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants