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

Add NetworkID field to transactions to help prevent replay attacks on and from side-chains #4370

Merged
merged 5 commits into from
Apr 12, 2023

Conversation

RichardAH
Copy link
Collaborator

@RichardAH RichardAH commented Dec 19, 2022

High Level Overview of Change

Help to prevent replay attacks by allowing side-chains to require their users specify an accurate NetworkID field in every transaction to that chain.

This change introduces a new field:

  • CONSTRUCT_TYPED_SFIELD(sfNetworkID, "NetworkID", UINT32, 1);

And three new local error codes:

  • telNETWORK_ID_MAKES_TX_NON_CANONICAL
  • telREQUIRES_NETWORK_ID
  • telWRONG_NETWORK.

To preserve legacy behaviour (and avoid the need for an amendment) all chains with a network ID less than 1025 retain the existing behaviour. This includes main, testnet, devnet and hooks-testnet. If sfNetworkID is present in any txn submitted to any of the nodes on this chain then a telNETWORK_ID_MAKES_TX_NON_CANONICAL is returned.

All other chains now require their users to include an appropriate sfNetworkID in every transaction to that chain.

Local error codes were chosen because a transaction is not necessarily malformed if it is submitted to a node running on the incorrect chain. This is a local error specific to that node and could be corrected by switching to a different node or by changing the network_id on that node.

Test Plan

A new unit test suite has been added:

# ./rippled -u ripple.app.NetworkID
ripple.app.NetworkID Require txn NetworkID to be specified (or not) depending on the network ID of the node
501ms, 1 suite, 1 case, 23 tests total, 0 failures

Config unit test was also updated to include some network_id tests:

# ./rippled -u ripple.core.Config
ripple.core.Config legacy
ripple.core.Config database_path
Loading: "testSetup12/rippled.cfg"
Loading: "testSetup12/rippled.cfg"
Loading: "testSetup13/rippled.cfg"
Loading: "testSetup13/rippled.cfg"
ripple.core.Config validator keys
ripple.core.Config validators_file
ripple.core.Config amendment
ripple.core.Config overlay: unknown time
ripple.core.Config overlay: diverged time
ripple.core.Config network id
101ms, 1 suite, 8 cases, 260 tests total, 0 failure

All tests:

Longest suite times:
   42.0s ripple.tx.Offer
   35.0s ripple.app.ValidatorSite
   21.7s ripple.rpc.NodeToShardRPC
   20.6s ripple.tx.NFToken
   17.3s ripple.tx.NFTokenBurn
   13.9s ripple.app.Flow
   13.7s ripple.app.ShardArchiveHandler
   13.0s ripple.app.TheoreticalQuality
    8.3s ripple.app.LedgerReplayer
    7.9s ripple.tx.NFTokenDir
337.1s, 205 suites, 1529 cases, 532189 tests total, 0 failures

@intelliot
Copy link
Collaborator

This is worth considering; however, I believe the recommendation is to use different accounts/keys on side-chains. The X-address format recommends using the X prefix for Mainnet and T for any test/development network. Additional prefixes could be defined for well-known sidechains; there could also be a prefix for side-chains in general.

@RichardAH
Copy link
Collaborator Author

This is worth considering; however, I believe the recommendation is to use different accounts/keys on side-chains. The X-address format recommends using the X prefix for Mainnet and T for any test/development network. Additional prefixes could be defined for well-known sidechains; there could also be a prefix for side-chains in general.

This wouldn't prevent replay attacks, it's just a client side syntactic sugar over the top of existing AccountIDs and destination tags. The resulting serialized transactions are the same and can be replayed in the same way on other chains.

Realistically people will absolutely use the same keys on multiple chains. This is an exceedingly common practice on Ethereum chains. There are even legitimate use-cases for it... such as swapping from your own account on one chain to your own account on another over a bridge without attracting a lot of unwanted/unnecessary AML legislation.

@intelliot
Copy link
Collaborator

