diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index a12e524a5f..8849bbda14 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -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(std::distance(path.rbegin(), rit)); if (node->isInner()) { - auto nodeId = SHAMapNodeID::createID(static_cast(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(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(); } diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index c84cdf504f..92492c514e 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -3,19 +3,23 @@ #include #include #include +#include #include #include #include +#include #include #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -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 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