network-driver: QuerySpentTokens/deleteTokens: unvalidated response length drives an unguarded index and panics

[Effi-S]

Fixes #2059

Summary

spentTokenFetcher.QuerySpentTokens unmarshals the chaincode's areTokensSpent response directly into []bool with no check that its length matches the number of ids requested. Its sole caller, Service.deleteTokens, then indexes the result positionally against its own input slice with no bounds check — an unguarded index driven entirely by an unvalidated response length from the network layer.

Where

token/services/network/fabric/tokenfetcher.go:94-132 (client):

query := stdChannelChaincode.Query(AreTokensSpent, idsRaw)
payloadBoxed, err := query.Call()
...
var spent []bool
if err := json.Unmarshal(payloadBoxed, &spent); err != nil { ... }
return spent, nil   // tokenfetcher.go:126-131 — no length check against len(IDs)

token/services/tokens/tokens.go:314-349 (consumer):

spent, err := network.AreTokensSpent(ctx, tms.Namespace(), ids, meta)
...
for i, tok := range tokens {
    if spent[i] {   // tokens.go:337 — panics if len(spent) < len(tokens)
        toDelete = append(toDelete, &tok.Id)
    }
}

On the chaincode side, Translator.AreTokensSpent (token/services/network/common/rws/translator/translator.go:170) does build its result as make([]bool, len(ids)) against the ids it received — so this is exploitable only if something between the client's request and the chaincode's response can desynchronize the two ids lists (e.g. a future refactor, an alternate driver/backend implementing the same interface less carefully, or a compromised/buggy peer returning a manipulated response), but nothing in the current code path structurally prevents that mismatch from reaching deleteTokens and panicking.

Impact

Any response shorter than the caller's tokens slice (whether from a bug in an alternate chaincode/driver implementation, a malformed/truncated response, or a future change that decouples request/response ordering) panics deleteTokens on the very next token-vault cleanup pass — a client-side crash driven entirely by data returned from the network layer, with no defensive check at the trust boundary between "what the chaincode returned" and "what we assumed it returned."

Reproduction

Reproduced locally with a unit test (not yet committed) exercising the full, real production call path — Service.PruneInvalidUnspentTokens → Service.deleteTokens → *network.Network.AreTokensSpent — with only the driver.Network boundary faked:

fakeDriverNetwork := &tokensmock.FakeNetwork{}
fakeDriverNetwork.AreTokensSpentReturns([]bool{true}, nil)   // 1 bool for 2 requested tokens
net := network.NewNetwork(fakeDriverNetwork, nil)
...
svc := tokens.NewService(tmsID, tmsProvider, networkProvider, storage, nil)

require.Panics(t, func() {
    _, _ = svc.PruneInvalidUnspentTokens(context.Background())
}, "expected an index-out-of-range panic in deleteTokens when spent[] is shorter than the token batch")

A contrast test with a correctly-sized spent slice proves the panic is specifically caused by the length mismatch. Happy to include both in the fix PR.

Suggested fix

Validate len(spent) == len(IDs) in QuerySpentTokens before returning (returning an error on mismatch instead of the raw unmarshaled slice), and/or add a defensive bounds check in deleteTokens before indexing spent[i].

Severity

HIGH

[AkramBitar]

@Effi-S

Thanks a lot for submitting this PR.

The guard here is right, but it only covers the Fabric fetcher. The sibling implementation of the saterface — Executor.QuerySpentTokens in token/services/network/fabricx/qe/qe.go — violates the samepositional-length contract by construction, so the panic this PR closes stays reachable on a fabricx TMS:

keys := make([]driver.PKey, 0, len(ids))
for _, id := range ids {
if id == nil {
continue // id silently dropped
}
...
keys = append(keys, outputID)
}
if len(keys) == 0 {
return nil, nil // all-nil ids -> len 0 with a nil error
}
...
spentFlags := make([]bool, len(keys)) // sized off keys, not ids

Any nil entry in ids yields len(spentFlags) < len(ids), and an all-nil slice returns nil, nil — exactly the shape that makes spent[i] panic in tokens.deleteTokens and htlc.OwnerWallet.deleteTokens.

It isn't triggerable from today's two callers (both build ids[i] = &tok.Id, never nil), so this is latent rather than a live bug — but it's the same defect in the same interface, and a future caller is a more likely trigger than a misbehaving peer.

[AkramBitar]

See my comment in the PR itself

[Effi-S]

@Effi-S

Thanks a lot for submitting this PR.

The guard here is right, but it only covers the Fabric fetcher. The sibling implementation of the saterface — Executor.QuerySpentTokens in token/services/network/fabricx/qe/qe.go — violates the samepositional-length contract by construction, so the panic this PR closes stays reachable on a fabricx TMS:

keys := make([]driver.PKey, 0, len(ids)) for _, id := range ids { if id == nil { continue // id silently dropped } ... keys = append(keys, outputID) } if len(keys) == 0 { return nil, nil // all-nil ids -> len 0 with a nil error } ... spentFlags := make([]bool, len(keys)) // sized off keys, not ids

Any nil entry in ids yields len(spentFlags) < len(ids), and an all-nil slice returns nil, nil — exactly the shape that makes spent[i] panic in tokens.deleteTokens and htlc.OwnerWallet.deleteTokens.

It isn't triggerable from today's two callers (both build ids[i] = &tok.Id, never nil), so this is latent rather than a live bug — but it's the same defect in the same interface, and a future caller is a more likely trigger than a misbehaving peer.

@AkramBitar Done

[AkramBitar]

LGTM