fix: reject truncated proofs in validateCompleteness (GHSA-r9fq-g486-v8pg)

[rootulp]

Summary

Fixes a defense-in-depth gap in validateCompleteness reported in GHSA-r9fq-g486-v8pg.

The left-traversal loop in validateCompleteness exits when either leafIndex == proof.Start() or len(nodes) == 0. In the second case, rightSubtrees gets the (now empty) nodes slice, the right-side namespace-check loop iterates zero times, and the function silently returns nil — even though no completeness check actually ran.

Add an explicit post-loop guard that returns a wrapped ErrFailedCompletenessCheck so early node exhaustion fails loudly and is identifiable via errors.Is, matching the two existing sentinel returns in the same function.

Impact

Narrow. The end-to-end VerifyLeafHashes / ComputeRootWithBasicValidation paths were still protected because computeRoot rejects truncated proofs (wrong root hash). The reporter's own PoC confirmed VerifyNamespace still returns false. But any future caller that split completeness checking from root verification would have been exposed, and the inner invariant was simply wrong — hence the fix.

Tests

Three unit tests in proof_completeness_test.go, written before the fix to verify red/green:

• TestValidateCompleteness_TruncatedProof_RejectsEmptyNodes — start=8, empty nodes. Loop exits immediately; without the fix, returns nil.
• TestValidateCompleteness_TruncatedProof_RejectsInsufficientNodes — start=12, one node. Loop consumes it on the first iteration, advances leafIndex to 8, then exits because nodes is empty even though leafIndex != 12.
• TestValidateCompleteness_RightSideCheck_StillRejectsSmallNamespace — control: start=0 with a right-subtree node whose namespace is <= the target. The existing right-side check still fires (returns ErrFailedCompletenessCheck), confirming the fix narrowly rejects truncation rather than breaking the normal path.

Both truncation tests assert identity via require.ErrorIs(err, ErrFailedCompletenessCheck).

Verified red/green:

• Before the proof.go change: two new tests fail, control passes.
• After the fix: all three pass; full go test ./... suite green.

Review follow-ups

• fd647f5: addressed gemini-code-assist review — wrap ErrFailedCompletenessCheck with %w in the new return and use require.ErrorIs in the tests for consistency with the other completeness-check returns in this function.

Addresses GHSA-r9fq-g486-v8pg.

Test plan

• go test ./... passes
• Three new tests cover empty-nodes, insufficient-nodes, and control cases
• Red/green verified manually
• Error is errors.Is-identifiable as ErrFailedCompletenessCheck

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request addresses a security vulnerability (GHSA-r9fq-g486-v8pg) by adding a guard in validateCompleteness to ensure the proof is not truncated before reaching the start index. It also includes new unit tests to verify this behavior. Feedback suggests wrapping the new error with the ErrFailedCompletenessCheck sentinel for consistency with existing checks and updating the corresponding tests to use require.ErrorIs for more precise error verification.

[rootulp]

Visual explainer — how the completeness check works and what this PR fixes

Diagram 1 — What a namespace proof looks like

Concrete example: 8-leaf NMT, target namespace 0x05, proof range [4, 6).

flowchart TB
    classDef target fill:#ffd6d6,stroke:#c33,stroke-width:2px,color:#000
    classDef left fill:#d4e8ff,stroke:#2a5f99,color:#000
    classDef right fill:#ffe6c7,stroke:#995a00,color:#000
    classDef note fill:#fafafa,stroke:#888,color:#000

    Q["Query: give me every leaf with namespace=05<br/>Prover: it is leaves [4, 6), here is the proof"]:::note

    subgraph T["NMT leaves sorted by namespace"]
        direction LR
        L0["leaf 0<br/>ns=01"]
        L1["leaf 1<br/>ns=01"]
        L2["leaf 2<br/>ns=02"]
        L3["leaf 3<br/>ns=02"]
        L4["leaf 4<br/>ns=05"]:::target
        L5["leaf 5<br/>ns=05"]:::target
        L6["leaf 6<br/>ns=09"]
        L7["leaf 7<br/>ns=09"]
    end

    LS["leftSibling<br/>covers [0,4)<br/>min=01, max=02<br/>check: max < 05 ✓"]:::left
    RS["rightSibling<br/>covers [6,8)<br/>min=09, max=09<br/>check: min > 05 ✓"]:::right

    P["proof.Start=4, proof.End=6<br/>proof.nodes = leftSibling, rightSibling"]:::note

    Q --> P
    P --> LS
    P --> RS
    LS -.-> L0
    LS -.-> L3
    RS -.-> L6
    RS -.-> L7

Loading

How it works: validateCompleteness walks proof.nodes left-to-right, consuming entries into a "left bucket" until leafIndex reaches proof.Start(). Whatever remains goes into a "right bucket." Then it checks:

• Every left sibling's max namespace is < the target (nothing to the left could have namespace 05)
• Every right sibling's min namespace is > the target (nothing to the right either)

If both pass → the proof is complete (no leaves with the target namespace exist outside the proven range).

---

Diagram 2 — The bug and the fix

The loop exits when either leafIndex reaches proof.Start() (correct) or len(nodes) == 0 (bug). An attacker can submit a truncated proof to trip the second condition.

flowchart TB
    classDef bad fill:#ffd6d6,stroke:#c33,stroke-width:2px,color:#000
    classDef good fill:#d9f2d9,stroke:#2a7a2a,stroke-width:2px,color:#000
    classDef step fill:#f9f9f9,stroke:#888,color:#000

    Input(["Truncated proof from attacker:<br/>proof.Start = 8, proof.nodes = empty<br/>targetNID = 05"]):::step

    subgraph Before["BEFORE this PR"]
        direction TB
        b1{"loop: leafIndex != 8<br/>AND len nodes > 0?"}:::step
        b2["exits on len=0<br/>leafIndex still 0"]:::step
        b3["rightSubtrees = nodes = empty"]:::step
        b4["left check: 0 iterations"]:::step
        b5["right check: 0 iterations"]:::step
        b6(["return nil<br/>proof silently accepted"]):::bad

        b1 -- "no: len=0" --> b2 --> b3 --> b4 --> b5 --> b6
    end

    subgraph After["AFTER this PR"]
        direction TB
        a1{"loop: leafIndex != 8<br/>AND len nodes > 0?"}:::step
        a2["exits on len=0<br/>leafIndex still 0"]:::step
        a3{"NEW GUARD:<br/>leafIndex == proof.Start?"}:::step
        a4(["return ErrFailedCompletenessCheck<br/>proof nodes insufficient: reached 0, expected 8"]):::good

        a1 -- "no: len=0" --> a2 --> a3
        a3 -- "no: 0 is not 8" --> a4
    end

    Input --> b1
    Input --> a1

Loading

The fix is 3 lines: after the loop, check whether leafIndex actually reached proof.Start(). If not, the nodes were exhausted early and the completeness check never actually ran — return ErrFailedCompletenessCheck instead of silently succeeding.