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

Some signatures are accepted by Raiden client but rejected by smart contracts #3054

Closed
pirapira opened this Issue Nov 20, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@pirapira
Contributor

pirapira commented Nov 20, 2018

I'm wondering if there is an attack with EIP-155 style chainID-aware signatures.

The Python codebase seems to accept signatures with v == 37 or 38.

The smart contract doesn't seem to accept those. v has to be 0, 1, 27 or 28.

So, can I create a BalanceProof or something that works with the Python core, but doesn't work with the TokenNetwork contract? Is there a test about this?

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 20, 2018

If a client believes a BalanceProof works, and the BalanceProof actually doesn't work onchain, that would be a problem.

@pirapira pirapira added the security label Nov 21, 2018

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 21, 2018

@hackaugusto @loredanacirstea which v values shall we accept? {0,1} / {27, 28} / {37, 38} / {39, 40} / ... All of them?

{0, 1}: vanilla signature scheme.
{27, 28}: generic Ethereum signature (on all chains)
{37, 38}: Ethereum signature expected on chain_id 1.
{39, 40}: Ethereum signature expected on chain_id 2.
...

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 21, 2018

I discussed how to test the "BalanceProof signed with v=37" case with @hackaugusto . He prefers unit tests going forward because they are faster. This time, I think I should learn the integration test (because I have never touched the API myself) first. The next exercise will be to turn it into a unite test.

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Nov 21, 2018

@pirapira if it also involves a smart contract it's kind of hard to only unit test it -- because if you want a test that provides the same balance proof with v=37 to both python and contracts then you need to deploy them. This is an integration test.

Alternative approach would be unit tests only here in Raiden for various v values, and unit tests for various v values in the smart contracts repository.

@loredanacirstea

This comment has been minimized.

Collaborator

loredanacirstea commented Nov 21, 2018

The easy option would be to leave the contracts unchanged and have the Raiden client support only {0,1} / {27, 28}, as we already use the chain ID in the signed protocol messages, to protect against replay attacks. I would go with this for Red Eyes.

We can discuss using Eip155 for Ithaca and remove the chain_id from the protocol messages (replacing with eip155 v values)

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Nov 21, 2018

I don't think it's a problem for Raiden, because the v value is set on the signature instead of on the data to be signed, so Raiden client can simply change v to whatever value is accepted by the smartcontract before submitting the transaction. This value is meant to only make a difference when calculating a transaction signature/hash, avoiding replay attacks between chains, but is trivial on message signatures. Of course, we still need to ensure the client, besides validating the v value, uses a valid one when eventually submitting it on chain.

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Nov 21, 2018

For true replay protection on off-chain data signatures, the chainId must be inserted on the message to be hashed/signed, as done in EIP712's chainId field of the EIP712Domain component of the to-be-signed data, or trivially added to our eth_sign's/EIP191-version-byte-0x45 signatures by ensuring e.g. the first 2 bytes of the message (right after the length) is the chainId (and, of course, new_length=len(message)+2)

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 21, 2018

I talked with @loredanacirstea and @hackaugusto , and his solution was, for Red Eyes,

  • the Raiden client rejects signatures, if v isn't {27, 28}

But I noticed @andrevmatos 's solution is

  • the Raiden client changes the v value when it talks to the smart contract.

I prefer outright rejecting because I like systems that fail on slightest weirdity. Does this align with the team?

@pirapira pirapira changed the title from Tests with BalanceProof signed with `v` = 37 or 38? to Some signatures are accepted by Raiden client but rejected by smart contracts Nov 21, 2018

@andrevmatos

This comment has been minimized.

Collaborator

andrevmatos commented Nov 21, 2018

@pirapira I agree with first solution, also think its saner. The point is just that it isn't an attack, as the current signature received is valid, just a bug if these values aren't checked/sanitized.

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Nov 21, 2018

I prefer outright rejecting because I like systems that fail on slightest weirdity. Does this align with the team?

Yes outright rejecting sounds like a better approach.

@pirapira pirapira removed the security label Nov 21, 2018

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 21, 2018

Alright! Yes, anybody can change v so it's not security, but more like annoyance problem. So I took out the security tag.

@LefterisJP LefterisJP added this to the Red Eyes Testnet 18 milestone Nov 26, 2018

@LefterisJP LefterisJP self-assigned this Nov 26, 2018

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Nov 26, 2018

Thanks for finding this out @pirapira. I would even argue this is a critical bug since it can indeed lead to loss of funds. Someone can have a customized client who uses different v values other than 27 or 28 and send them to their partner who will gladly accept them, while the smart contract won't accept them at settlement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment