V72 420 vulnerability fix

[Naviabheeman]

Fixed all High, Medium and Low risk issues from the report:

High

• SCHNORR-1 — Uncompressed aggregate-pubkey accepted in xfield rotation
• PSBT-1 / RPC-1 — Unchecked non_witness_utxo->vout[prevout.n] OOB read

Medium

• P2P-1A — CMPCTBLOCK READ_STATUS_FAILED + !first_in_flight heap use-after-free
• P2P-1B — MarkBlockAsReceived(hash, std::nullopt) only erases one peer's entry
• REST-1 — rest_getutxos memory amplification DoS
• REST-2 — rest_getutxos chainActive race outside cs_main
• ZMQ-1 — Shutdown asserts on uninitialized notifier after Initialize failure
• HTTPAUTH-1 — Failed-auth MilliSleep(250) on worker thread saturates RPC pool

Low

• PSBT-2/3/4 (combinepsbt empty crash, converttopsbt inverted predicate, PSBT_IN_SIGHASH duplicate gate)
• P2P-2 (FindNextBlocksToDownload equal_range(...).second past-the-end deref) — 1-character fix
• P2P-3 (ADDR handler lost version gate)
• RPC-2/3/4/5 (combinepsbt dup, setban overflow, dumptxoutset path traversal, decoderawtransaction unnecessary cs_main, wait* unlimited timeout)
• REINDEX-1 (VerifyDB Level 4 no ShutdownRequested() poll)
• INIT-1/2 (-persistmempool=true silently false, -bytespersigop unsigned wrap)
• WALLET-1/3/4 (IssueReissuableToken HaveKey check, feebumper minconf gate, listunspent minimumAmount scaling on token UTXOs)
• XFD-2 (CXField::Unserialize NONE type silently passes trailing bytes)
• CC-2 / TXV-4 (ConnectBlock CheckTxInputs no DoS(100), VerifyTokenBalances tpcin<=0 DoS=0)
• CCV-1 (ColorIdentifier deserialization ctors missing payload{} initializer)
• PKG-7 (testmempoolaccept chained-children missing-inputs)

Issue PKG-1 — SubmitPackageToMempool partial admission not rolled back:
In Tapyrus packages may be partially submitted. There is no "all-or-nothing" guarantee. So the rollback path is unnecessary

[azuchi]

Summary

This PR addresses the v0.7.2 pre-release report findings thoroughly and with high quality. All HIGH (2) findings, 6 of 7 MEDIUM (with PKG-1 explicitly declined with sound rationale), and all LOW items are correctly fixed, and the new regression tests are comprehensive. Two issues need to be addressed before merge.

---

✅ Findings successfully fixed

Finding	Outcome
SCHNORR-1 (HIGH)	Recommended fix + 11-case unit test covering compressed / uncompressed / hybrid / empty / size / prefix / off-curve / chain-to-CXField::IsValid()
PSBT-1 / RPC-1 (HIGH)	Bounds check added at all four sites (PartiallySignedTransaction::Unserialize, IsSane, SignPSBTInput, decodepsbt) + 3-case regression test
P2P-1A (MEDIUM)	return true added after MarkBlockAsReceived in the !first_in_flight branch + dedicated functional test that reproduces UAF via duplicate short IDs
P2P-1B (MEDIUM)	Loop refactored to use iterator + found flag + if (from_peer) return true; + post-condition assert. Stronger than my recommendation.
REST-1 (MEDIUM)	Body size cap applied to both BIN and HEX paths (HEX path correctly doubles the limit) + functional test
REST-2 (MEDIUM)	chainHeight and chainTip snapshotted under cs_main and reused in all three response paths
ZMQ-1 (MEDIUM)	if (!psocket) return; + psocket = nullptr on zmq_bind failure to prevent double-free
HTTPAUTH-1 (MEDIUM)	Per-IP backoff LRU with exponential backoff (250 ms → 32 s cap), bounded to 1024 entries with careful eviction policy, successful auth clears entry. Functional test verifies 4 parallel bad-auth complete under 1 s.
PKG-1 (MEDIUM)	Declined with the rationale that Tapyrus packages have no all-or-nothing guarantee. Acceptable as a documented design decision.
CCV-1, CC-2, TXV-4, PSBT-2/3/4, P2P-2/3, RPC-2/3/4/5, REINDEX-1, INIT-1/2, WALLET-1/3/4, XFD-2, PKG-7 (LOW / INFO)	All correctly fixed

---

🚨 Issues requiring changes before merge

1. Build failure: src/test/zmq_tests.cpp is missing

src/test/CMakeLists.txt adds zmq_tests.cpp to the test sources, but the file is not present in the PR:

   validation_block_tests.cpp
   xfieldhistory_tests.cpp
   xfield_tests.cpp
+  zmq_tests.cpp     # ← added to CMake, but the file is not in the PR
   ${JSON_HEADERS}

$ git show pr423-review:src/test/zmq_tests.cpp
fatal: path 'src/test/zmq_tests.cpp' does not exist in 'pr423-review'

This will fail CMake configure / compile. Please either commit the intended ZMQ-1 unit test or remove the entry from CMakeLists.txt.

2. dumptxoutset parent-path containment check is bypassable

In src/rpc/blockchain.cpp (around line 2274):

