Skip to content

Commit

Permalink
Enforce account RPC limits by account objects traversed
Browse files Browse the repository at this point in the history
  • Loading branch information
@natenichols
natenichols committed Dec 15, 2021
1 parent 72d75c3 commit d03b771
Show file tree
Hide file tree
Showing 6 changed files with 127 additions and 61 deletions.
29 changes: 15 additions & 14 deletions src/ripple/rpc/handlers/AccountChannels.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,20 +124,20 @@ doAccountChannels(RPC::JsonContext& context)

// Marker is composed of a comma separated index and start hint. The
// former will be read as hex, and the latter using boost lexical cast.
std::stringstream ss(params[jss::marker].asString());
std::string s;
if (!std::getline(ss, s, ','))
std::stringstream marker(params[jss::marker].asString());
std::string value;
if (!std::getline(marker, value, ','))
return rpcError(rpcINVALID_PARAMS);

if (!startAfter.parseHex(s))
if (!startAfter.parseHex(value))
return rpcError(rpcINVALID_PARAMS);

if (!std::getline(ss, s, ','))
if (!std::getline(marker, value, ','))
return rpcError(rpcINVALID_PARAMS);

try
{
startHint = boost::lexical_cast<std::uint64_t>(s);
startHint = boost::lexical_cast<std::uint64_t>(value);
}
catch (boost::bad_lexical_cast&)
{
Expand All @@ -151,13 +151,11 @@ doAccountChannels(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID);

if (!isOwned)
if (!RPC::isOwnedByAccount(*ledger, sle, accountID))
return rpcError(rpcINVALID_PARAMS);
}

