diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index 0f1b242c8a4..12f4b647f4c 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 { @@ -55,6 +56,14 @@ SetRegularKey::preflight (PreflightContext const& ctx) if (!isTesSuccess (ret)) return ret; + if (ctx.rules.enabled(fixDisabledRegularKey) + && ctx.tx.isFieldPresent(sfRegularKey) + && (ctx.tx.getAccountID(sfRegularKey) == + ctx.tx.getAccountID(sfAccount))) + { + return temBAD_REGKEY; + } + std::uint32_t const uTxFlags = ctx.tx.getFlags (); if (uTxFlags & tfUniversalMask) diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index f8c90283d39..27e55efd713 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 (ctx.view.rules().enabled(fixDisabledRegularKey)) + { + + // 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 (pkAccount == id) + if (idSigner == idAccount) { - // Authorized to continue. - if (sle->isFlag(lsfDisableMaster)) + // 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 e4c07a8e3ca..32e04534340 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -83,7 +83,8 @@ class FeatureCollections "fix1515", "fix1578", "MultiSignReserve", - "fixTakerDryOfferRemoval" + "fixTakerDryOfferRemoval", + "fixDisabledRegularKey" }; 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 fixDisabledRegularKey; } // ripple diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index 911e5d509c5..4b9b902e69f 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 212896c7977..cbb74b2ddda 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -117,6 +117,7 @@ detail::supportedAmendments () "fix1578", "MultiSignReserve", "fixTakerDryOfferRemoval", + "fixDisabledRegularKey", }; 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 fixDisabledRegularKey = *getRegisteredFeature("fixDisabledRegularKey"); } // ripple diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index ad488ef8fe1..b79e7572641 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 e190a384d20..701c3001b2a 100644 --- a/src/test/app/SetRegularKey_test.cpp +++ b/src/test/app/SetRegularKey_test.cpp @@ -30,33 +30,121 @@ class SetRegularKey_test : public beast::unit_test::suite void testDisableMasterKey() { using namespace test::jtx; - Env env(*this); + + testcase("Set regular key"); + Env env {*this, supported_amendments() - fixDisabledRegularKey}; 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() | fixDisabledRegularKey}; + 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 fixDisabledRegularKey)"); + Env env {*this, supported_amendments() - fixDisabledRegularKey}; + Account const alice("alice"); + env.fund(XRP(10000), alice); + + // Must be possible unless amendment `fixDisabledRegularKey` 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(fixDisabledRegularKey); + 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 fixDisabledRegularKey)"); + Env env {*this, supported_amendments() | fixDisabledRegularKey}; + Account const alice("alice"); + env.fund(XRP(10000), alice); + + // Must be possible unless amendment `fixDisabledRegularKey` 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 @@ class SetRegularKey_test : public beast::unit_test::suite 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 @@ class SetRegularKey_test : public beast::unit_test::suite 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 fbab37e8a14..1c416f7dc89 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -250,7 +250,7 @@ class Env_test : public beast::unit_test::suite { using namespace jtx; - Env env(*this); + Env env{*this, supported_amendments() | fixDisabledRegularKey}; Account const alice("alice", KeyType::ed25519); Account const bob("bob", KeyType::secp256k1); Account const carol("carol"); @@ -259,12 +259,12 @@ class Env_test : public beast::unit_test::suite // 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 @@ class Env_test : public beast::unit_test::suite 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 @@ class Env_test : public beast::unit_test::suite 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")); }