sign_for RPC command fixes (RIPD-182):

o Remove warning written to log by sign_for command. o The sign_for RPC command previously only worked in the "json sign_for" form. The command now works as a straight "sign_for". The "offline" parameter also works. o Don't autofill Fee or Paths when signing offline.
2025-12-06 17:27:52 +00:00 · 2015-09-17 17:56:12 -07:00
parent 780a553662
commit c28c516b22
8 changed files with 212 additions and 88 deletions
--- a/src/ripple/app/main/Main.cpp
+++ b/src/ripple/app/main/Main.cpp
@@ -150,7 +150,7 @@ void printHelp (const po::options_description& desc)
           "     version\n"
           "     server_info\n"
           "     sign <private_key> <json> [offline]\n"
-           "     sign_for\n"
+           "     sign_for <signer_address> <signer_private_key> <json> [offline]\n"
           "     stop\n"
           "     submit <tx_blob>|[<private_key> <json>]\n"
           "     submit_multisigned\n"
--- a/src/ripple/app/tx/Transaction.h
+++ b/src/ripple/app/tx/Transaction.h
@@ -66,10 +66,6 @@ public:
    Transaction (
        std::shared_ptr<STTx const> const&, std::string&, Application&) noexcept;

-    static
-    Transaction::pointer
-    sharedTransaction (Blob const&, Rules const& rules, Application& app);
-
    static
    Transaction::pointer
    transactionFromSQL (
--- a/src/ripple/app/tx/impl/Transaction.cpp
+++ b/src/ripple/app/tx/impl/Transaction.cpp
@@ -56,37 +56,6 @@ Transaction::Transaction (std::shared_ptr<STTx const> const& stx,
    mStatus = NEW;
 }

-Transaction::pointer Transaction::sharedTransaction (
-    Blob const& vucTransaction, Rules const& rules, Application& app)
-{
-    try
-    {
-        SerialIter sit (makeSlice(vucTransaction));
-        auto txn = std::make_shared<STTx const>(sit);
-        std::string reason;
-        auto result = std::make_shared<Transaction> (
-            txn, reason, app);
-        if (checkValidity(app.getHashRouter(),
-            *txn, rules, app.config()).first
-                != Validity::Valid)
-        {
-            result->setStatus(INVALID);
-        }
-        return result;
-    }
-    catch (std::exception& e)
-    {
-        JLOG (app.journal ("Ledger").warning) << "Exception constructing transaction" <<
-            e.what ();
-    }
-    catch (...)
-    {
-        JLOG (app.journal ("Ledger").warning) << "Exception constructing transaction";
-    }
-
-    return {};
-}
-
 //
 // Misc.
 //
--- a/src/ripple/net/impl/RPCCall.cpp
+++ b/src/ripple/net/impl/RPCCall.cpp
@@ -429,23 +429,27 @@ private:
        return jvRequest;
    }