auto ct = 0;
auto count = 0;
std::optional<uint256> marker = {};
std::uint64_t nextHint = 0;
if (!forEachItemAfter(
Expand All @@ -166,21 +164,21 @@ doAccountChannels(RPC::JsonContext& context)
startAfter,
startHint,
limit + 1,
[&visitData, &accountID, &ct, &limit, &marker, &nextHint](
[&visitData, &accountID, &count, &limit, &marker, &nextHint](
std::shared_ptr<SLE const> const& sleCur) {
if (!sleCur)
{
assert(false);
return false;
}

if (++ct == limit)
if (++count == limit)
{
marker = sleCur->key();
nextHint = RPC::getStartHint(sleCur, visitData.accountID);
}

if (ct <= limit && sleCur->getType() == ltPAYCHAN &&
if (count <= limit && sleCur->getType() == ltPAYCHAN &&
(*sleCur)[sfAccount] == accountID &&
(!visitData.hasDst ||
visitData.raDstAccount == (*sleCur)[sfDestination]))
Expand All @@ -194,7 +192,10 @@ doAccountChannels(RPC::JsonContext& context)
return rpcError(rpcINVALID_PARAMS);
}

if (ct == limit + 1 && marker)
// Both conditions need to be checked because marker is set on the limit-th
// item, but if there is no item on the limit + 1 iteration, then there is
// no need to return a marker.
if (count == limit + 1 && marker)
{
result[jss::limit] = limit;
result[jss::marker] =
Expand Down
43 changes: 24 additions & 19 deletions src/ripple/rpc/handlers/AccountLines.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,20 +151,20 @@ doAccountLines(RPC::JsonContext& context)

// Marker is composed of a comma separated index and start hint. The
// former will be read as hex, and the latter using boost lexical cast.
std::stringstream ss(params[jss::marker].asString());
std::string s;
if (!std::getline(ss, s, ','))
std::stringstream marker(params[jss::marker].asString());
std::string value;
if (!std::getline(marker, value, ','))
return rpcError(rpcINVALID_PARAMS);

if (!startAfter.parseHex(s))
if (!startAfter.parseHex(value))
return rpcError(rpcINVALID_PARAMS);

if (!std::getline(ss, s, ','))
if (!std::getline(marker, value, ','))
return rpcError(rpcINVALID_PARAMS);

try
{
startHint = boost::lexical_cast<std::uint64_t>(s);
startHint = boost::lexical_cast<std::uint64_t>(value);
}
catch (boost::bad_lexical_cast&)
{
Expand All @@ -178,13 +178,11 @@ doAccountLines(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID);

if (!isOwned)
if (!RPC::isOwnedByAccount(*ledger, sle, accountID))
return rpcError(rpcINVALID_PARAMS);
}

auto ct = 0;
auto count = 0;
std::optional<uint256> marker = {};
std::uint64_t nextHint = 0;
{
Expand All @@ -194,7 +192,7 @@ doAccountLines(RPC::JsonContext& context)
startAfter,
startHint,
limit + 1,
[&visitData, &ct, &marker, &limit, &nextHint](std::shared_ptr<SLE const> const& sleCur) {
[&visitData, &count, &marker, &limit, &nextHint](std::shared_ptr<SLE const> const& sleCur) {
bool ignore = false;
if (visitData.ignoreDefault)
{
Expand All @@ -213,21 +211,25 @@ doAccountLines(RPC::JsonContext& context)
return false;
}

if (++ct == limit)
if (++count == limit)
{
marker = sleCur->key();
nextHint =
RPC::getStartHint(sleCur, visitData.accountID);
}

auto const line =
RippleState::makeItem(visitData.accountID, sleCur);
if (ct <= limit && line != nullptr &&
(!visitData.hasPeer ||
visitData.raPeerAccount == line->getAccountIDPeer()))
if (!ignore && count <= limit)
{
if (!ignore)
auto const line =
RippleState::makeItem(visitData.accountID, sleCur);

if (line != nullptr &&
(!visitData.hasPeer ||
visitData.raPeerAccount ==
line->getAccountIDPeer()))
{
visitData.items.emplace_back(line);
}
}

return true;
Expand All @@ -237,7 +239,10 @@ doAccountLines(RPC::JsonContext& context)
}
}

if (ct == limit + 1 && marker)
// Both conditions need to be checked because marker is set on the limit-th
// item, but if there is no item on the limit + 1 iteration, then there is
// no need to return a marker.
if (count == limit + 1 && marker)
{
result[jss::limit] = limit;
result[jss::marker] =
Expand Down
29 changes: 15 additions & 14 deletions src/ripple/rpc/handlers/AccountOffers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,20 +101,20 @@ doAccountOffers(RPC::JsonContext& context)

// Marker is composed of a comma separated index and start hint. The
// former will be read as hex, and the latter using boost lexical cast.
std::stringstream ss(params[jss::marker].asString());
std::string s;
if (!std::getline(ss, s, ','))
std::stringstream marker(params[jss::marker].asString());
std::string value;
if (!std::getline(marker, value, ','))
return rpcError(rpcINVALID_PARAMS);

if (!startAfter.parseHex(s))
if (!startAfter.parseHex(value))
return rpcError(rpcINVALID_PARAMS);

if (!std::getline(ss, s, ','))
if (!std::getline(marker, value, ','))
return rpcError(rpcINVALID_PARAMS);

try
{
startHint = boost::lexical_cast<std::uint64_t>(s);
startHint = boost::lexical_cast<std::uint64_t>(value);
}
catch (boost::bad_lexical_cast&)
{
Expand All @@ -128,13 +128,11 @@ doAccountOffers(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID);

if (!isOwned)
if (!RPC::isOwnedByAccount(*ledger, sle, accountID))
return rpcError(rpcINVALID_PARAMS);
}

auto ct = 0;
auto count = 0;
std::optional<uint256> marker = {};
std::uint64_t nextHint = 0;
if (!forEachItemAfter(
Expand All @@ -143,21 +141,21 @@ doAccountOffers(RPC::JsonContext& context)
startAfter,
startHint,
limit + 1,
[&offers, &ct, &marker, &limit, &nextHint, &accountID](
[&offers, &count, &marker, &limit, &nextHint, &accountID](
std::shared_ptr<SLE const> const& sle) {
if (!sle)
{
assert(false);
return false;
}

if (++ct == limit)
if (++count == limit)
{
marker = sle->key();
nextHint = RPC::getStartHint(sle, accountID);
}

if (ct <= limit && sle->getType() == ltOFFER)
if (count <= limit && sle->getType() == ltOFFER)
{
offers.emplace_back(sle);
}
Expand All @@ -168,7 +166,10 @@ doAccountOffers(RPC::JsonContext& context)
return rpcError(rpcINVALID_PARAMS);
}

if (ct == limit + 1 && marker)
// Both conditions need to be checked because marker is set on the limit-th
// item, but if there is no item on the limit + 1 iteration, then there is
// no need to return a marker.
if (count == limit + 1 && marker)
{
result[jss::limit] = limit;
result[jss::marker] =
Expand Down
18 changes: 5 additions & 13 deletions src/ripple/rpc/impl/RPCHelpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ getStartHint(std::shared_ptr<SLE const> const& sle, AccountID const& accountID)
return sle->getFieldU64(sfHighNode);
}

if (!sle->isFieldPresent(sfOwnerNode))
return 0;

return sle->getFieldU64(sfOwnerNode);
}

Expand All @@ -121,19 +124,8 @@ isOwnedByAccount(
}
else if (sle->getType() == ltSIGNER_LIST)
{
auto hint = sle->getFieldU64(sfOwnerNode);
auto const rootIndex = keylet::ownerDir(accountID);
auto const pageIndex = keylet::page(rootIndex, hint);

auto directory = ledger.read({ltDIR_NODE, pageIndex.key});

if (!directory)
return false;

auto const& hashes = directory->getFieldV256(sfIndexes);
auto iter = std::find(hashes.begin(), hashes.end(), sle->key());

return iter != hashes.end();
Keylet const accountSignerList = keylet::signers(accountID);
return sle->key() == accountSignerList.key;
}

return false;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/impl/RPCHelpers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ getStartHint(std::shared_ptr<SLE const> const& sle, AccountID const& accountID);
* Tests if a SLE is owned by accountID.
* @param ledger - The ledger used to search for the sle.
* @param sle - The SLE to test for ownership.
* @param account - The account that supposedly owns the SLE.
* @param account - The account being tested for SLE ownership.
*/
bool
isOwnedByAccount(
Expand Down
67 changes: 67 additions & 0 deletions src/test/rpc/AccountLinesRPC_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,72 @@ class AccountLinesRPC_test : public beast::unit_test::suite
}
}

void
testAccountLinesMarker()
{
testcase("Entry pointed to by marker is not owned by account");
using namespace test::jtx;
Env env(*this);

// The goal of this test is observe account_lines RPC calls return an
// error message when the SLE pointed to by the marker is not owned by
// the Account being traversed.
//
// To start, we'll create an environment with some trust lines, and a
// signers list.
Account const alice{"alice"};
Account const becky{"becky"};
Account const cheri{"cheri"};
Account const gw1{"gw1"};
Account const gw2{"gw2"};
env.fund(XRP(10000), alice, becky, cheri, gw1, gw2);
env.close();

auto const USD = gw1["USD"];
auto const AUD = gw1["AUD"];
auto const EUR = gw2["EUR"];
env(trust(alice, USD(200)));
env(trust(alice, AUD(200)));
env(trust(becky, EUR(200)));
env(trust(cheri, EUR(200)));
env.close();

// Give alice a SignerList.
Account const bogie{"bogie"};

Json::Value const aliceSigners = signers(alice, 2, {{bogie, 3}});
env(aliceSigners);

// Get account_lines for alice. Limit at 1, so we get a marker pointing
// to a SignerList.
auto const aliceLines = env.rpc(
"json",
"account_lines",
R"({"account": ")" + alice.human() + R"(", "limit": 1})");
BEAST_EXPECT(aliceLines[jss::result].isMember(jss::marker));

// When we fetch Alice's remaining lines, we should see only one more.
auto const aliceMarker = env.rpc(
"json",
"account_lines",
R"({"account": ")" + alice.human() +
R"(", )"
R"("marker": ")" +
aliceLines[jss::result][jss::marker].asString() + R"("})");
BEAST_EXPECT(aliceMarker[jss::result][jss::lines].size() == 1);

// Get account lines for beckys account, using alices SignerList as a
// marker. This should cause an error.
auto const beckyLines = env.rpc(
"json",
"account_lines",
R"({"account": ")" + becky.human() +
R"(", )"
R"("marker": ")" +
aliceLines[jss::result][jss::marker].asString() + R"("})");
BEAST_EXPECT(beckyLines[jss::result].isMember(jss::error_message));
}

void
testAccountLineDelete()
{
Expand Down Expand Up @@ -1147,6 +1213,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite
run() override
{
testAccountLines();
testAccountLinesMarker();
testAccountLineDelete();
testAccountLines2();
testAccountLineDelete2();
Expand Down

0 comments on commit d03b771

Please sign in to comment.