Skip to content

Commit

Permalink
Prevent replay attacks with NetworkID field: (#4370)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
RichardAH authored Apr 12, 2023
1 parent 066f91c commit 4f95b9d
Show file tree
Hide file tree
Showing 14 changed files with 269 additions and 0 deletions.
1 change: 1 addition & 0 deletions Builds/CMake/RippledCore.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,7 @@ if (tests)
src/test/app/LoadFeeTrack_test.cpp
src/test/app/Manifest_test.cpp
src/test/app/MultiSign_test.cpp
src/test/app/NetworkID_test.cpp
src/test/app/NFToken_test.cpp
src/test/app/NFTokenBurn_test.cpp
src/test/app/NFTokenDir_test.cpp
Expand Down
21 changes: 21 additions & 0 deletions src/ripple/app/tx/impl/Transactor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,27 @@ namespace ripple {
NotTEC
preflight0(PreflightContext const& ctx)
{
uint32_t const nodeNID = ctx.app.config().NETWORK_ID;
std::optional<uint32_t> const txNID = ctx.tx[~sfNetworkID];

if (nodeNID <= 1024)
{
// legacy networks have IDs 1024 and below. These networks cannot
// specify NetworkID in txn
if (txNID)
return telNETWORK_ID_MAKES_TX_NON_CANONICAL;
}
else
{
// new networks both require the field to be present and require it to
// match
if (!txNID)
return telREQUIRES_NETWORK_ID;

if (*txNID != nodeNID)
return telWRONG_NETWORK;
}

auto const txID = ctx.tx.getTransactionID();

if (txID == beast::zero)
Expand Down
1 change: 1 addition & 0 deletions src/ripple/core/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ class Config : public BasicConfig
std::string START_LEDGER;

// Network parameters
uint32_t NETWORK_ID = 0;

// DEPRECATED - Fee units for a reference transction.
// Only provided for backwards compatibility in a couple of places
Expand Down
1 change: 1 addition & 0 deletions src/ripple/core/ConfigSections.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ struct ConfigSection
#define SECTION_LEDGER_REPLAY "ledger_replay"
#define SECTION_BETA_RPC_API "beta_rpc_api"
#define SECTION_SWEEP_INTERVAL "sweep_interval"
#define SECTION_NETWORK_ID "network_id"

} // namespace ripple

Expand Down
12 changes: 12 additions & 0 deletions src/ripple/core/impl/Config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,18 @@ Config::loadFromString(std::string const& fileContents)

std::string strTemp;

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);
}

if (getSingleSection(secConfig, SECTION_PEER_PRIVATE, strTemp, j_))
PEER_PRIVATE = beast::lexicalCastThrow<bool>(strTemp);

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/SField.h
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,7 @@ extern SF_UINT16 const sfHookExecutionIndex;
extern SF_UINT16 const sfHookApiVersion;

// 32-bit integers (common)
extern SF_UINT32 const sfNetworkID;
extern SF_UINT32 const sfFlags;
extern SF_UINT32 const sfSourceTag;
extern SF_UINT32 const sfSequence;
Expand Down
3 changes: 3 additions & 0 deletions src/ripple/protocol/TER.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ enum TELcodes : TERUnderlyingType {
telCAN_NOT_QUEUE_BLOCKED,
telCAN_NOT_QUEUE_FEE,
telCAN_NOT_QUEUE_FULL,
telWRONG_NETWORK,
telREQUIRES_NETWORK_ID,
telNETWORK_ID_MAKES_TX_NON_CANONICAL
};

//------------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/SField.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
CONSTRUCT_TYPED_SFIELD(sfFlags, "Flags", UINT32, 2);
CONSTRUCT_TYPED_SFIELD(sfSourceTag, "SourceTag", UINT32, 3);
CONSTRUCT_TYPED_SFIELD(sfSequence, "Sequence", UINT32, 4);
Expand Down
3 changes: 3 additions & 0 deletions src/ripple/protocol/impl/TER.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ transResults()
MAKE_ERROR(telCAN_NOT_QUEUE_BLOCKED, "Can not queue at this time: blocking transaction in queue."),
MAKE_ERROR(telCAN_NOT_QUEUE_FEE, "Can not queue at this time: fee insufficient to replace queued transaction."),
MAKE_ERROR(telCAN_NOT_QUEUE_FULL, "Can not queue at this time: queue is full."),
MAKE_ERROR(telWRONG_NETWORK, "Transaction specifies a Network ID that differs from that of the local node."),
MAKE_ERROR(telREQUIRES_NETWORK_ID, "Transactions submitted to this node/network must include a correct NetworkID field."),
MAKE_ERROR(telNETWORK_ID_MAKES_TX_NON_CANONICAL, "Transactions submitted to this node/network must NOT include a NetworkID field."),

MAKE_ERROR(temMALFORMED, "Malformed transaction."),
MAKE_ERROR(temBAD_AMOUNT, "Can only send positive amounts."),
Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/TxFormats.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ TxFormats::TxFormats()
{sfSigningPubKey, soeREQUIRED},
{sfTxnSignature, soeOPTIONAL},
{sfSigners, soeOPTIONAL}, // submit_multisigned
{sfNetworkID, soeOPTIONAL},
};

