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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Builds/CMake/RippledCore.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,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 @@ -138,6 +138,7 @@ class Config : public BasicConfig
std::string START_LEDGER;

// Network parameters
uint32_t NETWORK_ID = 0;

// The number of fee units a reference transaction costs
static constexpr FeeUnit32 TRANSACTION_FEE_BASE{10};
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 @@ -101,6 +101,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 @@ -479,6 +479,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);
}
Comment on lines +482 to +492
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.


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

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://github.com/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 <test/jtx.h>
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
#include <test/jtx/Env.h>
#include <ripple/protocol/jss.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);
}
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


void
testValidatorsFile()
{
Expand Down Expand Up @@ -1151,6 +1216,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";
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?

Throw<std::runtime_error>("Env::lookup:: unknown account ID");
}
return iter->second;
}

Expand Down