ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-23 15:20:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Cap untrusted manifests per message and drop oversized ones

Browse Source
This commit is contained in:
Pratik Mankawde
2026-08-03 17:00:11 +01:00
committed by Ayaz Salikhov
parent 4eece4003d
commit 8461ded0d8
11 changed files with 175 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/libxrpl/server/Manifest.cpp
View File

@@ -382,9 +382,9 @@ ManifestCache::revoked(PublicKey const& pk) const
}
ManifestDisposition
ManifestCache::applyManifest(Manifest m, ManifestRateLimitCap const cap)
ManifestCache::applyManifest(Manifest m, ManifestRateLimitCapPolicy const cap)
{
bool const uncapped = cap == ManifestRateLimitCap::Uncapped;
bool const uncapped = cap == ManifestRateLimitCapPolicy::Uncapped;
// The signature is checked only on the first `prewriteCheck` run (under the
// read lock). It is expensive, so `checkSignature` is cleared the first
@@ -634,7 +634,7 @@ ManifestCache::load(
JLOG(j_.warn()) << "Configured manifest revokes public key";
}
if (applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped) ==
if (applyManifest(std::move(*mo), ManifestRateLimitCapPolicy::Uncapped) ==
ManifestDisposition::Invalid)
{
JLOG(j_.error()) << "Manifest in config was rejected";
@@ -658,7 +658,7 @@ ManifestCache::load(
auto mo = deserializeManifest(base64Decode(revocationStr));
if (!mo || !mo->revoked() ||
applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped) ==
applyManifest(std::move(*mo), ManifestRateLimitCapPolicy::Uncapped) ==
ManifestDisposition::Invalid)
{
JLOG(j_.error()) << "Invalid validator key revocation in config";

2
src/libxrpl/server/Wallet.cpp
View File

@@ -80,7 +80,7 @@ getManifests(
// Only trusted manifests are persisted (see saveManifests), so
// anything loaded from the DB bypasses the untrusted cap.
cache.applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped);
cache.applyManifest(std::move(*mo), ManifestRateLimitCapPolicy::Uncapped);
}
else
{

38
src/test/app/Manifest_test.cpp
View File

@@ -400,7 +400,7 @@ public:
ManifestDisposition::Accepted ==
cache.applyManifest(
makeManifest(sk, KeyType::Ed25519, kp0.second, KeyType::Secp256k1, 0),
ManifestRateLimitCap::Capped));
ManifestRateLimitCapPolicy::Capped));
BEAST_EXPECT(cache.getSigningKey(pk) == kp0.first);
BEAST_EXPECT(cache.getMasterKey(kp0.first) == pk);
@@ -413,7 +413,7 @@ public:
ManifestDisposition::Accepted ==
cache.applyManifest(
makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 1),
ManifestRateLimitCap::Capped));
ManifestRateLimitCapPolicy::Capped));
BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first);
BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk);
BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first);
@@ -424,7 +424,7 @@ public:
ManifestDisposition::BadEphemeralKey ==
cache.applyManifest(
makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 2),
ManifestRateLimitCap::Capped));
ManifestRateLimitCapPolicy::Capped));
BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first);
BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk);
BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first);
@@ -435,7 +435,7 @@ public:
BEAST_EXPECT(
ManifestDisposition::Accepted ==
cache.applyManifest(
makeRevocation(sk, KeyType::Ed25519), ManifestRateLimitCap::Capped));
makeRevocation(sk, KeyType::Ed25519), ManifestRateLimitCapPolicy::Capped));
BEAST_EXPECT(cache.revoked(pk));
BEAST_EXPECT(cache.getSigningKey(pk) == pk);
BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first);
@@ -907,28 +907,28 @@ public:
// higher sequence numbers
auto const seq0 = cache.sequence();
BEAST_EXPECT(
cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
BEAST_EXPECT(cache.sequence() > seq0);
auto const seq1 = cache.sequence();
BEAST_EXPECT(
cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(cache.sequence() == seq1);
BEAST_EXPECT(
cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA1), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
BEAST_EXPECT(
cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA1), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(
cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(
cache.applyManifest(clone(sA2), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA2), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::BadEphemeralKey);
// applyManifest should accept manifests with max sequence numbers
@@ -936,38 +936,38 @@ public:
BEAST_EXPECT(!cache.revoked(pkA));
BEAST_EXPECT(sAMax.revoked());
BEAST_EXPECT(
cache.applyManifest(clone(sAMax), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sAMax), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
BEAST_EXPECT(
cache.applyManifest(clone(sAMax), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sAMax), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(
cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA1), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(
cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(cache.revoked(pkA));
// applyManifest should reject manifests with invalid signatures
BEAST_EXPECT(
cache.applyManifest(clone(sB0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sB0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
BEAST_EXPECT(
cache.applyManifest(clone(sB0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sB0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Stale);
BEAST_EXPECT(!deserializeManifest(fake));
BEAST_EXPECT(
cache.applyManifest(clone(sB1), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sB1), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Invalid);
BEAST_EXPECT(
cache.applyManifest(clone(sB2), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sB2), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
auto const sC0 = makeManifest(
kpB2.second, KeyType::Ed25519, randomSecretKey(), KeyType::Ed25519, 47);
BEAST_EXPECT(
cache.applyManifest(clone(sC0), ManifestRateLimitCap::Capped) ==
cache.applyManifest(clone(sC0), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::BadMasterKey);
}

16
src/test/app/ValidatorList_test.cpp
View File

@@ -280,7 +280,7 @@ private:
// NOLINTBEGIN(bugprone-unchecked-optional-access)
manifests.applyManifest(
*deserializeManifest(cfgManifest), ManifestRateLimitCap::Capped);
*deserializeManifest(cfgManifest), ManifestRateLimitCapPolicy::Capped);
// NOLINTEND(bugprone-unchecked-optional-access)
BEAST_EXPECT(
trustedKeys->load(localSigningPublicOuter, emptyCfgKeys, emptyCfgPublishers));
@@ -373,7 +373,7 @@ private:
// NOLINTBEGIN(bugprone-unchecked-optional-access)
manifests.applyManifest(
*deserializeManifest(cfgManifest), ManifestRateLimitCap::Capped);
*deserializeManifest(cfgManifest), ManifestRateLimitCapPolicy::Capped);
// NOLINTEND(bugprone-unchecked-optional-access)
BEAST_EXPECT(trustedKeys->load(localSigningPublicOuter, cfgKeys, emptyCfgPublishers));
@@ -467,7 +467,7 @@ private:
pubRevokedSigning.first,
pubRevokedSigning.second,
std::numeric_limits<std::uint32_t>::max())),
ManifestRateLimitCap::Capped);
ManifestRateLimitCapPolicy::Capped);
// NOLINTEND(bugprone-unchecked-optional-access)
// these two are not revoked (and not in the manifest cache at all.)
@@ -509,7 +509,7 @@ private:
pubRevokedSigning.first,
pubRevokedSigning.second,
std::numeric_limits<std::uint32_t>::max())),
ManifestRateLimitCap::Capped);
ManifestRateLimitCapPolicy::Capped);
// NOLINTEND(bugprone-unchecked-optional-access)
// this one is not revoked (and not in the manifest cache at all.)
@@ -1228,7 +1228,7 @@ private:
BEAST_EXPECT(
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
manifestsOuter.applyManifest(std::move(*m1), ManifestRateLimitCap::Capped) ==
manifestsOuter.applyManifest(std::move(*m1), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
BEAST_EXPECT(trustedKeysOuter->listed(masterPublic));
BEAST_EXPECT(trustedKeysOuter->trusted(masterPublic));
@@ -1243,7 +1243,7 @@ private:
masterPublic, masterPrivate, signingPublic2, signingKeys2.second, 2));
BEAST_EXPECT(
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
manifestsOuter.applyManifest(std::move(*m2), ManifestRateLimitCap::Capped) ==
manifestsOuter.applyManifest(std::move(*m2), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
BEAST_EXPECT(trustedKeysOuter->listed(masterPublic));
BEAST_EXPECT(trustedKeysOuter->trusted(masterPublic));
@@ -1261,7 +1261,7 @@ private:
// NOLINTBEGIN(bugprone-unchecked-optional-access)
BEAST_EXPECT(max->revoked());
BEAST_EXPECT(
manifestsOuter.applyManifest(std::move(*max), ManifestRateLimitCap::Capped) ==
manifestsOuter.applyManifest(std::move(*max), ManifestRateLimitCapPolicy::Capped) ==
ManifestDisposition::Accepted);
// NOLINTEND(bugprone-unchecked-optional-access)
@@ -2683,7 +2683,7 @@ private:
{
valManifests.applyManifest(
*deserializeManifest(base64Decode(self->manifest)),
ManifestRateLimitCap::Capped);
ManifestRateLimitCapPolicy::Capped);
BEAST_EXPECT(
result->load(self->signingPublic, emptyCfgKeys, cfgPublishers, threshold));
}

8
src/xrpld/app/misc/detail/ValidatorList.cpp
View File

@@ -1105,8 +1105,8 @@ ValidatorList::updatePublisherList(
continue;
}
if (auto const r =
validatorManifests_.applyManifest(std::move(*m), ManifestRateLimitCap::Uncapped);
if (auto const r = validatorManifests_.applyManifest(
std::move(*m), ManifestRateLimitCapPolicy::Uncapped);
r == ManifestDisposition::Invalid)
{
JLOG(j_.warn()) << "List for " << strHex(pubKey)
@@ -1362,8 +1362,8 @@ ValidatorList::verify(
// Publisher keys are configured/trusted (checked above), so bypass the
// untrusted cap.
auto const result =
publisherManifests_.applyManifest(std::move(manifest), ManifestRateLimitCap::Uncapped);
auto const result = publisherManifests_.applyManifest(
std::move(manifest), ManifestRateLimitCapPolicy::Uncapped);
if (revoked && result == ManifestDisposition::Accepted)
{

8
src/xrpld/overlay/Message.h
View File

@@ -4,6 +4,7 @@
#include <xrpl/basics/ByteUtilities.h>
#include <xrpl/protocol/PublicKey.h>
#include <xrpl/server/Manifest.h>
#include <google/protobuf/message.h>
@@ -23,6 +24,13 @@ constexpr std::size_t kMaximumMessageSize = megabytes(64);
// so we define a separate limit for them.
constexpr std::size_t kMaximumPingMessageSize = kilobytes(1);
// Upper bound on the wire size of a TMManifests message: kMaxManifestsPerMessage entries
// of at most kMaxManifestBytes each, plus a small allowance for protobuf
// framing per entry.
constexpr std::size_t kManifestFramingBytes = 8;
constexpr std::size_t kMaximumManifestsMessageSize =
kMaxManifestsPerMessage * (kMaxManifestBytes + kManifestFramingBytes);
// VFALCO NOTE If we forward declare Message and write out shared_ptr
// instead of using the in-class type alias, we can remove the
// entire ripple.pb.h from the main headers.

98
src/xrpld/overlay/detail/OverlayImpl.cpp
View File

@@ -44,6 +44,7 @@
#include <xrpl/protocol/Serializer.h>
#include <xrpl/protocol/SystemParameters.h>
#include <xrpl/protocol/jss.h>
#include <xrpl/resource/Fees.h>
#include <xrpl/resource/ResourceManager.h>
#include <xrpl/server/Handoff.h>
#include <xrpl/server/Manifest.h>
@@ -663,12 +664,17 @@ OverlayImpl::onManifests(
std::shared_ptr<protocol::TMManifests> const& m,
std::shared_ptr<PeerImp> const& from)
{
auto const n = m->list_size();
auto const& journal = from->pJournal();
// Process every trusted manifest, but stop processing untrusted ones once
// kMaxManifestsPerMessage of them have been handled, so the work stays bounded.
auto const total = static_cast<std::size_t>(m->list_size());
std::size_t untrusted = 0;
bool skippedUntrusted = false;
protocol::TMManifests relay;
for (std::size_t i = 0; i < n; ++i)
for (std::size_t i = 0; i < total; ++i)
{
auto& s = m->list().Get(i).stobject();
@@ -679,6 +685,19 @@ OverlayImpl::onManifests(
// lock: listed() takes the validator-list lock, so ordering it
// first avoids holding the two locks in opposite orders.
bool const isTrusted = app_.getValidators().listed(mo->masterKey);
// Bound untrusted work: process at most kMaxManifestsPerMessage
// untrusted manifests, but never skip a trusted one. Trusted
// manifests are not counted against the cap.
if (!isTrusted)
{
if (untrusted >= kMaxManifestsPerMessage)
{
skippedUntrusted = true;
continue;
}
++untrusted;
}
// Updates to a known key are relayed even when untrusted. Use
// getSequence, not getManifest, to avoid copying the cached payload
// on this hot path.
@@ -687,7 +706,8 @@ OverlayImpl::onManifests(
auto const result = app_.getValidatorManifests().applyManifest(
std::move(*mo),
isTrusted ? ManifestRateLimitCap::Uncapped : ManifestRateLimitCap::Capped);
isTrusted ? ManifestRateLimitCapPolicy::Uncapped
: ManifestRateLimitCapPolicy::Capped);
if (result == ManifestDisposition::Accepted)
{
@@ -726,6 +746,18 @@ OverlayImpl::onManifests(
}
}
if (skippedUntrusted)
{
// The sender exceeded the untrusted per-message cap. Charge it (once,
// here) so a flood of untrusted manifests is penalized, while an honest
// message of trusted manifests never is.
from->charge(Resource::kFeeMalformedRequest, "too many untrusted manifests");
JLOG(journal.warn()) << "Manifests: message had " << total
<< " entries; processed all trusted plus the first "
<< kMaxManifestsPerMessage << " untrusted";
}
if (!relay.list().empty())
{
forEach([m2 = std::make_shared<Message>(relay, protocol::mtMANIFESTS)](
@@ -1228,15 +1260,63 @@ OverlayImpl::getManifestsMessage()
if (auto seq = app_.getValidatorManifests().sequence(); seq != manifestListSeq_)
{
protocol::TMManifests tm;
// Phase 1: snapshot the cache under its own lock. Do not call
// Validators::listed() here — that takes the validator-list lock, and
// forEachManifest holds the manifest-cache lock, so consulting trust
// inside the callback would invert the lock order used elsewhere
// (see onManifests) and risk deadlock. Capture the manifest hash now,
// while we have the Manifest object, for the suppression key.
struct CachedManifest
{
PublicKey masterKey;
std::string serialized;
uint256 hash;
};
std::vector<CachedManifest> cached;
app_.getValidatorManifests().forEachManifest(
[&tm](std::size_t s) { tm.mutable_list()->Reserve(s); },
[&tm, &hr = app_.getHashRouter()](Manifest const& manifest) {
tm.add_list()->set_stobject(manifest.serialized.data(), manifest.serialized.size());
hr.addSuppression(manifest.hash());
[&cached](std::size_t s) { cached.reserve(s); },
[&cached](Manifest const& manifest) {
cached.push_back(
{.masterKey = manifest.masterKey,
.serialized = manifest.serialized,
.hash = manifest.hash()});
});
// Phase 2: no cache lock held, so trust checks are safe. Include every
// trusted manifest, then fill any remaining headroom up to
// kMaxManifestsPerMessage with untrusted gossip, so the whole message
// stays within the per-message cap the receiver enforces (trusted
// count is tiny in practice, so this effectively never drops trusted).
std::vector<CachedManifest const*> selected;
std::vector<CachedManifest const*> untrusted;
for (auto const& e : cached)
{
if (app_.getValidators().listed(e.masterKey))
{
selected.push_back(&e);
}
else
{
untrusted.push_back(&e);
}
}
// Cap untrusted only; trusted manifests are all included above.
auto const take = std::min(kMaxManifestsPerMessage, untrusted.size());
selected.insert(selected.end(), untrusted.begin(), untrusted.begin() + take);
// Shuffle the order. Cryptographic randomness is not needed here.
std::shuffle(selected.begin(), selected.end(), defaultPrng());
protocol::TMManifests tm;
auto& hr = app_.getHashRouter();
tm.mutable_list()->Reserve(static_cast<int>(selected.size()));
for (auto const* e : selected)
{
tm.add_list()->set_stobject(e->serialized.data(), e->serialized.size());
hr.addSuppression(e->hash);
}
manifestMessage_.reset();
if (tm.list_size() != 0)

4
src/xrpld/overlay/detail/PeerImp.cpp
View File

@@ -60,6 +60,7 @@
#include <xrpl/resource/Fees.h>
#include <xrpl/resource/Gossip.h>
#include <xrpl/server/LoadFeeTrack.h>
#include <xrpl/server/Manifest.h>
#include <xrpl/server/NetworkOPs.h>
#include <xrpl/shamap/SHAMapNodeID.h>
#include <xrpl/tx/apply.h>
@@ -1103,6 +1104,9 @@ PeerImp::onMessage(std::shared_ptr<protocol::TMManifests> const& m)
if (s > 100)
fee_.update(Resource::kFeeModerateBurdenPeer, "oversize");
// OverlayImpl::onManifests bounds the untrusted work and charges the fee
// if the untrusted count exceeds the per-message cap; trusted manifests
// are always processed and not counted against it.
app_.getJobQueue().addJob(JtManifest, "RcvManifests", [this, that = shared_from_this(), m]() {
overlay_.onManifests(m, that);
});

10
src/xrpld/overlay/detail/ProtocolMessage.h
View File

@@ -374,6 +374,16 @@ invokeProtocolMessage(Buffers const& buffers, Handler& handler, std::size_t& hin
return result;
}
// Drop an oversized TMManifests without penalty: consume the bytes and
// return no error, so the connection is preserved.
if (header->messageType == protocol::mtMANIFESTS &&
(header->payloadWireSize > kMaximumManifestsMessageSize ||
header->uncompressedSize > kMaximumManifestsMessageSize))
{
result.first = header->totalWireSize;
return result;
}
bool success = false;
switch (header->messageType)