Yes, you are correct. It is an issue if people use the same keys on multiple chains. It may be common practice, but I'm loathe to recommend it, partly because that does open up the risk of replay attacks.

Anyway, I'm fine with requiring NetworkID on new chains (as you said - Mainnet would remain unchanged). Let's get community feedback and get this reviewed.

@RichardAH
Copy link
Collaborator Author

Ultimately the code doesn’t affect mainnet at all (and thus doesn’t require an amendment). It’s here to automatically apply to future side chains (which I assume there will be quite a few of given the resources going into XChain amendment).

If the PR is rejected we should still ensure UINT32 field code 1 is reserved for NetworkID. As long as everyone agrees on that then dealing with multiple chains in the future will be a lot easier.

@intelliot
Copy link
Collaborator

Agreed - that's why I didn't put the Amendment label on this

Copy link
Collaborator

@mDuo13 mDuo13 left a comment

Choose a reason for hiding this comment

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

Good change. Just curious, where did you get the 1024 cutoff? Personally I think I'd rather make it so that every non-Mainnet network requires a NetworkID, but I guess it would be disruptive to reset all the test networks just for this.

src/ripple/app/tx/impl/Transactor.cpp Outdated Show resolved Hide resolved
Comment on lines +482 to +492
if (getSingleSection(secConfig, SECTION_NETWORK_ID, strTemp, j_))
{
if (strTemp == "main")
NETWORK_ID = 0;
else if (strTemp == "testnet")
NETWORK_ID = 1;
else if (strTemp == "devnet")
NETWORK_ID = 2;
else
NETWORK_ID = beast::lexicalCastThrow<uint32_t>(strTemp);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed this section is similar to the OverlayImpl.cpp version; is that something that can be updated to be consistent or shared in some way so this switch doesn't have to be in multiple places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also saw this but didn't want to touch OverlayImpl. Feel free to suggest an additional commit with appropriate tests to remove this redundant code.

@RichardAH
Copy link
Collaborator Author

Good change. Just curious, where did you get the 1024 cutoff? Personally I think I'd rather make it so that every non-Mainnet network requires a NetworkID, but I guess it would be disruptive to reset all the test networks just for this.

It comes from the restriction on port numbers in Linux. We feel that new networks should pick a peering port that is = to their network ID, and this should always be > 1024. :)

Co-authored-by: Rome Reginelli <mduo13@gmail.com>
@SFsourDoh
Copy link

should this be "help prevent replay attacks" since network IDs might not be unique?

