diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index 0f1b242c8..ae46397b5 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.cpp +++ b/src/ripple/app/tx/impl/SetRegularKey.cpp @@ -19,6 +19,7 @@ #include #include +#include #include namespace ripple { @@ -65,6 +66,14 @@ SetRegularKey::preflight (PreflightContext const& ctx) return temINVALID_FLAG; } + + if (ctx.rules.enabled(fixMasterKeyAsRegularKey) + && ctx.tx.isFieldPresent(sfRegularKey) + && (ctx.tx.getAccountID(sfRegularKey) == ctx.tx.getAccountID(sfAccount))) + { + return temBAD_REGKEY; + } + return preflight2(ctx); } @@ -84,9 +93,9 @@ SetRegularKey::doApply () } else { + // Account has disabled master key and no multi-signer signer list. if (sle->isFlag (lsfDisableMaster) && !view().peek (keylet::signers (account_))) - // Account has disabled master key and no multi-signer signer list. return tecNO_ALTERNATIVE_KEY; sle->makeFieldAbsent (sfRegularKey); diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index f8c90283d..5c7fd6781 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -31,6 +31,7 @@ #include #include #include +#include namespace ripple { @@ -340,44 +341,68 @@ Transactor::checkSign (PreclaimContext const& ctx) NotTEC Transactor::checkSingleSign (PreclaimContext const& ctx) { - auto const id = ctx.tx.getAccountID(sfAccount); - - auto const sle = ctx.view.read( - keylet::account(id)); - auto const hasAuthKey = sle->isFieldPresent (sfRegularKey); - - // Consistency: Check signature & verify the transaction's signing - // public key is authorized for signing. - auto const spk = ctx.tx.getSigningPubKey(); - if (!publicKeyType (makeSlice (spk))) + // Check that the value in the signing key slot is a public key. + auto const pkSigner = ctx.tx.getSigningPubKey(); + if (!publicKeyType(makeSlice(pkSigner))) { JLOG(ctx.j.trace()) << "checkSingleSign: signing public key type is unknown"; return tefBAD_AUTH; // FIXME: should be better error! } - auto const pkAccount = calcAccountID ( - PublicKey (makeSlice (spk))); + // Look up the account. + auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner))); + auto const idAccount = ctx.tx.getAccountID(sfAccount); + auto const sleAccount = ctx.view.read(keylet::account(idAccount)); + bool const isMasterDisabled = sleAccount->isFlag(lsfDisableMaster); - if (pkAccount == id) + if (ctx.view.rules().enabled(fixMasterKeyAsRegularKey)) { - // Authorized to continue. - if (sle->isFlag(lsfDisableMaster)) + + // Signed with regular key. + if ((*sleAccount)[~sfRegularKey] == idSigner) + { + return tesSUCCESS; + } + + // Signed with enabled mater key. + if (!isMasterDisabled && idAccount == idSigner) + { + return tesSUCCESS; + } + + // Signed with disabled master key. + if (isMasterDisabled && idAccount == idSigner) + { + return tefMASTER_DISABLED; + } + + // Signed with any other key. + return tefBAD_AUTH; + + } + + if (idSigner == idAccount) + { + // Signing with the master key. Continue if it is not disabled. + if (isMasterDisabled) return tefMASTER_DISABLED; } - else if (hasAuthKey && - (pkAccount == sle->getAccountID (sfRegularKey))) + else if ((*sleAccount)[~sfRegularKey] == idSigner) { - // Authorized to continue. + // Signing with the regular key. Continue. } - else if (hasAuthKey) + else if (sleAccount->isFieldPresent(sfRegularKey)) { + // Signing key does not match master or regular key. JLOG(ctx.j.trace()) << "checkSingleSign: Not authorized to use account."; return tefBAD_AUTH; } else { + // No regular key on account and signing key does not match master key. + // FIXME: Why differentiate this case from tefBAD_AUTH? JLOG(ctx.j.trace()) << "checkSingleSign: Not authorized to use account."; return tefBAD_AUTH_MASTER; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index e4c07a8e3..47825a14a 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -83,7 +83,8 @@ class FeatureCollections "fix1515", "fix1578", "MultiSignReserve", - "fixTakerDryOfferRemoval" + "fixTakerDryOfferRemoval", + "fixMasterKeyAsRegularKey" }; std::vector features; @@ -372,6 +373,7 @@ extern uint256 const fix1515; extern uint256 const fix1578; extern uint256 const featureMultiSignReserve; extern uint256 const fixTakerDryOfferRemoval; +extern uint256 const fixMasterKeyAsRegularKey; } // ripple diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 911e5d509..4b9b902e6 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -87,6 +87,7 @@ enum TEMcodes : TERUnderlyingType temBAD_OFFER, temBAD_PATH, temBAD_PATH_LOOP, + temBAD_REGKEY, temBAD_SEND_XRP_LIMIT, temBAD_SEND_XRP_MAX, temBAD_SEND_XRP_NO_DIRECT, diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 212896c79..1645a01c4 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -117,6 +117,7 @@ detail::supportedAmendments () "fix1578", "MultiSignReserve", "fixTakerDryOfferRemoval", + "fixMasterKeyAsRegularKey", }; return supported; } @@ -173,5 +174,6 @@ uint256 const fix1515 = *getRegisteredFeature("fix1515"); uint256 const fix1578 = *getRegisteredFeature("fix1578"); uint256 const featureMultiSignReserve = *getRegisteredFeature("MultiSignReserve"); uint256 const fixTakerDryOfferRemoval = *getRegisteredFeature("fixTakerDryOfferRemoval"); +uint256 const fixMasterKeyAsRegularKey = *getRegisteredFeature("fixMasterKeyAsRegularKey"); } // ripple diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index ad488ef8f..b79e75726 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -119,6 +119,7 @@ transResults() { temBAD_PATH, { "temBAD_PATH", "Malformed: Bad path." } }, { temBAD_PATH_LOOP, { "temBAD_PATH_LOOP", "Malformed: Loop in path." } }, { temBAD_QUORUM, { "temBAD_QUORUM", "Malformed: Quorum is unreachable." } }, + { temBAD_REGKEY, { "temBAD_REGKEY", "Malformed: Regular key cannot be same as master key." } }, { temBAD_SEND_XRP_LIMIT, { "temBAD_SEND_XRP_LIMIT", "Malformed: Limit quality is not allowed for XRP to XRP." } }, { temBAD_SEND_XRP_MAX, { "temBAD_SEND_XRP_MAX", "Malformed: Send max is not allowed for XRP to XRP." } }, { temBAD_SEND_XRP_NO_DIRECT, { "temBAD_SEND_XRP_NO_DIRECT","Malformed: No Ripple direct is not allowed for XRP to XRP." } }, diff --git a/src/test/app/SetRegularKey_test.cpp b/src/test/app/SetRegularKey_test.cpp index fdbb9af55..6367fddbc 100644 --- a/src/test/app/SetRegularKey_test.cpp +++ b/src/test/app/SetRegularKey_test.cpp @@ -30,33 +30,121 @@ public: void testDisableMasterKey() { using namespace test::jtx; - Env env(*this); + + testcase("Set regular key"); + Env env {*this, supported_amendments() - fixMasterKeyAsRegularKey}; Account const alice("alice"); Account const bob("bob"); env.fund(XRP(10000), alice, bob); - // Master and Regular key env(regkey(alice, bob)); auto const ar = env.le(alice); BEAST_EXPECT(ar->isFieldPresent(sfRegularKey) && (ar->getAccountID(sfRegularKey) == bob.id())); - env(noop(alice)); env(noop(alice), sig(bob)); env(noop(alice), sig(alice)); - // Regular key only + testcase("Disable master key"); env(fset(alice, asfDisableMaster), sig(alice)); - env(noop(alice)); env(noop(alice), sig(bob)); env(noop(alice), sig(alice), ter(tefMASTER_DISABLED)); + + testcase("Re-enable master key"); env(fclear(alice, asfDisableMaster), sig(alice), ter(tefMASTER_DISABLED)); + env(fclear(alice, asfDisableMaster), sig(bob)); + env(noop(alice), sig(bob)); env(noop(alice), sig(alice)); + + testcase("Revoke regular key"); + env(regkey(alice, disabled)); + env(noop(alice), sig(bob), ter(tefBAD_AUTH_MASTER)); + env(noop(alice), sig(alice)); + } + + void testDisableMasterKeyAfterFix() + { + using namespace test::jtx; + + testcase("Set regular key"); + Env env{*this, supported_amendments() | fixMasterKeyAsRegularKey}; + Account const alice("alice"); + Account const bob("bob"); + env.fund(XRP(10000), alice, bob); + + env(regkey(alice, bob)); + env(noop(alice), sig(bob)); + env(noop(alice), sig(alice)); + + testcase("Disable master key"); + env(fset(alice, asfDisableMaster), sig(alice)); + env(noop(alice), sig(bob)); + env(noop(alice), sig(alice), ter(tefMASTER_DISABLED)); + + testcase("Re-enable master key"); + env( + fclear(alice, asfDisableMaster), + sig(alice), + ter(tefMASTER_DISABLED) + ); + + env(fclear(alice, asfDisableMaster), sig(bob)); + env(noop(alice), sig(bob)); + env(noop(alice), sig(alice)); + + testcase("Revoke regular key"); + env(regkey(alice, disabled)); + env(noop(alice), sig(bob), ter(tefBAD_AUTH)); + env(noop(alice), sig(alice)); + } + + void testDisabledRegularKey() + { + using namespace test::jtx; + + // See https://ripplelabs.atlassian.net/browse/RIPD-1721. + testcase("Set regular key to master key (before fixMasterKeyAsRegularKey)"); + Env env {*this, supported_amendments() - fixMasterKeyAsRegularKey}; + Account const alice("alice"); + env.fund(XRP(10000), alice); + + // Must be possible unless amendment `fixMasterKeyAsRegularKey` enabled. + env(regkey(alice, alice), sig(alice)); + env(fset(alice, asfDisableMaster), sig(alice)); + + // No way to sign... + env(noop(alice), ter(tefMASTER_DISABLED)); + env(noop(alice), sig(alice), ter(tefMASTER_DISABLED)); + + // ... until now. + env.enableFeature(fixMasterKeyAsRegularKey); + env(noop(alice)); + env(noop(alice), sig(alice)); + + env(regkey(alice, disabled), ter(tecNO_ALTERNATIVE_KEY)); + env(fclear(alice, asfDisableMaster)); + env(regkey(alice, disabled)); + env(fset(alice, asfDisableMaster), ter(tecNO_ALTERNATIVE_KEY)); + } + + void testDisableRegularKeyAfterFix() + { + using namespace test::jtx; + + testcase("Set regular key to master key (after fixMasterKeyAsRegularKey)"); + Env env {*this, supported_amendments() | fixMasterKeyAsRegularKey}; + Account const alice("alice"); + env.fund(XRP(10000), alice); + + // Must be possible unless amendment `fixMasterKeyAsRegularKey` enabled. + env(regkey(alice, alice), ter(temBAD_REGKEY)); } void testPasswordSpent() { using namespace test::jtx; + + testcase("Password spent"); Env env(*this); Account const alice("alice"); Account const bob("bob"); @@ -79,9 +167,11 @@ public: BEAST_EXPECT(ar->isFieldPresent(sfFlags) && ((ar->getFieldU32(sfFlags) & lsfPasswordSpent) == 0)); } - void testUniversalMaskError() + void testUniversalMask() { using namespace test::jtx; + + testcase("Universal mask"); Env env(*this); Account const alice("alice"); Account const bob("bob"); @@ -95,8 +185,11 @@ public: void run() override { testDisableMasterKey(); + testDisableMasterKeyAfterFix(); + testDisabledRegularKey(); + testDisableRegularKeyAfterFix(); testPasswordSpent(); - testUniversalMaskError(); + testUniversalMask(); } }; diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index 6a1eb2697..edde0c4dc 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -250,7 +250,7 @@ public: { using namespace jtx; - Env env(*this); + Env env{*this, supported_amendments() | fixMasterKeyAsRegularKey}; Account const alice("alice", KeyType::ed25519); Account const bob("bob", KeyType::secp256k1); Account const carol("carol"); @@ -259,12 +259,12 @@ public: // Master key only env(noop(alice)); env(noop(bob)); - env(noop(alice), sig("alice"), ter(tefBAD_AUTH_MASTER)); + env(noop(alice), sig("alice"), ter(tefBAD_AUTH)); env(noop(alice), sig(Account("alice", - KeyType::secp256k1)), ter(tefBAD_AUTH_MASTER)); + KeyType::secp256k1)), ter(tefBAD_AUTH)); env(noop(bob), sig(Account("bob", - KeyType::ed25519)), ter(tefBAD_AUTH_MASTER)); - env(noop(alice), sig(carol), ter(tefBAD_AUTH_MASTER)); + KeyType::ed25519)), ter(tefBAD_AUTH)); + env(noop(alice), sig(carol), ter(tefBAD_AUTH)); // Master and Regular key env(regkey(alice, bob)); @@ -302,7 +302,7 @@ public: env(pay(env.master, "alice", XRP(1000)), seq(none), ter(temMALFORMED)); env(pay(env.master, "alice", XRP(1000)), seq(20), ter(terPRE_SEQ)); env(pay(env.master, "alice", XRP(1000)), sig(none), ter(temMALFORMED)); - env(pay(env.master, "alice", XRP(1000)), sig("bob"), ter(tefBAD_AUTH_MASTER)); + env(pay(env.master, "alice", XRP(1000)), sig("bob"), ter(tefBAD_AUTH)); env(pay(env.master, "dilbert", XRP(1000)), sig(env.master)); @@ -344,7 +344,7 @@ public: env(fclear("alice", asfDisableMaster)); env.require(nflags("alice", asfDisableMaster)); env(regkey("alice", disabled)); - env(noop("alice"), sig("eric"), ter(tefBAD_AUTH_MASTER)); + env(noop("alice"), sig("eric"), ter(tefBAD_AUTH)); env(noop("alice")); }