ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-19 05:10:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Reject an inner node claimed at leaf depth in verifyProofPath

Browse Source 
A SHAMap has 65 levels, and nibbles run out at level 64: `selectBranch` indexes
the key byte at `depth / 2`, so at depth 64 it reads byte 32 of a 32-byte key.
Only the leaf terminating a path may sit at that depth, but proof path nodes
come off the wire, so a peer could send 65 hash-chained inner nodes and drive
that read past the end of the buffer.

The existing length bound cannot be tightened to catch this: a path for two
keys sharing all 63 leading nibbles legitimately holds 64 inner nodes plus a
leaf, so 65 elements is valid. The claimed node type at the final depth is the
thing to reject, using `>=` rather than `==` to match the convention every
other `kLeafDepth` comparison in this codebase already follows.

Reachable from `TMProofPathResponse` via `LedgerReplayMsgHandler`; confirmed
under ASan with asserts compiled out that the unguarded read lands one byte
past a 32-byte heap allocation. Also closes a second gap: nothing checked the
terminal leaf's own key against `key`, so the hash chain alone let a peer
substitute any leaf whose subtree hashes matched at every level above it.
Pinned by tests: the 65-element path that must verify for both keys sharing
the deep prefix, and the forged all-inner path that must not.

Also guards `visitDifferences` against an inner node claimed at leaf depth:
`hasLeafNode` only checked the comparison map, not the map being walked, so a
corrupt node in the map under `visitDifferences` itself could still throw
uncaught. Skip such a node's children instead.
This commit is contained in:
Bart
2026-08-01 21:35:08 -04:00
parent ca39bff3c8
commit d854982fd7
2 changed files with 103 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
src/libxrpl/shamap/SHAMapSync.cpp
View File