const std::string cp = canonical_parent.string();
const std::string cd = canonical_datadir.string();
if (cp.substr(0, cd.size()) != cd) {
    throw JSONRPCError(RPC_INVALID_PARAMETER, "Path resolves outside the data directory");
}

This is plain string-prefix comparison without a path-separator boundary check. A sibling directory whose name shares the datadir prefix bypasses it.

Scenario (admin-only, but worth fixing):

• datadir = /home/user/.tapyrus
• An attacker with RPC access and datadir write permission creates a symlink /home/user/.tapyrus/evil → /home/user/.tapyrus2/
• dumptxoutset evil/snapshot resolves canonical_parent = /home/user/.tapyrus2
• cp.substr(0, 19) == "/home/user/.tapyrus" == cd → accepted
• The snapshot lands in /home/user/.tapyrus2/, outside datadir

Suggested fix:

if (cp.size() < cd.size() ||
    cp.substr(0, cd.size()) != cd ||
    (cp.size() > cd.size() && cp[cd.size()] != fs::path::preferred_separator)) {
    throw JSONRPCError(RPC_INVALID_PARAMETER, "Path resolves outside the data directory");
}

Or use std::filesystem::relative(canonical_parent, canonical_datadir) and reject if the result starts with ...

Severity is LOW (authenticated RPC plus filesystem write access required), but since this is the very mitigation being introduced, please close the gap.

---

Bonus hardening worth highlighting

The PR includes several improvements that go beyond the report:

• PSBTInput::Merge now throws on sighash_type mismatch (silent discard previously could let a combined PSBT be signed with the wrong sighash).
• CheckBlockHeader enforces block.proof.size() == SCHNORR_SIGNATURE_SIZE — this is the defense in depth that backs the comment in the CXField::Unserialize NONE branch (XFD-2 hardening).
• SubmitPackageToMempool adds a g_connman null guard.
• The self-review commit (ee36565fe) added symlink canonicalization to dumptxoutset — the right direction, just needs the separator-boundary fix noted above.
• Comment cleanups: satoshis → TPC, BTC/kB → TPC/kB in functional tests. Small but appreciated.

Notable design choices

• PKG-7: The report described the bug as "chained children spuriously report missing-inputs". The PR uncovered the actual underlying defect: the virtualView was adding missingInputs transactions to the overlay, causing chained children to spuriously pass validation as ALLOWED. The !opt.state.missingInputs gate is the correct fix, and testmempoolaccept_missing_inputs_no_overlay_leak is a clean regression test.
• WALLET-1: Choosing throw RPC_WALLET_ERROR over fall-through-to-keypool surfaces the misconfiguration to the user explicitly. Better than silently allocating a new address.
• CC-2: Capturing the inner RejectCode and RejectReason before state.DoS() overwrites them preserves the specific failure cause for logs — a nice touch.

Minor observations (not blocking)

1. REST-2 — chainActive.Tip() is dereferenced inside the cs_main snapshot without a null check. The race is fully fixed, but the (very narrow) shutdown-teardown window where Tip() could be nullptr is not handled. Acceptable for this severity; documenting the assumption nearby would help.
2. HTTPAUTH-1 — With the 1024-entry cap, an attacker with > 1024 distinct IPs effectively gets "one free attempt per evicted IP" against the throttle. Inherent limit of per-IP LRU throttling; a global-rate backstop would be a reasonable v0.7.3 hardening, not required here.
3. wait* RPCs — Converting timeout=0 to a 10-minute effective timeout is the right call, but it is a documented behavior change. Worth a line in the release notes so monitoring scripts that expected indefinite waits can be adjusted.

Recommendation

Approve once the two issues above are addressed.

After merge to master, this will need a backport PR to the v0.7 branch for inclusion in v0.7.2.

Excellent overall response to the report.

[azuchi]

Re-review for 394732fca

Both previous blockers are fixed correctly. One follow-up issue.

✅ Blocker 1 — zmq_tests.cpp

108-line test file added. The key case shutdown_without_initialize_noop directly validates the ZMQ-1 fix; 5 supporting cases cover null context, idempotent shutdown, and shared-socket ref-counting. Solid.

✅ Blocker 2 — dumptxoutset containment

fs::relative + *rel.begin() == ".." matches my suggested alternative. Combined with the pre-checks (absolute reject, .. component reject) from 6147d75256, this is defense in depth at both input and canonical layers. Symlink-escape (evil → /elsewhere/) correctly rejected.

🚨 Follow-up — rpc_dumptxoutset.py:88-90 will fail

invalid_path = os.path.join(get_datadir_path(...), "invalid", "path")  # absolute
assert_raises_rpc_error(-8, "Couldn't open file temp file for writing", ...)

The new pre-check rejects this earlier with "Path must be relative". Code (-8) matches, but assert_raises_rpc_error does a substring check on the message — so the assertion fails. The previous CI hid this by failing at the zmq_tests.cpp build step; the current run will surface it.

Suggested update:

assert_raises_rpc_error(-8, "Path must be relative", node.dumptxoutset, "/tmp/foo/snap")
assert_raises_rpc_error(-8, "Path must not contain '..'", node.dumptxoutset, "../snap")
assert_raises_rpc_error(-8, "Couldn't open file temp file for writing",
    node.dumptxoutset, "nonexistent_subdir/path")  # relative, write fails

Adding a symlink-escape test case would also directly cover the bypass that prompted this change.

---

Ready to approve and backport to v0.7 once the functional test is updated.