Guard queryByID against oversized peer responses to prevent node crash

[Effi-S]

Fixes #2055

Summary

queryByID unmarshals a single peer's raw chaincode-query response directly into values, then ranges over it while indexing the original keys slice by position — with nothing validating that the response returned exactly len(keys) elements. This function runs inside a goroutine with no recover() anywhere in the call chain, so an oversized response crashes the whole process, not just the request.

Where

token/services/network/fabric/lookup/deliveryqs.go:109-132:

values := make([][]byte, 0, len(keys))
err = json.Unmarshal(res, &values)
...
for i, value := range values {
    ...
    notFound = append(notFound, keys[i])   // deliveryqs.go — panics if len(values) > len(keys)
    ...
}

res is the raw response from a single peer's chaincode query (ChannelStateQuerier.QueryStates → Channel.Chaincode(ns).Query(...).Query()).

Impact

queryByID runs inside a goroutine spawned by QueryByID (go q.queryByID(...)) with no recover() anywhere in the call chain. A byzantine, buggy, or simply out-of-sync peer that answers a QueryStates request with more elements than were requested for a given namespace drives i past the end of keys, panicking that goroutine — which is unrecovered and therefore crashes the entire process, not just the one request. This is a full-availability attack surface reachable by anything capable of influencing or replacing a single peer's chaincode-query response.

Fix

Two independent, complementary defenses:

A — Validate the response length before the loop. Never trust the peer to return the right number of values. A mismatch is treated like the other failure cases already handled in this function (bad marshal, query error): log it and fall back to the slower block scan instead of trusting the response.

if len(values) != len(keys) {
    logger.Errorf("peer returned %d values for %d keys in ns [%s]; falling back to block scan",
        len(values), len(keys), ns)
    startDelivery = true
    continue // treat as a per-namespace failure (=> fall back to the slow block scan)
}

B — Wrap the goroutine body in recover() (defense in depth). Even after Fix A, any future unforeseen panic in this background path must degrade to a failed request rather than crashing the node.

go func() {
    defer func() {
        if r := recover(); r != nil {
            logger.Errorf("recovered from panic in queryByID: %v", r)
        }
    }()
    q.queryByID(ctx, keys, ch, startingBlock, evicted)
}()

Tests

TestQueryByID_OversizedResponse in deliveryqs_test.go feeds a crafted response with more values (2) than keys requested (1). Before the fix this panics; after Fix A it delivers nothing for the oversized namespace and falls back to the block scan:

querier := &fakeQuerier{results: map[driver.Namespace]querierResult{
    "ns1": {raw: values(t, []byte("v1"), []byte("v2-unexpected"))},
}}
scanner := &fakeScanner{}

ch, err := newQuery(querier, scanner).QueryByID(t.Context(), 100, evictedFor(map[driver.Namespace]driver.PKey{
    "ns1": "k1",
}))
require.NoError(t, err)
assert.Empty(t, drain(ch))
assert.True(t, scanner.called, "the oversized response must fall back to the block scan")

[Effi-S]

@adecaro , Any suggestions regarding these changes?

[atharrva01]

hi @Effi-S , I spent some time in this function recently (#1426, #1999) so I read this one closely. The panic looks real to me: the loop ranges values but indexes keys, and since for ns, keys := range keysByNS shadows the outer parameter, the check is comparing against that namespace's keys, which is the right thing to compare against. Using != rather than > also seems right, since a short response would index in range but pair values with the wrong keys.

One thing I checked in case it was worth widening the PR: the finality twin at network/fabric/finality/deliveryqs.go does not have this bug, it walks the key set and fetches per txID with no positional indexing. So Fix A really is specific to the lookup path.

Ran the package at -count=2 -race, green. Three notes below, all non-blocking, and the first is more of a question than a suggestion.

[AkramBitar]

@Effi-S, few comments

[Effi-S]

I'm going to push the out of bounds check only.
We will leave the recover up to future discussion.

[AkramBitar]

LGTM