From 38c3a46a337916216a1e3ad481229fe87e07d542 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 10 Aug 2018 15:05:39 -0700 Subject: [PATCH] Deprecate commands that perform remote tx signing (RIPD-1649): In order to facilitate transaction signing, `rippled` offers the `sign` and `sign_for` and `submit` commands, which, given a seed, can be used to sign or sign-and-submit transactions. These commands are accessible from the command line, as well as over the WebSocket and RPC interfaces that `rippled` can be configured to provide. These commands, unfortunately, have significant security implications: 1. They require divulging an account's seed (commonly known as a "secret key") to the server. 2. When executing these commands against remote servers, the seeds can be transported over clear-text links. 3. When executing these commands over the command line, the account seed may be visible using common tools that show running processes and may potentially be inadvertently stored by system monitoring tools or facilities designed to maintain a history of previously typed commands. While this commit cannot prevent users from issuing these commands to a server, whether locally or remotely, it restricts the `sign` and `sign_for` commands, as well as the `submit` command when used to sign-and-submit, so that they require administrative privileges on the server. Server operators that want to allow unrestricted signing can do so by adding the following stanza to their configuration file: [signing_support] true Ripple discourages server operators from doing so and advises against using these commands, which will be removed in a future release. If you rely on these commands for signing, please migrate to a standalone signing solution as soon as possible. One option is to use `ripple-lib`; documentation is available at https://developers.ripple.com/rippleapi-reference.html#sign. If the commands are administratively enabled, the server includes a warning on startup and adds a new field in the resulting JSON, informing the caller that the commands are deprecated and may become unavailable at any time. Acknowledgements: Jesper Wallin for reporting this issue to Ripple. Bug Bounties and Responsible Disclosures: We welcome reviews of the rippled code and urge researchers to responsibly disclose any issues that they may find. For more on Ripple's Bug Bounty program, please visit: https://ripple.com/bug-bounty --- cfg/rippled-example.cfg | 27 ++++++++++++++++++- src/ripple/app/main/Application.cpp | 20 ++++++++++++++ src/ripple/core/Config.h | 10 +++++++ src/ripple/core/ConfigSections.h | 1 + src/ripple/core/impl/Config.cpp | 3 +++ src/ripple/rpc/handlers/SignFor.cpp | 20 +++++++++----- src/ripple/rpc/handlers/SignHandler.cpp | 21 ++++++++++----- src/ripple/rpc/handlers/Submit.cpp | 22 ++++++++++----- src/ripple/rpc/impl/RPCHandler.cpp | 17 +++++++++++- src/ripple/rpc/impl/ServerHandlerImp.cpp | 34 ++++++++++++++++++++++-- src/test/app/MultiSign_test.cpp | 20 +++++++++++--- src/test/rpc/AmendmentBlocked_test.cpp | 7 ++++- src/test/rpc/JSONRPC_test.cpp | 2 ++ src/test/rpc/RPCOverload_test.cpp | 7 ++++- 14 files changed, 183 insertions(+), 28 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 5b20becb23..0049dd41fa 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -1002,7 +1002,32 @@ # #------------------------------------------------------------------------------- # -# 8. Example Settings +# 8. Misc Settings +# +#---------- +# +# [signing_support] +# +# Specifies whether the server will accept "sign" and "sign_for" commands +# from remote users. Even if the commands are sent over a secure protocol +# like secure websocket, this should generally be discouraged, because it +# requires sending the secret to use for signing to the server. In order +# to sign transactions, users should prefer to use a standalone signing +# tool instead. +# +# This flag has no effect on the "sign" and "sign_for" command line options +# that rippled makes available. +# +# The default value of this field is "false" +# +# Example: +# +# [signing_support] +# true +# +#------------------------------------------------------------------------------- +# +# 9. Example Settings # #-------------------- # diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 6117d2d315..3766787ca6 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1399,6 +1399,26 @@ bool ApplicationImp::setup() m_networkOPs->setStandAlone (); } + if (config_->canSign()) + { + JLOG(m_journal.warn()) << + "*** The server is configured to allow the 'sign' and 'sign_for'"; + JLOG(m_journal.warn()) << + "*** commands. These commands have security implications and have"; + JLOG(m_journal.warn()) << + "*** been deprecated. They will be removed in a future release of"; + JLOG(m_journal.warn()) << + "*** rippled."; + JLOG(m_journal.warn()) << + "*** If you do not use them to sign transactions please edit your"; + JLOG(m_journal.warn()) << + "*** configuration file and remove the [enable_signing] stanza."; + JLOG(m_journal.warn()) << + "*** If you do use them to sign transactions please migrate to a"; + JLOG(m_journal.warn()) << + "*** standalone signing solution as soon as possible."; + } + // // Execute start up rpc commands. // diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 14babaf8cf..6e929c1494 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -107,6 +107,14 @@ private: */ bool RUN_STANDALONE = false; + /** Determines if the server will sign a tx, given an account's secret seed. + + In the past, this was allowed, but this functionality can have security + implications. The new default is to not allow this functionality, but + a config option is included to enable this. + */ + bool signingEnabled_ = false; + public: bool doImport = false; bool nodeToShard = false; @@ -196,6 +204,8 @@ public: bool quiet() const { return QUIET; } bool silent() const { return SILENT; } bool standalone() const { return RUN_STANDALONE; } + + bool canSign() const { return signingEnabled_; } }; } // ripple diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index f0817f527a..bc4e36478f 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -58,6 +58,7 @@ struct ConfigSection #define SECTION_PEER_PRIVATE "peer_private" #define SECTION_PEERS_MAX "peers_max" #define SECTION_RPC_STARTUP "rpc_startup" +#define SECTION_SIGNING_SUPPORT "signing_support" #define SECTION_SNTP "sntp_servers" #define SECTION_SSL_VERIFY "ssl_verify" #define SECTION_SSL_VERIFY_FILE "ssl_verify_file" diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index fcae22281b..1242d245f1 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -345,6 +345,9 @@ void Config::loadFromString (std::string const& fileContents) } } + if (getSingleSection (secConfig, SECTION_SIGNING_SUPPORT, strTemp, j_)) + signingEnabled_ = beast::lexicalCastThrow (strTemp); + if (getSingleSection (secConfig, SECTION_ELB_SUPPORT, strTemp, j_)) ELB_SUPPORT = beast::lexicalCastThrow (strTemp); diff --git a/src/ripple/rpc/handlers/SignFor.cpp b/src/ripple/rpc/handlers/SignFor.cpp index 6fb46ab33a..2a4ab53067 100644 --- a/src/ripple/rpc/handlers/SignFor.cpp +++ b/src/ripple/rpc/handlers/SignFor.cpp @@ -33,6 +33,12 @@ namespace ripple { // } Json::Value doSignFor (RPC::Context& context) { + if (context.role != Role::ADMIN && !context.app.config().canSign()) + { + return RPC::make_error (rpcNOT_SUPPORTED, + "Signing is not supported by this server."); + } + // Bail if multisign is not enabled. if (! context.app.getLedgerMaster().getValidatedRules(). enabled (featureMultiSign)) @@ -44,12 +50,14 @@ Json::Value doSignFor (RPC::Context& context) auto const failHard = context.params[jss::fail_hard].asBool(); auto const failType = NetworkOPs::doFailHard (failHard); - return RPC::transactionSignFor ( - context.params, - failType, - context.role, - context.ledgerMaster.getValidatedLedgerAge(), - context.app); + auto ret = RPC::transactionSignFor ( + context.params, failType, context.role, + context.ledgerMaster.getValidatedLedgerAge(), context.app); + + ret[jss::deprecated] = "This command has been deprecated and will be " + "removed in a future version of the server. Please " + "migrate to a standalone signing tool."; + return ret; } } // ripple diff --git a/src/ripple/rpc/handlers/SignHandler.cpp b/src/ripple/rpc/handlers/SignHandler.cpp index f95ce7b429..a19535bbbe 100644 --- a/src/ripple/rpc/handlers/SignHandler.cpp +++ b/src/ripple/rpc/handlers/SignHandler.cpp @@ -31,18 +31,27 @@ namespace ripple { // } Json::Value doSign (RPC::Context& context) { + if (context.role != Role::ADMIN && !context.app.config().canSign()) + { + return RPC::make_error (rpcNOT_SUPPORTED, + "Signing is not supported by this server."); + } + context.loadType = Resource::feeHighBurdenRPC; NetworkOPs::FailHard const failType = NetworkOPs::doFailHard ( context.params.isMember (jss::fail_hard) && context.params[jss::fail_hard].asBool ()); - return RPC::transactionSign ( - context.params, - failType, - context.role, - context.ledgerMaster.getValidatedLedgerAge(), - context.app); + auto ret = RPC::transactionSign ( + context.params, failType, context.role, + context.ledgerMaster.getValidatedLedgerAge(), context.app); + + ret[jss::deprecated] = "This command has been deprecated and will be " + "removed in a future version of the server. Please " + "migrate to a standalone signing tool."; + + return ret; } } // ripple diff --git a/src/ripple/rpc/handlers/Submit.cpp b/src/ripple/rpc/handlers/Submit.cpp index e74ee824d6..0668ed0e44 100644 --- a/src/ripple/rpc/handlers/Submit.cpp +++ b/src/ripple/rpc/handlers/Submit.cpp @@ -48,13 +48,21 @@ Json::Value doSubmit (RPC::Context& context) { auto const failType = getFailHard (context); - return RPC::transactionSubmit ( - context.params, - failType, - context.role, - context.ledgerMaster.getValidatedLedgerAge(), - context.app, - RPC::getProcessTxnFn (context.netOps)); + if (context.role != Role::ADMIN && !context.app.config().canSign()) + return RPC::make_error (rpcNOT_SUPPORTED, + "Signing is not supported by this server."); + + auto ret = RPC::transactionSubmit ( + context.params, failType, context.role, + context.ledgerMaster.getValidatedLedgerAge(), + context.app, RPC::getProcessTxnFn (context.netOps)); + + ret[jss::deprecated] = "Signing support in the 'submit' command has been " + "deprecated and will be removed in a future version " + "of the server. Please migrate to a standalone " + "signing tool."; + + return ret; } Json::Value jvResult; diff --git a/src/ripple/rpc/impl/RPCHandler.cpp b/src/ripple/rpc/impl/RPCHandler.cpp index 1a1a153839..1afc1c4c16 100644 --- a/src/ripple/rpc/impl/RPCHandler.cpp +++ b/src/ripple/rpc/impl/RPCHandler.cpp @@ -235,7 +235,22 @@ void getResult ( { JLOG (context.j.debug()) << "rpcError: " << status.toString(); result[jss::status] = jss::error; - result[jss::request] = context.params; + + auto rq = context.params; + + if (rq.isObject()) + { + if (rq.isMember(jss::passphrase.c_str())) + rq[jss::passphrase.c_str()] = ""; + if (rq.isMember(jss::secret.c_str())) + rq[jss::secret.c_str()] = ""; + if (rq.isMember(jss::seed.c_str())) + rq[jss::seed.c_str()] = ""; + if (rq.isMember(jss::seed_hex.c_str())) + rq[jss::seed_hex.c_str()] = ""; + } + + result[jss::request] = rq; } else { diff --git a/src/ripple/rpc/impl/ServerHandlerImp.cpp b/src/ripple/rpc/impl/ServerHandlerImp.cpp index 967efcd160..1d8f56c5e5 100644 --- a/src/ripple/rpc/impl/ServerHandlerImp.cpp +++ b/src/ripple/rpc/impl/ServerHandlerImp.cpp @@ -469,7 +469,22 @@ ServerHandlerImp::processSession( { jr = jr[jss::result]; jr[jss::status] = jss::error; - jr[jss::request] = jv; + + auto rq = jv; + + if (rq.isObject()) + { + if (rq.isMember(jss::passphrase.c_str())) + rq[jss::passphrase.c_str()] = ""; + if (rq.isMember(jss::secret.c_str())) + rq[jss::secret.c_str()] = ""; + if (rq.isMember(jss::seed.c_str())) + rq[jss::seed.c_str()] = ""; + if (rq.isMember(jss::seed_hex.c_str())) + rq[jss::seed_hex.c_str()] = ""; + } + + jr[jss::request] = rq; } else { @@ -780,8 +795,23 @@ ServerHandlerImp::processRequest (Port const& port, // Always report "status". On an error report the request as received. if (result.isMember (jss::error)) { + auto rq = params; + + if (rq.isObject()) + { // But mask potentially sensitive information. + if (rq.isMember(jss::passphrase.c_str())) + rq[jss::passphrase.c_str()] = ""; + if (rq.isMember(jss::secret.c_str())) + rq[jss::secret.c_str()] = ""; + if (rq.isMember(jss::seed.c_str())) + rq[jss::seed.c_str()] = ""; + if (rq.isMember(jss::seed_hex.c_str())) + rq[jss::seed_hex.c_str()] = ""; + } + result[jss::status] = jss::error; - result[jss::request] = params; + result[jss::request] = rq; + JLOG (m_journal.debug()) << "rpcError: " << result [jss::error] << ": " << result [jss::error_message]; diff --git a/src/test/app/MultiSign_test.cpp b/src/test/app/MultiSign_test.cpp index 0dde40e986..3145fd7029 100644 --- a/src/test/app/MultiSign_test.cpp +++ b/src/test/app/MultiSign_test.cpp @@ -15,6 +15,7 @@ */ //============================================================================== +#include #include // jss:: definitions #include #include @@ -190,7 +191,12 @@ public: test_enablement() { using namespace jtx; - Env env(*this, FeatureBitset{}); + Env env(*this, envconfig([](std::unique_ptr cfg) + { + cfg->loadFromString ("[" SECTION_SIGNING_SUPPORT "]\ntrue"); + return cfg; + }), FeatureBitset{}); + Account const alice {"alice", KeyType::ed25519}; env.fund(XRP(1000), alice); env.close(); @@ -414,7 +420,11 @@ public: void test_regularSignersUsingSubmitMulti() { using namespace jtx; - Env env(*this); + Env env(*this, envconfig([](std::unique_ptr cfg) + { + cfg->loadFromString ("[" SECTION_SIGNING_SUPPORT "]\ntrue"); + return cfg; + })); Account const alice {"alice", KeyType::secp256k1}; Account const becky {"becky", KeyType::ed25519}; Account const cheri {"cheri", KeyType::secp256k1}; @@ -1142,7 +1152,11 @@ public: using namespace jtx; Account const alice {"alice", KeyType::ed25519}; - Env env(*this); + Env env(*this, envconfig([](std::unique_ptr cfg) + { + cfg->loadFromString ("[" SECTION_SIGNING_SUPPORT "]\ntrue"); + return cfg; + })); env.fund (XRP(1000), alice); env.close(); diff --git a/src/test/rpc/AmendmentBlocked_test.cpp b/src/test/rpc/AmendmentBlocked_test.cpp index 2cc61c9d8c..9a8093959d 100644 --- a/src/test/rpc/AmendmentBlocked_test.cpp +++ b/src/test/rpc/AmendmentBlocked_test.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include #include @@ -29,7 +30,11 @@ class AmendmentBlocked_test : public beast::unit_test::suite void testBlockedMethods() { using namespace test::jtx; - Env env {*this}; + Env env {*this, envconfig([](std::unique_ptr cfg) + { + cfg->loadFromString ("[" SECTION_SIGNING_SUPPORT "]\ntrue"); + return cfg; + })}; auto const gw = Account {"gateway"}; auto const USD = gw["USD"]; auto const alice = Account {"alice"}; diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index aaf363d9df..1d58c5c02e 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -1948,6 +1949,7 @@ public: using namespace test::jtx; Env env {*this, envconfig([](std::unique_ptr cfg) { + cfg->loadFromString ("[" SECTION_SIGNING_SUPPORT "]\ntrue"); cfg->section("transaction_queue") .set("minimum_txn_in_ledger_standalone", "3"); return cfg; diff --git a/src/test/rpc/RPCOverload_test.cpp b/src/test/rpc/RPCOverload_test.cpp index cdf6a364a9..bad8b955bb 100644 --- a/src/test/rpc/RPCOverload_test.cpp +++ b/src/test/rpc/RPCOverload_test.cpp @@ -15,6 +15,7 @@ */ //============================================================================== +#include #include #include #include @@ -31,7 +32,11 @@ public: { testcase << "Overload " << (useWS ? "WS" : "HTTP") << " RPC client"; using namespace jtx; - Env env {*this, envconfig(no_admin)}; + Env env {*this, envconfig([](std::unique_ptr cfg) + { + cfg->loadFromString ("[" SECTION_SIGNING_SUPPORT "]\ntrue"); + return no_admin(std::move(cfg)); + })}; Account const alice {"alice"}; Account const bob {"bob"};