add(jss::AccountSet,
Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/jss.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ JSS(Invalid); //
JSS(LastLedgerSequence); // in: TransactionSign; field
JSS(LedgerHashes); // ledger type.
JSS(LimitAmount); // field.
JSS(NetworkID); // field.
JSS(NFTokenBurn); // transaction type.
JSS(NFTokenMint); // transaction type.
JSS(NFTokenOffer); // ledger type.
Expand Down
154 changes: 154 additions & 0 deletions src/test/app/NetworkID_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
//------------------------------------------------------------------------------
/*
This file is part of rippled: https:/ripple/rippled
Copyright (c) 2020 Dev Null Productions
Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.
THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
//==============================================================================

#include <ripple/basics/BasicConfig.h>
#include <ripple/core/ConfigSections.h>
#include <ripple/protocol/jss.h>
#include <test/jtx.h>
#include <test/jtx/Env.h>

namespace ripple {
namespace test {

class NetworkID_test : public beast::unit_test::suite
{
public:
void
run() override
{
testNetworkID();
}

std::unique_ptr<Config>
makeNetworkConfig(uint32_t networkID)
{
using namespace jtx;
return envconfig([&](std::unique_ptr<Config> cfg) {
cfg->NETWORK_ID = networkID;
return cfg;
});
}

void
testNetworkID()
{
testcase(
"Require txn NetworkID to be specified (or not) depending on the "
"network ID of the node");
using namespace jtx;

auto const alice = Account{"alice"};

auto const runTx = [&](test::jtx::Env& env,
Json::Value const& jv,
TER expectedOutcome) {
env.memoize(env.master);
env.memoize(alice);

// fund alice
{
Json::Value jv;
jv[jss::Account] = env.master.human();
jv[jss::Destination] = alice.human();
jv[jss::TransactionType] = "Payment";
jv[jss::Amount] = "10000000000";
if (env.app().config().NETWORK_ID > 1024)
jv[jss::NetworkID] =
std::to_string(env.app().config().NETWORK_ID);

env(jv, fee(1000), sig(env.master));
}

// run tx
env(jv, fee(1000), ter(expectedOutcome));
env.close();
};

// test mainnet
{
test::jtx::Env env{*this, makeNetworkConfig(0)};
BEAST_EXPECT(env.app().config().NETWORK_ID == 0);

// try to submit a txn without network id, this should work
Json::Value jv;
jv[jss::Account] = alice.human();
jv[jss::TransactionType] = jss::AccountSet;
runTx(env, jv, tesSUCCESS);

// try to submit a txn with NetworkID present against a mainnet
// node, this will fail
jv[jss::NetworkID] = 0;
runTx(env, jv, telNETWORK_ID_MAKES_TX_NON_CANONICAL);

// change network id to something else, should still return same
// error
jv[jss::NetworkID] = 10000;
runTx(env, jv, telNETWORK_ID_MAKES_TX_NON_CANONICAL);
}

// any network up to and including networkid 1024 cannot support
// NetworkID
{
test::jtx::Env env{*this, makeNetworkConfig(1024)};
BEAST_EXPECT(env.app().config().NETWORK_ID == 1024);

// try to submit a txn without network id, this should work
Json::Value jv;
jv[jss::Account] = alice.human();
jv[jss::TransactionType] = jss::AccountSet;
runTx(env, jv, tesSUCCESS);

// now submit with a network id, this will fail
jv[jss::NetworkID] = 1024;
runTx(env, jv, telNETWORK_ID_MAKES_TX_NON_CANONICAL);

jv[jss::NetworkID] = 1000;
runTx(env, jv, telNETWORK_ID_MAKES_TX_NON_CANONICAL);
}

// any network above networkid 1024 will produce an error if fed a txn
// absent networkid
{
test::jtx::Env env{*this, makeNetworkConfig(1025)};
BEAST_EXPECT(env.app().config().NETWORK_ID == 1025);

// try to submit a txn without network id, this should not work
Json::Value jv;
jv[jss::Account] = alice.human();
jv[jss::TransactionType] = jss::AccountSet;
runTx(env, jv, telREQUIRES_NETWORK_ID);

// try to submit with wrong network id
jv[jss::NetworkID] = 0;
runTx(env, jv, telWRONG_NETWORK);

jv[jss::NetworkID] = 1024;
runTx(env, jv, telWRONG_NETWORK);

// submit the correct network id
jv[jss::NetworkID] = 1025;
runTx(env, jv, tesSUCCESS);
}
}
};

BEAST_DEFINE_TESTSUITE(NetworkID, app, ripple);

} // namespace test
} // namespace ripple
66 changes: 66 additions & 0 deletions src/test/core/Config_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,71 @@ port_wss_admin
}
}

void
testNetworkID()
{
testcase("network id");
std::string error;
Config c;
try
{
c.loadFromString(R"rippleConfig(
[network_id]
main
)rippleConfig");
}
catch (std::runtime_error& e)
{
error = e.what();
}

BEAST_EXPECT(error == "");
BEAST_EXPECT(c.NETWORK_ID == 0);

try
{
c.loadFromString(R"rippleConfig(
)rippleConfig");
}
catch (std::runtime_error& e)
{
error = e.what();
}

BEAST_EXPECT(error == "");
BEAST_EXPECT(c.NETWORK_ID == 0);

try
{
c.loadFromString(R"rippleConfig(
[network_id]
255
)rippleConfig");
}
catch (std::runtime_error& e)
{
error = e.what();
}

BEAST_EXPECT(error == "");
BEAST_EXPECT(c.NETWORK_ID == 255);

try
{
c.loadFromString(R"rippleConfig(
[network_id]
10000
)rippleConfig");
}
catch (std::runtime_error& e)
{
error = e.what();
}

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

void
testValidatorsFile()
{
Expand Down Expand Up @@ -1230,6 +1295,7 @@ r.ripple.com:51235
testGetters();
testAmendment();
testOverlay();
testNetworkID();
}
};

Expand Down
3 changes: 3 additions & 0 deletions src/test/jtx/impl/Env.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Throw<std::runtime_error>("Env::lookup:: unknown account ID");
}
return iter->second;
}

Expand Down

0 comments on commit 4f95b9d

Please sign in to comment.