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.

Sign up for GitHub

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

[Amendment] Disallow setting an account's regular key to match its master key #2873

Closed
wants to merge 12 commits into from
Closed

[Amendment] Disallow setting an account's regular key to match its master key #2873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f6a79cc
WIP: debugging tests
thejohnfreeman Mar 5, 2019
c0f2e96
Finish tests for fix1721
thejohnfreeman Mar 5, 2019
0de8170
Add test for revoking regular key
thejohnfreeman Mar 5, 2019
bc3a6d5
Requested changes
thejohnfreeman Mar 8, 2019
5f104de
Change amendment name
thejohnfreeman Mar 8, 2019
44a339a
Always let the regular key sign
thejohnfreeman Mar 13, 2019
b7c57cd
Change error codes for signing key check
thejohnfreeman Mar 18, 2019
f28444b
Restore tefMASTER_DISABLED
thejohnfreeman Mar 22, 2019
5b00b39
Change tests to expect tefBAD_AUTH
thejohnfreeman Mar 22, 2019
28e2b52
Code review
thejohnfreeman Apr 4, 2019
12136c4
Code review
thejohnfreeman Apr 4, 2019
c55adb6
Split and comment conditional
thejohnfreeman Apr 15, 2019
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
9 changes: 9 additions & 0 deletions src/ripple/app/tx/impl/SetRegularKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <ripple/app/tx/impl/SetRegularKey.h>
#include <ripple/basics/Log.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/TxFlags.h>

namespace ripple {
Expand Down Expand Up @@ -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)
Expand Down
63 changes: 44 additions & 19 deletions src/ripple/app/tx/impl/Transactor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <ripple/protocol/Indexes.h>
#include <ripple/protocol/UintTypes.h>
#include <ripple/protocol/Protocol.h>
#include <ripple/protocol/STAccount.h>

namespace ripple {

Expand Down Expand Up @@ -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))
{

thejohnfreeman marked this conversation as resolved.
Show resolved Hide resolved
// 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;
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ class FeatureCollections
"fix1515",
"fix1578",
"MultiSignReserve",
"fixTakerDryOfferRemoval"
"fixTakerDryOfferRemoval",
"fixDisabledRegularKey"
};

std::vector<uint256> features;
Expand Down Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/TER.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ detail::supportedAmendments ()
"fix1578",
"MultiSignReserve",
"fixTakerDryOfferRemoval",
"fixDisabledRegularKey",
};
return supported;
}
Expand Down Expand Up @@ -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
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/TER.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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." } },
Expand Down
107 changes: 100 additions & 7 deletions src/test/app/SetRegularKey_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
thejohnfreeman marked this conversation as resolved.
Show resolved Hide resolved

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");
Expand All @@ -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");
Expand All @@ -95,8 +185,11 @@ class SetRegularKey_test : public beast::unit_test::suite
void run() override
{
testDisableMasterKey();
testDisableMasterKeyAfterFix();
testDisabledRegularKey();
testDisableRegularKeyAfterFix();
testPasswordSpent();
testUniversalMaskError();
testUniversalMask();
}

};
Expand Down
14 changes: 7 additions & 7 deletions src/test/jtx/Env_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ class Env_test : public beast::unit_test::suite
{
using namespace jtx;

Env env(*this);
Env env{*this, supported_amendments() | fixDisabledRegularKey};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because fixDisabledRegularKey is in supported_amendments(), is it necessary to explicitly add it in here?

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 want to be robust against changes in supported_amendments. To be honest, I'm surprised that the tests aren't already like this, because it meant I had to add - fixDisabledRegularKey to some existing tests. I prefer that new code not have to change existing tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amendments aren't removed from supported_amendments very often, so I hadn't really considered the need. I agree about new code not changing existing tests in general, but when you're changing functionality, sometimes you gotta do what you gotta do. 😄

Account const alice("alice", KeyType::ed25519);
Account const bob("bob", KeyType::secp256k1);
Account const carol("carol");
Expand All @@ -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));
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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"));
}

Expand Down