identity/boolpolicy: parser depth cap doesn't bound the evaluator, and evaluation isn't memoized

.github/workflows/nightly-fuzz.yml

@@ -48,6 +48,9 @@ jobs:
           - name: identity-marshal-decode-identity
             pkg: ./token/services/identity/marshal
             func: FuzzDecodeIdentityNoPanic
+          - name: identity-boolpolicy-parse
+            pkg: ./token/services/identity/boolpolicy
+            func: FuzzParseNoPanic
           - name: identity-multisig-deserializer
             pkg: ./token/services/identity/multisig
             func: FuzzMultiIdentityDeserializeNoPanic

token/services/identity/boolpolicy/parser.go

@@ -18,11 +18,12 @@ SPDX-License-Identifier: Apache-2.0
 package boolpolicy
 
 import (
-	"errors"
 	"fmt"
 	"strconv"
 	"strings"
 	"unicode"
+
+	"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
 )
 
 const (
@@ -36,6 +37,17 @@ const (
 	// Go call stack; capping at 64 prevents goroutine stack exhaustion from
 	// attacker-supplied deeply-nested expressions.
 	maxParseDepth = 64
+
+	// maxParseNodes is the maximum number of AST nodes (RefNode/AndNode/OrNode)
+	// that Parse will construct for a single expression.  The depth cap bounds
+	// the shape of the tree but not its size: a flat, unparenthesised chain such
+	// as "$0 OR $1 OR ..." stays at depth 0 while producing one node per token.
+	// Capping the total node count bounds the memory footprint of the AST — and
+	// the work any later traversal performs — regardless of expression shape.
+	// 1024 nodes comfortably covers real policies (hundreds of component
+	// identities) while staying well under the ~2000-node worst case the
+	// maxPolicyLen byte limit alone would otherwise permit.
+	maxParseNodes = 1024
 )
 
 // ---------------------------------------------------------------------------
@@ -140,11 +152,11 @@ func (l *lexer) next() (lexToken, error) {
 			l.pos++
 		}
 		if l.pos == start {
-			return lexToken{}, fmt.Errorf("expected digit after '$' at position %d", l.pos)
+			return lexToken{}, errors.Errorf("expected digit after '$' at position %d", l.pos)
 		}
 		idx, err := strconv.Atoi(string(l.runes[start:l.pos]))
 		if err != nil {
-			return lexToken{}, fmt.Errorf("invalid index at position %d: %w", start, err)
+			return lexToken{}, errors.Wrapf(err, "invalid index at position %d", start)
 		}
 
 		return lexToken{kind: tokRef, index: idx}, nil
@@ -161,11 +173,11 @@ func (l *lexer) next() (lexToken, error) {
 		case "OR":
 			return lexToken{kind: tokOr}, nil
 		default:
-			return lexToken{}, fmt.Errorf("unknown keyword %q at position %d", word, start)
+			return lexToken{}, errors.Errorf("unknown keyword %q at position %d", word, start)
 		}
 
 	default:
-		return lexToken{}, fmt.Errorf("unexpected character %q at position %d", string(ch), l.pos)
+		return lexToken{}, errors.Errorf("unexpected character %q at position %d", string(ch), l.pos)
 	}
 }
 
@@ -178,19 +190,40 @@ type parser struct {
 	lex     *lexer
 	current lexToken
 	err     error
+	nodes   int // number of AST nodes constructed so far
+}
+
+// countNode records the construction of one AST node and returns whether the
+// parser is still within the maxParseNodes budget.  On overflow it latches an
+// error (if one is not already set) so the caller can bail out immediately.
+func (p *parser) countNode() bool {
+	p.nodes++
+	if p.nodes > maxParseNodes {
+		if p.err == nil {
+			p.err = errors.Errorf("policy expression exceeds maximum node count of %d", maxParseNodes)
+		}
+
+		return false
+	}
+
+	return true
 }
 
 // Parse parses a boolean expression string and returns the root AST node.
 // It returns an error for any lexical or syntactic problems.
 //
-// Parse enforces two hard limits to prevent resource exhaustion:
+// Parse enforces three hard limits to prevent resource exhaustion:
 //   - input longer than maxPolicyLen bytes is rejected immediately.
 //   - parenthesis nesting deeper than maxParseDepth levels is rejected;
 //     this bounds the Go call-stack depth of the recursive descent and
 //     prevents goroutine stack exhaustion from attacker-supplied input.
