From 3ab849db2a7b9e1f4bc44137a4d4e69c988bcda7 Mon Sep 17 00:00:00 2001 From: German Velasco Date: Wed, 15 Aug 2018 10:17:50 -0400 Subject: [PATCH] Validate signature's s-value in transaction validity (#354) We validate that a transaction signature's s-value is not too high. How high the s-value can be depends on the fork (Homestead changed the value). We make use of `EVM.Configuration` to ensure that all forks provide the correct s-value. --- apps/blockchain/lib/blockchain/account.ex | 2 +- .../lib/blockchain/transaction/signature.ex | 30 +++++++----- .../lib/blockchain/transaction/validity.ex | 46 ++++++++++++------- .../blockchain/transaction/validity_test.exs | 28 ++++++++++- apps/evm/lib/evm/configuration.ex | 4 ++ apps/evm/lib/evm/configuration/eip150.ex | 3 ++ apps/evm/lib/evm/configuration/eip158.ex | 3 ++ apps/evm/lib/evm/configuration/frontier.ex | 4 ++ apps/evm/lib/evm/configuration/homestead.ex | 4 ++ 9 files changed, 93 insertions(+), 31 deletions(-) diff --git a/apps/blockchain/lib/blockchain/account.ex b/apps/blockchain/lib/blockchain/account.ex index a2ff8c63d..517ef078e 100644 --- a/apps/blockchain/lib/blockchain/account.ex +++ b/apps/blockchain/lib/blockchain/account.ex @@ -123,7 +123,7 @@ defmodule Blockchain.Account do ...> |> Blockchain.Account.get_account(<<0x01::160>>) nil """ - @spec get_account(EVM.state(), EVM.address()) :: t | nil + @spec get_account(Trie.t(), EVM.address()) :: t | nil def get_account(state, address) do address = if is_binary(address), do: address, else: :binary.encode_unsigned(address) trie = Trie.get(state, Keccak.kec(address)) diff --git a/apps/blockchain/lib/blockchain/transaction/signature.ex b/apps/blockchain/lib/blockchain/transaction/signature.ex index 1c9ef19d9..d8ecf52b6 100644 --- a/apps/blockchain/lib/blockchain/transaction/signature.ex +++ b/apps/blockchain/lib/blockchain/transaction/signature.ex @@ -26,6 +26,9 @@ defmodule Blockchain.Transaction.Signature do @base_recovery_id 27 @base_recovery_id_eip_155 35 + def secp256k1n, do: @secp256k1n + def secp256k1n_2, do: @secp256k1n_2 + @doc """ Given a private key, returns a public key. @@ -153,41 +156,44 @@ defmodule Blockchain.Transaction.Signature do ## Examples - iex> Blockchain.Transaction.Signature.is_signature_valid?(true, 1, 1, 27) + iex> Blockchain.Transaction.Signature.is_signature_valid?(1, 1, 27, max_s: :secp256k1n) true - iex> Blockchain.Transaction.Signature.is_signature_valid?(true, 1, 1, 20) # invalid v + iex> Blockchain.Transaction.Signature.is_signature_valid?(1, 1, 20, max_s: :secp256k1n) # invalid v false iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337 - iex> Blockchain.Transaction.Signature.is_signature_valid?(false, secp256k1n - 1, 1, 28) # r okay + iex> Blockchain.Transaction.Signature.is_signature_valid?(secp256k1n - 1, 1, 28, max_s: :secp256k1n) # r okay true iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337 - iex> Blockchain.Transaction.Signature.is_signature_valid?(false, secp256k1n + 1, 1, 28) # r too high + iex> Blockchain.Transaction.Signature.is_signature_valid?(secp256k1n + 1, 1, 28, max_s: :secp256k1n) # r too high false iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337 - iex> Blockchain.Transaction.Signature.is_signature_valid?(false, 1, secp256k1n + 1, 28) # s too high for non-homestead + iex> Blockchain.Transaction.Signature.is_signature_valid?(1, secp256k1n + 1, 28, max_s: :secp256k1n) # s too high for non-homestead false iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337 - iex> Blockchain.Transaction.Signature.is_signature_valid?(false, 1, secp256k1n - 1, 28) # s okay for non-homestead + iex> Blockchain.Transaction.Signature.is_signature_valid?(1, secp256k1n - 1, 28, max_s: :secp256k1n) # s okay for non-homestead true iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337 - iex> Blockchain.Transaction.Signature.is_signature_valid?(true, 1, secp256k1n - 1, 28) # s too high for homestead + iex> Blockchain.Transaction.Signature.is_signature_valid?(1, secp256k1n - 1, 28, max_s: :secp256k1n_2) # s too high for homestead false iex> secp256k1n = 115792089237316195423570985008687907852837564279074904382605163141518161494337 iex> secp256k1n_2 = round(:math.floor(secp256k1n / 2)) - iex> Blockchain.Transaction.Signature.is_signature_valid?(true, secp256k1n_2 - 1, 1, 28) # s okay for homestead + iex> Blockchain.Transaction.Signature.is_signature_valid?(secp256k1n_2 - 1, 1, 28, max_s: :secp256k1n_2) # s okay for homestead true """ - @spec is_signature_valid?(boolean(), integer(), integer(), integer()) :: boolean() - def is_signature_valid?(is_homestead, r, s, v) do - r > 0 and r < @secp256k1n and s > 0 and - if(is_homestead, do: s < @secp256k1n_2, else: s < @secp256k1n) and (v == 27 || v == 28) + @spec is_signature_valid?(integer(), integer(), integer(), max_s: atom()) :: boolean() + def is_signature_valid?(r, s, v, max_s: :secp256k1n) do + r > 0 and r < @secp256k1n and s > 0 and s < @secp256k1n and (v == 27 || v == 28) + end + + def is_signature_valid?(r, s, v, max_s: :secp256k1n_2) do + r > 0 and r < @secp256k1n and s > 0 and s <= @secp256k1n_2 and (v == 27 || v == 28) end @doc """ diff --git a/apps/blockchain/lib/blockchain/transaction/validity.ex b/apps/blockchain/lib/blockchain/transaction/validity.ex index a42866470..a721c9ffc 100644 --- a/apps/blockchain/lib/blockchain/transaction/validity.ex +++ b/apps/blockchain/lib/blockchain/transaction/validity.ex @@ -6,31 +6,19 @@ defmodule Blockchain.Transaction.Validity do alias Blockchain.{Account, Transaction} alias Block.Header + alias MerklePatriciaTree.Trie @doc """ Validates the validity of a transaction that is required to be true before we're willing to execute a transaction. This is specified in Section 6.2 of the Yellow Paper Eq.(65) and Eq.(66). """ - @spec validate(EVM.state(), Transaction.t(), Block.Header.t(), EVM.Configuration.t()) :: + @spec validate(Trie.t(), Transaction.t(), Block.Header.t(), EVM.Configuration.t()) :: :valid | {:invalid, atom()} def validate(state, trx, block_header, config) do - result = - case Transaction.Signature.sender(trx) do - {:error, _reason} -> - {:invalid, :invalid_sender} - - {:ok, sender_address} -> - case Account.get_account(state, sender_address) do - nil -> - {:invalid, :missing_account} - - sender_account -> - {:ok, sender_account} - end - end - - with {:ok, sender_account} <- result do + with :ok <- validate_signature(trx, config), + {:ok, sender_address} <- Transaction.Signature.sender(trx), + {:ok, sender_account} <- retrieve_account(state, sender_address) do errors = [] |> check_sender_nonce(trx, sender_account) @@ -42,6 +30,30 @@ defmodule Blockchain.Transaction.Validity do end end + @spec validate_signature(Transaction.t(), EVM.Configuration.t()) :: + :ok | {:invalid, :invalid_signature} + defp validate_signature(trx, config) do + max_s_value = EVM.Configuration.max_signature_s(config) + + if Transaction.Signature.is_signature_valid?(trx.r, trx.s, trx.v, max_s: max_s_value) do + :ok + else + {:invalid, :invalid_sender} + end + end + + @spec retrieve_account(Trie.t(), EVM.address()) :: + {:ok, Account.t()} | {:invalid, :missing_account} + defp retrieve_account(state, sender_address) do + case Account.get_account(state, sender_address) do + nil -> + {:invalid, :missing_account} + + sender_account -> + {:ok, sender_account} + end + end + @spec check_sender_nonce([atom()], Transaction.t(), Account.t()) :: [atom()] defp check_sender_nonce(errors, transaction, account) do if account.nonce != transaction.nonce do diff --git a/apps/blockchain/test/blockchain/transaction/validity_test.exs b/apps/blockchain/test/blockchain/transaction/validity_test.exs index 740ead9bf..e5c6406bc 100644 --- a/apps/blockchain/test/blockchain/transaction/validity_test.exs +++ b/apps/blockchain/test/blockchain/transaction/validity_test.exs @@ -4,6 +4,32 @@ defmodule Blockchain.Transaction.ValidityTest do alias Blockchain.{Transaction, Account} describe "validate/3" do + test "invalidates transaction when signature's s-value is too high (after homestead fork)" do + trx = %Transaction{ + data: <<>>, + gas_limit: 1_000, + gas_price: 1, + init: <<1>>, + nonce: 5, + to: <<>>, + value: 5, + r: 1, + s: Transaction.Signature.secp256k1n_2() + 1, + v: 27 + } + + result = + MerklePatriciaTree.Test.random_ets_db() + |> MerklePatriciaTree.Trie.new() + |> Validity.validate( + trx, + %Block.Header{}, + EVM.Configuration.Homestead.new() + ) + + assert result == {:invalid, :invalid_sender} + end + test "invalidates transaction when sender address is nil" do trx = %Transaction{ data: <<>>, @@ -138,7 +164,7 @@ defmodule Blockchain.Transaction.ValidityTest do assert result == {:invalid, [:over_gas_limit, :insufficient_balance]} end - test "invalidates account when tranaction gas limit exceeds header gas limit" do + test "invalidates account when transaction gas limit exceeds header gas limit" do private_key = <<1::256>> # based on simple private key sender = diff --git a/apps/evm/lib/evm/configuration.ex b/apps/evm/lib/evm/configuration.ex index f12d96eda..9c25b41f0 100644 --- a/apps/evm/lib/evm/configuration.ex +++ b/apps/evm/lib/evm/configuration.ex @@ -13,6 +13,10 @@ defprotocol EVM.Configuration do @spec fail_contract_creation_lack_of_gas?(t) :: boolean() def fail_contract_creation_lack_of_gas?(t) + # EIP2 + @spec max_signature_s(t) :: atom() + def max_signature_s(t) + # EIP7 @spec has_delegate_call?(t) :: boolean() def has_delegate_call?(t) diff --git a/apps/evm/lib/evm/configuration/eip150.ex b/apps/evm/lib/evm/configuration/eip150.ex index 0ccd8d5a7..d7da0d3e6 100644 --- a/apps/evm/lib/evm/configuration/eip150.ex +++ b/apps/evm/lib/evm/configuration/eip150.ex @@ -23,6 +23,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.EIP150 do @spec has_delegate_call?(Configuration.t()) :: boolean() def has_delegate_call?(config), do: config.fallback_config.has_delegate_call + @spec max_signature_s(Configuration.t()) :: atom() + def max_signature_s(config), do: Configuration.max_signature_s(config.fallback_config) + @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean() def fail_contract_creation_lack_of_gas?(config), do: config.fallback_config.fail_contract_creation diff --git a/apps/evm/lib/evm/configuration/eip158.ex b/apps/evm/lib/evm/configuration/eip158.ex index fa0258831..94a887486 100644 --- a/apps/evm/lib/evm/configuration/eip158.ex +++ b/apps/evm/lib/evm/configuration/eip158.ex @@ -19,6 +19,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.EIP158 do @spec has_delegate_call?(Configuration.t()) :: boolean() def has_delegate_call?(config), do: Configuration.has_delegate_call?(config.fallback_config) + @spec max_signature_s(Configuration.t()) :: atom() + def max_signature_s(config), do: Configuration.max_signature_s(config.fallback_config) + @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean() def fail_contract_creation_lack_of_gas?(config), do: Configuration.fail_contract_creation_lack_of_gas?(config.fallback_config) diff --git a/apps/evm/lib/evm/configuration/frontier.ex b/apps/evm/lib/evm/configuration/frontier.ex index 7f8e1eee9..9ded41cc4 100644 --- a/apps/evm/lib/evm/configuration/frontier.ex +++ b/apps/evm/lib/evm/configuration/frontier.ex @@ -2,6 +2,7 @@ defmodule EVM.Configuration.Frontier do defstruct contract_creation_cost: 21_000, has_delegate_call: false, fail_contract_creation: false, + max_signature_s: :secp256k1n, extcodesize_cost: 20, extcodecopy_cost: 20, balance_cost: 20, @@ -27,6 +28,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.Frontier do @spec has_delegate_call?(Configuration.t()) :: boolean() def has_delegate_call?(config), do: config.has_delegate_call + @spec max_signature_s(Configuration.t()) :: atom() + def max_signature_s(config), do: config.max_signature_s + @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean() def fail_contract_creation_lack_of_gas?(config), do: config.fail_contract_creation diff --git a/apps/evm/lib/evm/configuration/homestead.ex b/apps/evm/lib/evm/configuration/homestead.ex index b4ab84638..715cdb413 100644 --- a/apps/evm/lib/evm/configuration/homestead.ex +++ b/apps/evm/lib/evm/configuration/homestead.ex @@ -1,6 +1,7 @@ defmodule EVM.Configuration.Homestead do defstruct contract_creation_cost: 53_000, has_delegate_call: true, + max_signature_s: :secp256k1n_2, fail_contract_creation: true, fallback_config: EVM.Configuration.Frontier.new() @@ -18,6 +19,9 @@ defimpl EVM.Configuration, for: EVM.Configuration.Homestead do @spec has_delegate_call?(Configuration.t()) :: boolean() def has_delegate_call?(config), do: config.has_delegate_call + @spec max_signature_s(Configuration.t()) :: atom() + def max_signature_s(config), do: config.max_signature_s + @spec fail_contract_creation_lack_of_gas?(Configuration.t()) :: boolean() def fail_contract_creation_lack_of_gas?(config), do: config.fail_contract_creation