-    // sign_for
+    // sign_for <account> <secret> <json> offline
+    // sign_for <account> <secret> <json>
    Json::Value parseSignFor (Json::Value const& jvParams)
    {
-        Json::Value     txJSON;
-        Json::Reader    reader;
+        bool const bOffline = 4 == jvParams.size () && jvParams[3u].asString () == "offline";

-        if ((4 == jvParams.size ())
-            && reader.parse (jvParams[3u].asString (), txJSON))
+        if (3 == jvParams.size () || bOffline)
        {
-            if (txJSON.type () == Json::objectValue)
+            Json::Value txJSON;
+            Json::Reader reader;
+            if (reader.parse (jvParams[2u].asString (), txJSON))
            {
-                // Return SigningFor object for the submitted transaction.
+                // sign_for txJSON.
                Json::Value jvRequest;
-                jvRequest["signing_for"] = jvParams[0u].asString ();
-                jvRequest["account"] = jvParams[1u].asString ();
-                jvRequest["secret"]  = jvParams[2u].asString ();
-                jvRequest["tx_json"] = txJSON;
+
+                jvRequest[jss::account] = jvParams[0u].asString ();
+                jvRequest[jss::secret]  = jvParams[1u].asString ();
+                jvRequest[jss::tx_json] = txJSON;
+
+                if (bOffline)
+                    jvRequest[jss::offline] = true;

                return jvRequest;
            }
@@ -929,7 +933,7 @@ public:
            {   "random",               &RPCParser::parseAsIs,                  0,  0   },
            {   "ripple_path_find",     &RPCParser::parseRipplePathFind,        1,  2   },
            {   "sign",                 &RPCParser::parseSignSubmit,            2,  3   },
-            {   "sign_for",             &RPCParser::parseSignFor,               4,  4   },
+            {   "sign_for",             &RPCParser::parseSignFor,               3,  4   },
            {   "submit",               &RPCParser::parseSignSubmit,            1,  3   },
            {   "submit_multisigned",   &RPCParser::parseSubmitMultiSigned,     1,  1   },
            {   "server_info",          &RPCParser::parseAsIs,                  0,  0   },
--- a/src/ripple/protocol/impl/Sign.cpp
+++ b/src/ripple/protocol/impl/Sign.cpp
@@ -51,6 +51,37 @@ verify (STObject const& st,
                true);
 }

+// Questions regarding buildMultiSigningData:
+//
+// Why do we include the Signer.Account in the blob to be signed?
+//
+// Unless you include the Account which is signing in the signing blob,
+// you could swap out any Signer.Account for any other, which may also
+// be on the SignerList and have a RegularKey matching the
+// Signer.SigningPubKey.
+//
+// That RegularKey may be set to allow some 3rd party to sign transactions
+// on the account's behalf, and that RegularKey could be common amongst all
+// users of the 3rd party. That's just one example of sharing the same
+// RegularKey amongst various accounts and just one vulnerability.
+//
+//   "When you have something that's easy to do that makes entire classes of
+//    attacks clearly and obviously impossible, you need a damn good reason
+//    not to do it."  --  David Schwartz
+//
+// Why would we include the signingFor account in the blob to be signed?
+//
+// In the current signing scheme, the account that a signer is `signing
+// for/on behalf of` is the tx_json.Account.
+//
+// Later we might support more levels of signing.  Suppose Bob is a signer
+// for Alice, and Carol is a signer for Bob, so Carol can sign for Bob who
+// signs for Alice.  But suppose Alice has two signers: Bob and Dave.  If
+// Carol is a signer for both Bob and Dave, then the signature needs to
+// distinguish between Carol signing for Bob and Carol signing for Dave.
+//
+// So, if we support multiple levels of signing, then we'll need to
+// incorporate the "signing for" accounts into the signing data as well.
 Serializer
 buildMultiSigningData (STObject const& obj, AccountID const& signingID)
 {
--- a/src/ripple/rpc/impl/TransactionSign.cpp
+++ b/src/ripple/rpc/impl/TransactionSign.cpp
@@ -22,6 +22,7 @@
 #include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/main/Application.h>
 #include <ripple/app/paths/Pathfinder.h>
+#include <ripple/app/tx/apply.h>              // Validity::Valid
 #include <ripple/basics/Log.h>
 #include <ripple/core/LoadFeeTrack.h>
 #include <ripple/json/json_writer.h>
@@ -386,7 +387,7 @@ transactionPreProcessImpl (
        Json::Value err = checkFee (
            params,
            role,
-            signingArgs.editFields(),
+            verify && signingArgs.editFields(),
            app.config(),
            app.getFeeTrack(),
            ledger);
@@ -401,7 +402,7 @@ transactionPreProcessImpl (
            role,
            app,
            ledger,
-            signingArgs.editFields());
+            verify && signingArgs.editFields());

        if (RPC::contains_error(err))
            return std::move (err);
@@ -427,6 +428,10 @@ transactionPreProcessImpl (
            tx_json[jss::Flags] = tfFullyCanonicalSig;
    }

+    // If multisigning then we need to return the public key.
+    if (signingArgs.isMultiSigning())
+        signingArgs.setPublicKey (keypair.publicKey);
+
    if (verify)
    {
        if (! sle)
@@ -437,12 +442,9 @@ transactionPreProcessImpl (
            << "verify: " << toBase58(calcAccountID(keypair.publicKey))
            << " : " << toBase58(srcAddressID);

-        // If multisigning then we need to return the public key.
-        if (signingArgs.isMultiSigning())
-        {
-            signingArgs.setPublicKey (keypair.publicKey);
-        }
-        else
+        // Don't do this test if multisigning since the account and secret
+        // probably don't belong together in that case.
+        if (!signingArgs.isMultiSigning())
        {
            // Make sure the account and secret belong together.
            error_code_i const err =
@@ -507,7 +509,7 @@ transactionPreProcessImpl (
 static
 std::pair <Json::Value, Transaction::pointer>
 transactionConstructImpl (std::shared_ptr<STTx const> const& stpTrans,
-    Rules const& rules, Application& app)
+    Rules const& rules, bool validateSig, Application& app, ApplyFlags flags)
 {
    std::pair <Json::Value, Transaction::pointer> ret;

@@ -533,17 +535,33 @@ transactionConstructImpl (std::shared_ptr<STTx const> const& stpTrans,
        {
            Serializer s;
            tpTrans->getSTransaction ()->add (s);
+            Blob transBlob = s.getData ();
+            SerialIter sit {makeSlice(transBlob)};

-            Transaction::pointer tpTransNew =
-                Transaction::sharedTransaction(s.getData(), rules, app);
-
-            if (tpTransNew && (
-                !tpTransNew->getSTransaction ()->isEquivalent (
-                    *tpTrans->getSTransaction ())))
+            // Check the signature if that's called for.
+            auto sttxNew = std::make_shared<STTx const> (sit);
+            if (validateSig &&
+                checkValidity(app.getHashRouter(),
+                *sttxNew, rules, app.config(), flags).first != Validity::Valid)
            {
-                tpTransNew.reset ();
+                ret.first = RPC::make_error (rpcINTERNAL,
+                    "Invalid signature.");
+                return ret;
+            }
+
+            std::string reason;
+            auto tpTransNew =
+                std::make_shared<Transaction> (sttxNew, reason, app);
+
+            if (tpTransNew)
+            {
+                if (!tpTransNew->getSTransaction()->isEquivalent (
+                        *tpTrans->getSTransaction()))
+                {
+                    tpTransNew.reset ();
+                }
+                tpTrans = std::move (tpTransNew);
            }
-            tpTrans = std::move (tpTransNew);
        }
    }
    catch (std::exception&)
@@ -656,7 +674,8 @@ Json::Value transactionSign (
    Role role,
    int validatedLedgerAge,
    Application& app,
-    std::shared_ptr<ReadView const> ledger)
+    std::shared_ptr<ReadView const> ledger,
+    ApplyFlags flags)
 {
    using namespace detail;

@@ -674,8 +693,8 @@ Json::Value transactionSign (

    // Make sure the STTx makes a legitimate Transaction.
    std::pair <Json::Value, Transaction::pointer> txn =
-        transactionConstructImpl (preprocResult.second,
-            ledger->rules(), app);
+        transactionConstructImpl (
+            preprocResult.second, ledger->rules(), true, app, flags);

    if (!txn.second)
        return txn.first;
@@ -691,7 +710,8 @@ Json::Value transactionSubmit (
    int validatedLedgerAge,
    Application& app,
    std::shared_ptr<ReadView const> ledger,
-    ProcessTransactionFn const& processTransaction)
+    ProcessTransactionFn const& processTransaction,
+    ApplyFlags flags)
 {
    using namespace detail;

@@ -709,8 +729,8 @@ Json::Value transactionSubmit (

    // Make sure the STTx makes a legitimate Transaction.
    std::pair <Json::Value, Transaction::pointer> txn =
-        transactionConstructImpl (preprocResult.second,
-            ledger->rules(), app);
+        transactionConstructImpl (
+            preprocResult.second, ledger->rules(), true, app, flags);

    if (!txn.second)
        return txn.first;
@@ -735,7 +755,7 @@ namespace detail
 {
 // There are a some field checks shared by transactionSignFor
 // and transactionSubmitMultiSigned.  Gather them together here.
-Json::Value checkMultiSignFields (Json::Value const& jvRequest)
+static Json::Value checkMultiSignFields (Json::Value const& jvRequest)
 {
   if (! jvRequest.isMember (jss::tx_json))
        return RPC::missing_field_error (jss::tx_json);
@@ -767,7 +787,8 @@ Json::Value transactionSignFor (
    Role role,
    int validatedLedgerAge,
    Application& app,
-    std::shared_ptr<ReadView const> ledger)
+    std::shared_ptr<ReadView const> ledger,
+    ApplyFlags flags)
 {
    auto j = app.journal ("RPCHandler");
    JLOG (j.debug) << "transactionSignFor: " << jvRequest;
@@ -828,8 +849,8 @@ Json::Value transactionSignFor (

    // Make sure the STTx makes a legitimate Transaction.
    std::pair <Json::Value, Transaction::pointer> txn =
-        transactionConstructImpl (preprocResult.second,
-            ledger->rules(), app);
+        transactionConstructImpl (
+            preprocResult.second, ledger->rules(), false, app, flags);

    if (!txn.second)
        return txn.first;
@@ -870,7 +891,8 @@ Json::Value transactionSubmitMultiSigned (
    int validatedLedgerAge,
    Application& app,
    std::shared_ptr<ReadView const> ledger,
-    ProcessTransactionFn const& processTransaction)
+    ProcessTransactionFn const& processTransaction,
+    ApplyFlags flags)
 {
    auto j = app.journal ("RPCHandler");
    JLOG (j.debug)
@@ -1056,8 +1078,7 @@ Json::Value transactionSubmitMultiSigned (

    // Make sure the SerializedTransaction makes a legitimate Transaction.
    std::pair <Json::Value, Transaction::pointer> txn =
-        transactionConstructImpl (stpTrans,
-            ledger->rules(), app);
+        transactionConstructImpl (stpTrans, ledger->rules(), true, app, flags);

    if (!txn.second)
        return txn.first;
--- a/src/ripple/rpc/impl/TransactionSign.h
+++ b/src/ripple/rpc/impl/TransactionSign.h
@@ -22,6 +22,7 @@

 #include <ripple/app/misc/NetworkOPs.h>
 #include <ripple/server/Role.h>
+#include <ripple/ledger/ApplyView.h>

 namespace ripple {

@@ -81,7 +82,8 @@ Json::Value transactionSign (
    Role role,
    int validatedLedgerAge,
    Application& app,
-    std::shared_ptr<ReadView const> ledger);
+    std::shared_ptr<ReadView const> ledger,
+    ApplyFlags flags = tapNONE);

 /** Returns a Json::objectValue. */
 Json::Value transactionSubmit (
@@ -91,7 +93,8 @@ Json::Value transactionSubmit (
    int validatedLedgerAge,
    Application& app,
    std::shared_ptr<ReadView const> ledger,
-    ProcessTransactionFn const& processTransaction);
+    ProcessTransactionFn const& processTransaction,
+    ApplyFlags flags = tapNONE);

 /** Returns a Json::objectValue. */
 Json::Value transactionSignFor (
@@ -100,7 +103,8 @@ Json::Value transactionSignFor (
    Role role,
    int validatedLedgerAge,
    Application& app,
-    std::shared_ptr<ReadView const> ledger);
+    std::shared_ptr<ReadView const> ledger,
+    ApplyFlags flags = tapNONE);

 /** Returns a Json::objectValue. */
 Json::Value transactionSubmitMultiSigned (
@@ -110,7 +114,8 @@ Json::Value transactionSubmitMultiSigned (
    int validatedLedgerAge,
    Application& app,
    std::shared_ptr<ReadView const> ledger,
-    ProcessTransactionFn const& processTransaction);
+    ProcessTransactionFn const& processTransaction,
+    ApplyFlags flags = tapNONE);

 } // RPC
 } // ripple
--- a/src/ripple/rpc/tests/JSONRPC.test.cpp
+++ b/src/ripple/rpc/tests/JSONRPC.test.cpp
@@ -557,6 +557,7 @@ R"({
    "secret": "masterpassphrase",
    "offline": 1,
    "tx_json": {
+        "Fee": 10,
        "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
        "Amount": "1000000000",
        "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
@@ -569,7 +570,7 @@ R"({
 "Missing field 'tx_json.Sequence'.",
 "Missing field 'tx_json.Sequence'."}},

-{ "Valid transaction if 'offline' is true.",
+{ "If 'offline' is true then a 'Fee' field must be supplied.",
 R"({
    "command": "doesnt_matter",
    "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
@@ -584,11 +585,54 @@ R"({
    }
 })",
 {
+"Missing field 'tx_json.Fee'.",
+"Missing field 'tx_json.Fee'.",
+"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.SigningPubKey'."}},
+
+{ "Valid transaction if 'offline' is true.",
+R"({
+    "command": "doesnt_matter",
+    "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+    "secret": "masterpassphrase",
+    "offline": 1,
+    "tx_json": {
+        "Sequence": 0,
+        "Fee": 10,
+        "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Amount": "1000000000",
+        "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "TransactionType": "Payment"
+    }
+})",
+{
 "",
 "",
 "Missing field 'tx_json.SigningPubKey'.",
 "Missing field 'tx_json.SigningPubKey'."}},

+{ "'offline' and 'build_path' are mutually exclusive.",
+R"({
+    "command": "doesnt_matter",
+    "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+    "secret": "masterpassphrase",
+    "offline": 1,
+    "build_path": 1,
+    "tx_json": {
+        "Sequence": 0,
+        "Fee": 10,
+        "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Amount": "1000000000",
+        "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "TransactionType": "Payment"
+    }
+})",
+{
+"Field 'build_path' not allowed in this context.",
+"Field 'build_path' not allowed in this context.",
+"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.SigningPubKey'."}},
+
 { "A 'Flags' field may be specified.",
 R"({
    "command": "doesnt_matter",
@@ -667,6 +711,28 @@ R"({
 "",
 "Missing field 'tx_json.Signers'."}},

+{ "Minimal offline sign_for.",
+R"({
+    "command": "doesnt_matter",
+    "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+    "secret": "masterpassphrase",
+    "offline": 1,
+    "tx_json": {
+        "Account": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "Amount": "1000000000",
+        "Destination": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Fee": 50,
+        "Sequence": 0,
+        "SigningPubKey": "",
+        "TransactionType": "Payment"
+    }
+})",
+{
+"",
+"",
+"",
+"Missing field 'tx_json.Signers'."}},
+
 { "Missing 'Account' in sign_for.",
 R"({
    "command": "doesnt_matter",
@@ -829,6 +895,34 @@ R"({
 "Missing field 'tx_json.TransactionType'."}},

 { "Minimal submit_multisigned.",
+R"({
+    "command": "submit_multisigned",
+    "tx_json": {
+        "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Amount": "1000000000",
+        "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "Fee": 50,
+        "Sequence": 0,
+        "Signers" : [
+             {
+                "Signer" : {
+                    "Account" : "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux",
+                    "SigningPubKey" : "02FE36A690D6973D55F88553F5D2C4202DE75F2CF8A6D0E17C70AC223F044501F8",
+                    "TxnSignature" : "3045022100909D01399AFFAD1E30D250CE61F93975B7F61E47B5244D78C3E86D9806535D95022012E389E0ACB016334052B7FE07FA6CEFDC8BE82CB410FA841D5049641C89DC8F"
+                }
+            }
+        ],
+        "SigningPubKey": "",
+        "TransactionType": "Payment"
+    }
+})",
+{
+"Missing field 'secret'.",
+"Missing field 'secret'.",
+"Missing field 'account'.",
+""}},
+
+{ "Minimal submit_multisigned with bad signature.",
 R"({
    "command": "submit_multisigned",
    "tx_json": {
@@ -854,7 +948,7 @@ R"({
 "Missing field 'secret'.",
 "Missing field 'secret'.",
 "Missing field 'account'.",
-""}},
+"Invalid signature."}},

 { "Missing tx_json in submit_multisigned.",
 R"({
@@ -1463,7 +1557,8 @@ public:
            Role role,
            int validatedLedgerAge,
            Application& app,
-            std::shared_ptr<ReadView const> ledger);
+            std::shared_ptr<ReadView const> ledger,
+            ApplyFlags flags);

        using submitFunc = Json::Value (*) (
            Json::Value params,
@@ -1472,7 +1567,8 @@ public:
            int validatedLedgerAge,
            Application& app,
            std::shared_ptr<ReadView const> ledger,
-            ProcessTransactionFn const& processTransaction);
+            ProcessTransactionFn const& processTransaction,
+            ApplyFlags flags);

        using TestStuff =
            std::tuple <signFunc, submitFunc, char const*, unsigned int>;
@@ -1512,7 +1608,8 @@ public:
                            testRole,
                            1,
                            env.app(),
-                            ledger);
+                            ledger,
+                            tapENABLE_TESTING);
                    }
                    else
                    {
@@ -1525,7 +1622,8 @@ public:
                            1,
                            env.app(),
                            ledger,
-                            processTxn);
+                            processTxn,
+                            tapENABLE_TESTING);
                    }

                    std::string errStr;