@RichardAH RichardAH changed the title Add NetworkID field to transactions to prevent replay attacks on and from side-chains Add NetworkID field to transactions to help prevent replay attacks on and from side-chains Dec 22, 2022
@@ -104,6 +104,7 @@ CONSTRUCT_TYPED_SFIELD(sfHookExecutionIndex, "HookExecutionIndex", UINT16,
CONSTRUCT_TYPED_SFIELD(sfHookApiVersion, "HookApiVersion", UINT16, 20);

// 32-bit integers (common)
CONSTRUCT_TYPED_SFIELD(sfNetworkID, "NetworkID", UINT32, 1);
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 not resue the unused value unless you are confidente that its not used elsewhere. Also why use the ''common'' value? To save a one byte in the encoding version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’ve never seen UINT32 field code 1 used anywhere. Not even in old old ledgers. Maybe someone can shed light on why it was free?
It seems appropriate to use it for an important task such as preventing replay attacks.

@intelliot
Copy link
Collaborator

Requires at least 2 full code reviews from maintainers or well-known C++ engineers

Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@intelliot
Copy link
Collaborator

Suggested commit message:

Prevent replay attacks with NetworkID field: (#4370)

Add a `NetworkID` field to help prevent replay attacks on and from
side-chains.

The new field must be used when the server is using a network id > 1024.

To preserve legacy behavior, all chains with a network ID less than 1025
retain the existing behavior. This includes Mainnet, Testnet, Devnet,
and hooks-testnet. If `sfNetworkID` is present in any transaction
submitted to any of the nodes on one of these chains, then
`telNETWORK_ID_MAKES_TX_NON_CANONICAL` is returned.

Since chains with a network ID less than 1025, including Mainnet, retain
the existing behavior, there is no need for an amendment.

The `NetworkID` helps to prevent replay attacks because users specify a
`NetworkID` field in every transaction for that chain.

This change introduces a new UINT32 field, `sfNetworkID` ("NetworkID").
There are also three new local error codes for transaction results:

- `telNETWORK_ID_MAKES_TX_NON_CANONICAL`
- `telREQUIRES_NETWORK_ID`
- `telWRONG_NETWORK`

To learn about the other transaction result codes, see
https://xrpl.org/transaction-results.html.

Local error codes were chosen because a transaction is not necessarily
malformed if it is submitted to a node running on the incorrect chain.
This is a local error specific to that node and could be corrected by
switching to a different node or by changing the `network_id` on that
node. See
https://xrpl.org/connect-your-rippled-to-the-xrp-test-net.html.

In addition to using `NetworkID`, it is still generally recommended to
use different accounts and keys on side-chains. However, people will
undoubtedly use the same keys on multiple chains; for example, this is
common practice on other blockchain networks. There are also some
legitimate use cases for this.

A `app.NetworkID` test suite has been added, and `core.Config` was
updated to include some network_id tests.

Copy link
Contributor

@pwang200 pwang200 left a comment

Choose a reason for hiding this comment

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

I have a couple minor comments, but looks good!

@@ -163,7 +163,10 @@ Env::lookup(AccountID const& id) const
{
auto const iter = map_.find(id);
if (iter == map_.end())
{
std::cout << "Unknown account: " << id << "\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider to remove.

Choose a reason for hiding this comment

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

How?


BEAST_EXPECT(error == "");
BEAST_EXPECT(c.NETWORK_ID == 10000);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider adding a negative case, e.g.
[network_id]
abcd

@intelliot
Copy link
Collaborator

explanation:

@pwang200 approved the PR. The suggestions could be incorporated (by @pwang200) in a commit (to be cherry picked); or if this PR has been merged, they can be suggested in a new PR.

@RichardAH confirmed this is good to go as-is.

@will-yjn
Copy link

will-yjn commented Jun 21, 2023

For mainnet/testnet/devnet, do we have to upgrade js sdk or add networkId when we construct the transaction? Is the new networkID mechanism already on testnet/devnet so we could test?

@intelliot
Copy link
Collaborator

This change does not affect mainnet/testnet/devnet.

To preserve legacy behaviour (and avoid the need for an amendment) all chains with a network ID less than 1025 retain the existing behaviour. This includes main, testnet, devnet and hooks-testnet.

The NetworkID field must not be set in transactions on these networks.

Only sidechains and test networks with IDs of 1025 or higher need to have the NetworkID set.

intelliot added a commit to intelliot/rippled that referenced this pull request Jul 14, 2023
intelliot pushed a commit that referenced this pull request Oct 19, 2023
The Network ID logic should not be applied to pseudo-transactions.

This allows amendments to enable on a network with an ID > 1024.

Context:
- NetworkID: #4370
- Pseudo-transactions: https://xrpl.org/pseudo-transaction-types.html

Fix #4736

---------

Co-authored-by: RichardAH <richard.holland@starstone.co.nz>
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
The Network ID logic should not be applied to pseudo-transactions.

This allows amendments to enable on a network with an ID > 1024.

Context:
- NetworkID: XRPLF#4370
- Pseudo-transactions: https://xrpl.org/pseudo-transaction-types.html

Fix XRPLF#4736

---------

Co-authored-by: RichardAH <richard.holland@starstone.co.nz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Used to indicate requests to add new features Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Testable Will Need Documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.