Conversation
1102a40 to
02a90b4
Compare
d2aa9aa to
dcc48d8
Compare
AkramBitar
left a comment
There was a problem hiding this comment.
The fix is correct and well-structured — maxParseNodes on the parser, memoised evalRef on the evaluator, fuzz target wired into the nightly matrix, unit tests for both the cap and the at-limit boundary. One blocker before I can approve:
parser.go uses fmt.Errorf instead of the required errors.Errorf
Per AGENTS.md, fmt must not be used to build or wrap errors anywhere in the tree — use github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors throughout. The specific line:
p.err = fmt.Errorf("policy expression exceeds maximum node count of %d", maxParseNodes)should be
p.err = errors.Errorf("policy expression exceeds maximum node count of %d", maxParseNodes)with the "fmt" import removed from parser.go if that was its only use.
That's the only change needed — everything else looks good.
6093d99 to
ab6bbf0
Compare
|
" |
Signed-off-by: Effi-S <effi.szt@gmail.com>
Fixes #2077
Summary
The boolpolicy parser's
maxParseDepthlimits how deeply nested parentheses can be, but the ASTevaluator that walks the parsed policy has no equivalent bound, and evaluation isn't memoized. A
policy string that stays within the parser's depth limit and length limit can still produce a large,
repetitive AST that causes the evaluator to repeat the same verification work many times over.
Where
token/services/identity/boolpolicy/parser.go:38setsmaxParseDepth = 64, incremented only whenentering a parenthesized group (
parser.go:259-265).parseOr/parseAndbuild their chainsiteratively, so a policy like
$0 OR $0 OR ... OR $0— well withinmaxPolicyLen(4 KiB) — producesa long, flat-but-large AST without ever tripping the depth counter.
sig.go:91-110evalNodewalks that AST and, for eachRefNodeoccurrence, callsv.Verifiers[i].Verifyagain — so a policy referencing the same signature index many times causesthat verification to run many times, even though the signature itself only needs to be checked once.
parser.go:66-82String()recurses over the same AST shape.Impact
Because the parser's depth bound doesn't constrain how large or repetitive the resulting AST can be,
a policy that's well-formed and within the configured size limit can still cause a disproportionate
amount of cryptographic verification work relative to its size, on the identity-matching path. This
is a resource-usage concern rather than a crash, and is worth addressing with either memoization or
an explicit bound on total AST node count / reference count.
Suggested fix
Add a cap on the total number of nodes (or
RefNodeoccurrences) produced during parsing, alongsidethe existing depth cap, so the AST size itself is bounded regardless of shape. Separately, memoizing
evalNode's per-index verification result within a singleVerifycall (a signature at a givenindex is being checked against the same data regardless of how many times it's referenced in the
policy) would remove the redundant work without needing to change the AST structure at all.
Severity
MEDIUM-HIGH — bounded by the existing 4 KiB policy-length limit, but the per-length cost is higher
than intended given the parser's depth cap doesn't constrain it.