+//   - expressions producing more than maxParseNodes AST nodes are rejected;
+//     this bounds the size of the AST (and the cost of any later traversal)
+//     independently of its shape, since the depth cap alone does not limit a
+//     flat chain such as "$0 OR $1 OR ...".
 func Parse(input string) (Node, error) {
 	if len(input) > maxPolicyLen {
-		return nil, fmt.Errorf("policy expression exceeds maximum length of %d bytes (got %d)", maxPolicyLen, len(input))
+		return nil, errors.Errorf("policy expression exceeds maximum length of %d bytes (got %d)", maxPolicyLen, len(input))
 	}
 
 	p := &parser{lex: newLexer(input)}
@@ -224,6 +257,9 @@ func (p *parser) parseOr(depth int) Node {
 	for p.err == nil && p.current.kind == tokOr {
 		p.advance()
 		right := p.parseAnd(depth)
+		if !p.countNode() {
+			return nil
+		}
 		left = &OrNode{Left: left, Right: right}
 	}
 
@@ -237,6 +273,9 @@ func (p *parser) parseAnd(depth int) Node {
 	for p.err == nil && p.current.kind == tokAnd {
 		p.advance()
 		right := p.parsePrimary(depth)
+		if !p.countNode() {
+			return nil
+		}
 		left = &AndNode{Left: left, Right: right}
 	}
 
@@ -250,14 +289,17 @@ func (p *parser) parsePrimary(depth int) Node {
 	}
 	switch p.current.kind {
 	case tokRef:
+		if !p.countNode() {
+			return nil
+		}
 		node := &RefNode{Index: p.current.index}
 		p.advance()
 
 		return node
 
 	case tokLParen:
 		if depth >= maxParseDepth {
-			p.err = fmt.Errorf("policy expression exceeds maximum nesting depth of %d", maxParseDepth)
+			p.err = errors.Errorf("policy expression exceeds maximum nesting depth of %d", maxParseDepth)
 
 			return nil
 		}
@@ -267,7 +309,7 @@ func (p *parser) parsePrimary(depth int) Node {
 			return nil
 		}
 		if p.current.kind != tokRParen {
-			p.err = fmt.Errorf("expected ')' but got token kind %v", p.current.kind)
+			p.err = errors.Errorf("expected ')' but got token kind %v", p.current.kind)
 
 			return nil
 		}
@@ -276,7 +318,7 @@ func (p *parser) parsePrimary(depth int) Node {
 		return node
 
 	default:
-		p.err = fmt.Errorf("expected '$N' or '(' at position %d", p.lex.pos)
+		p.err = errors.Errorf("expected '$N' or '(' at position %d", p.lex.pos)
 
 		return nil
 	}

token/services/identity/boolpolicy/parser_fuzz_test.go

@@ -0,0 +1,54 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package boolpolicy
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// FuzzParseNoPanic hunts for policy strings that panic Parse instead of
+// returning a Node or an error. Parse is the entry point for
+// attacker-controllable policy expressions carried inside a PolicyIdentity, so
+// it must never panic and must always terminate within its resource caps
+// (length, nesting depth, and node count).
+func FuzzParseNoPanic(f *testing.F) {
+	seeds := []string{
+		"",                        // empty
+		"$0",                      // single ref
+		"$42",                     // multi-digit ref
+		"$0 AND $1",               // simple AND
+		"$0 OR $1",                // simple OR
+		"$0 OR ($1 AND $2)",       // nested
+		"((($0)))",                // parenthesised
+		"$0 OR $0 OR $0 OR $0",    // repeated refs (exercises memoisation callers)
+		"$",                       // dangling dollar
+		"$0 NOT $1",               // unknown keyword
+		"($0 AND $1",              // unmatched open paren
+		"$0 AND $1)",              // unmatched close paren
+		"$0 AND",                  // missing operand
+		"$0 & $1",                 // unexpected character
+		"$99999999999999999999",   // index overflows int
+		"(((((((((((((((((((((",   // deeply nested opens
+		"$0 OR $1 OR $2 OR $3 OR", // trailing operator
+	}
+	for _, s := range seeds {
+		f.Add(s)
+	}
+
+	f.Fuzz(func(t *testing.T, input string) {
+		require.NotPanics(t, func() {
+			node, err := Parse(input)
+			// A successful parse must yield a non-nil node; an error must yield
+			// a nil node. Neither may panic regardless of input.
+			if err == nil {
+				require.NotNil(t, node)
+			}
+		})
+	})
+}

token/services/identity/boolpolicy/parser_test.go

@@ -299,6 +299,44 @@ func TestErrorNestingTooDeep(t *testing.T) {
 	assert.Contains(t, err.Error(), "exceeds maximum nesting depth")
 }
 
+// orChain builds a flat, unparenthesised OR chain of `refs` references using
+// the compact "$0OR$0OR..." form (no surrounding spaces), so the byte length
+// stays minimal while the node count grows.  The result has `refs` RefNodes and
+// `refs-1` OrNodes, i.e. 2*refs-1 AST nodes, all at nesting depth 0.
+func orChain(refs int) string {
+	var sb strings.Builder
+	sb.WriteString("$0")
+	for i := 1; i < refs; i++ {
+		sb.WriteString("OR$0")
+	}
+
+	return sb.String()
+}
+
+func TestErrorTooManyNodes(t *testing.T) {
+	// A flat OR chain stays at nesting depth 0 but produces one node per ref
+	// plus one per operator, so it exercises the node cap rather than the depth
+	// cap.  With R refs the chain yields 2R-1 nodes; pick R large enough to
+	// exceed maxParseNodes while the string stays under maxPolicyLen.
+	refs := maxParseNodes // 2*maxParseNodes-1 nodes, comfortably over the cap
+	policy := orChain(refs)
+	require.LessOrEqual(t, len(policy), maxPolicyLen, "test input must stay within the length cap")
+
+	_, err := Parse(policy)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "exceeds maximum node count")
+}
+
+func TestNodeCountAtLimitIsAllowed(t *testing.T) {
+	// Build an OR chain whose node count lands just at/under maxParseNodes: with
+	// R refs the node count is 2R-1, so R = (maxParseNodes+1)/2 hits the cap
+	// exactly when maxParseNodes is odd and lands just under it when even —
+	// either way it must parse without error.
+	refs := (maxParseNodes + 1) / 2
+	_, err := Parse(orChain(refs))
+	require.NoError(t, err)
+}
+
 func TestNestingAtLimitIsAllowed(t *testing.T) {
 	// A policy nested exactly at maxParseDepth must succeed.
 	policy := ""

token/services/identity/boolpolicy/sig.go

@@ -81,31 +81,62 @@ func (v *PolicyVerifier) Verify(msg, sigBytes []byte) error {
 		return errors.Errorf("policy signature has [%d] slots, expected [%d]",
 			len(sig.Signatures), len(v.Verifiers))
 	}
-	if !v.evalNode(v.Policy, msg, sig.Signatures) {
+	// memo caches each index's verification result for the duration of this
+	// Verify call: a $N reference is checked against the same (msg, sigs[N])
+	// no matter how many times it appears in the policy, so the potentially
+	// expensive Verifiers[N].Verify is invoked at most once per index.
+	memo := make([]refResult, len(sig.Signatures))
+	if !v.evalNode(v.Policy, msg, sig.Signatures, memo) {
 		return errors.New("policy not satisfied")
 	}
 
 	return nil
 }
 
+// refResult is the memoised outcome of verifying a single component index.
+type refResult int8
+
+const (
+	refUnknown refResult = iota // not yet verified in this Verify call
+	refPass                     // verification succeeded
+	refFail                     // verification failed (absent slot or bad signature)
+)
+
 // evalNode recursively evaluates the policy AST against the provided signatures.
-func (v *PolicyVerifier) evalNode(node Node, msg []byte, sigs [][]byte) bool {
+// memo is indexed by component index ($N) and caches per-index verification
+// results across the whole traversal; it must have one entry per signature slot.
+func (v *PolicyVerifier) evalNode(node Node, msg []byte, sigs [][]byte, memo []refResult) bool {
 	switch n := node.(type) {
 	case *RefNode:
-		i := n.Index
-		if i < 0 || i >= len(sigs) || len(sigs[i]) == 0 {
-			return false
-		}
-
-		return v.Verifiers[i].Verify(msg, sigs[i]) == nil
+		return v.evalRef(n.Index, msg, sigs, memo)
 
 	case *AndNode:
-		return v.evalNode(n.Left, msg, sigs) && v.evalNode(n.Right, msg, sigs)
+		return v.evalNode(n.Left, msg, sigs, memo) && v.evalNode(n.Right, msg, sigs, memo)
 
 	case *OrNode:
-		return v.evalNode(n.Left, msg, sigs) || v.evalNode(n.Right, msg, sigs)
+		return v.evalNode(n.Left, msg, sigs, memo) || v.evalNode(n.Right, msg, sigs, memo)
 
 	default:
 		return false
 	}
 }
+
+// evalRef verifies the component identity at index i, reusing a previously
+// computed result for the same index when one is available.
+func (v *PolicyVerifier) evalRef(i int, msg []byte, sigs [][]byte, memo []refResult) bool {
+	if i < 0 || i >= len(sigs) || len(sigs[i]) == 0 {
+		return false
+	}
+	if memo[i] != refUnknown {
+		return memo[i] == refPass
+	}
+
+	ok := v.Verifiers[i].Verify(msg, sigs[i]) == nil
+	if ok {
+		memo[i] = refPass
+	} else {
+		memo[i] = refFail
+	}
+
+	return ok
+}