@@ -143,6 +143,17 @@ SHAMap::visitDifferences(
if (!function(*node))
return;
// Nibbles run out at kLeafDepth, so only a leaf belongs there. An inner node at that depth
// means the store holds one, possibly seeded by a peer via an earlier fetch-pack exchange;
// skip its children rather than descending into it.
if (nodeID.getDepth() >= kLeafDepth)
{
// LCOV_EXCL_START
UNREACHABLE("xrpl::SHAMap::visitDifferences : inner node at leaf depth");
continue;
// LCOV_EXCL_STOP
}
// 2) push non-matching child inner nodes
for (auto i = 0u; i < kBranchFactor; ++i)
{
@@ -749,11 +760,9 @@ SHAMap::hasLeafNode(uint256 const& tag, SHAMapHash const& targetNodeHash) const
do
{
// An inner node is only reachable here at a depth below kLeafDepth in a well-formed map,
// where the loop always finds a leaf first. A malformed map could still have an inner
// node claiming kLeafDepth, and getChildNodeID below throws in that case: reject rather
// than let the throw escape uncaught. Not reachable through any public entry point,
// since addKnownNode already marks such a map invalid, so no test can cover this.
// Same kLeafDepth hazard as in visitDifferences above. That's the only caller and already
// bails out before reaching here; this is a second line of defense since the loop below
// descends on its own and doesn't depend on the caller's bound.
if (nodeID.getDepth() >= kLeafDepth)
{
// LCOV_EXCL_START
@@ -830,15 +839,30 @@ SHAMap::verifyProofPath(uint256 const& rootHash, uint256 const& key, std::vector
if (node->getHash() != hash)
return false;
auto const depth = std::distance(path.rbegin(), rit);
auto const depth = static_cast<unsigned int>(std::distance(path.rbegin(), rit));
if (node->isInner())
{
auto nodeId = SHAMapNodeID::createID(static_cast<unsigned int>(depth), key);
// Nibbles run out at kLeafDepth, so only the leaf terminating the path may sit
// there. These nodes come off the wire, so a peer can still claim an inner one;
// reject it rather than passing this depth to selectBranch.
SOMETIMES(
depth >= kLeafDepth, "xrpl::SHAMap::verifyProofPath : inner at leaf depth");
if (depth >= kLeafDepth)
return false;
auto nodeId = SHAMapNodeID::createID(depth, key);
hash = safeDowncast<SHAMapInnerNode*>(node.get())
->getChildHash(selectBranch(nodeId, key));
}
else
{
// The hash chain up to rootHash only proves this leaf sits where the path claims,
// not that it is the leaf for `key`: a peer could substitute any other leaf whose
// subtree hashes to the same value at every level above it. Checking the terminal
// leaf's own key is what ties the proof to `key` specifically.
if (leafKey(*node) != key)
return false;
// should exhaust all the blobs now
return depth + 1 == path.size();
}

72
src/tests/libxrpl/shamap/SHAMap.cpp
View File

@@ -3,19 +3,23 @@
#include <xrpl/basics/Blob.h>
#include <xrpl/basics/Buffer.h>
#include <xrpl/basics/SHAMapHash.h>
#include <xrpl/basics/Slice.h>
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/protocol/Serializer.h>
#include <xrpl/shamap/SHAMapInnerNode.h>
#include <xrpl/shamap/SHAMapItem.h>
#include <xrpl/shamap/SHAMapLeafNode.h>
#include <xrpl/shamap/SHAMapMissingNode.h>
#include <xrpl/shamap/SHAMapNodeID.h>
#include <xrpl/shamap/SHAMapTreeNode.h>
#include <gtest/gtest.h>
#include <helpers/TestSink.h>
#include <shamap/common.h>
#include <algorithm>
#include <array>
#include <cstddef>
#include <cstdint>
@@ -346,4 +350,72 @@ TEST_F(SHAMapPathProof, verify_proof_path)
EXPECT_FALSE(map.verifyProofPath(rootHash, key, badPath));
}
// A legitimate proof path for two keys sharing all 63 leading nibbles is 65 elements: inner nodes
// at depths 0..63 plus the leaf at depth 64. This pins that the 65 bound is real, so the fix for
// the forged-path case below must not simply tighten the length limit.
TEST_F(SHAMapPathProof, legitimate_deep_path_is_sixty_five_elements)
{
tests::TestNodeFamily f{j_};
SHAMap map{SHAMapType::FREE, f};
map.setUnbacked();
auto const kA = uint256{std::string_view{std::string(63, 'a') + "1"}};
auto const kB = uint256{std::string_view{std::string(63, 'a') + "2"}};
for (auto const& k : {kA, kB})
{
Buffer vuc{32};
std::fill_n(vuc.data(), vuc.size(), std::uint8_t{1});
ASSERT_TRUE(map.addItem(SHAMapNodeType::TnAccountState, makeShamapitem(k, std::move(vuc))));
}
map.invariants();
auto const pathA = map.getProofPath(kA);
ASSERT_TRUE(pathA.has_value());
// NOLINTBEGIN(bugprone-unchecked-optional-access) has_value() checked above
EXPECT_EQ(pathA->size(), 65u);
EXPECT_TRUE(SHAMap::verifyProofPath(map.getHash().asUInt256(), kA, *pathA));
// NOLINTEND(bugprone-unchecked-optional-access)
auto const pathB = map.getProofPath(kB);
ASSERT_TRUE(pathB.has_value());
// NOLINTBEGIN(bugprone-unchecked-optional-access) has_value() checked above
EXPECT_EQ(pathB->size(), 65u);
EXPECT_TRUE(SHAMap::verifyProofPath(map.getHash().asUInt256(), kB, *pathB));
// NOLINTEND(bugprone-unchecked-optional-access)
}
// A forged path of 65 hash-chained inner nodes reaches depth kLeafDepth, where only the leaf
// terminating the path may sit. Such a path must be rejected.
TEST_F(SHAMapPathProof, all_inner_path_at_leaf_depth_is_rejected)
{
// An arbitrary well-formed key; the test does not care about its specific value.
constexpr uint256 kTestKey("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8");
// Build upwards from the deepest node so each parent's selected branch carries its child's hash
// and the hash chain validates at every level.
std::vector<Blob> path;
SHAMapHash childHash{uint256{1}};
for (auto depth = SHAMap::kLeafDepth + 1u; depth-- > 0;)
{
auto const id = SHAMapNodeID::createID(std::min(depth, SHAMap::kLeafDepth - 1u), kTestKey);
auto const branch = selectBranch(id, kTestKey);
Serializer s;
for (auto i = 0u; i < SHAMap::kBranchFactor; ++i)
s.addBitString(i == branch ? childHash.asUInt256() : uint256{});
s.add8(kWireTypeInner);
path.push_back(s.getData());
auto node = SHAMapTreeNode::makeFromWire(makeSlice(path.back()));
ASSERT_TRUE(node);
node->updateHash();
childHash = node->getHash();
}
ASSERT_EQ(path.size(), 65u);
EXPECT_FALSE(SHAMap::verifyProofPath(childHash.asUInt256(), kTestKey, path));
}
} // namespace xrpl::tests