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.

[atharrva01]

A question rather than a request, since I might be weighing this differently to you.

When the recover fires, queryByID's own defer close(ch) has already run during unwinding, so the caller gets a closed empty channel and no error. ScanFromBlock is never reached, so those keys get no block scan fallback either. As far as I can tell the listener then just never fires for them, which looks like a silent permanent non-resolution rather than a failed request.

Your own test pins this, which is what made me notice: TestQueryByID_PanicIsRecovered asserts assert.False(t, scanner.called) (line 274).

A crash is clearly worse, so this is still an improvement. But the Fix A path is careful to set startDelivery and degrade to the scan, and it might be nice if the recovered path landed somewhere similar rather than closing empty. I appreciate that is awkward given the inner defer has already closed the channel, so possibly not worth it here.

[atharrva01]

Not for this PR, just noting it while it is in view: the finality side has the same bare go q.queryByID(...) with no recover (network/fabric/finality/deliveryqs.go:57). Fix A's bug genuinely is not there, but if an unrecovered panic in one of these background goroutines counts as a node-crash surface, the argument for Fix B seems to apply to that one too.

Happy to open an issue for it so it does not widen this PR, if you think it is worth tracking.

[atharrva01]

Minor, and possibly not worth changing: this leans on slices2.GetAny panicking on an empty slice, so the test is tied to how that FSC helper behaves on empty input rather than to anything in this file.

A fakeQuerier whose QueryStates panics would pin Fix B through one of the interfaces the test already controls, and would keep working regardless of what the helper does. The current version does exercise the recover today, so this is only about how it ages.