Skip to content

vdaf/prio3: reject degenerate parameters - #685

Merged
bwesterb merged 1 commit into
mainfrom
bas/zk26
Aug 5, 2026
Merged

vdaf/prio3: reject degenerate parameters#685
bwesterb merged 1 commit into
mainfrom
bas/zk26

Conversation

@bwesterb

@bwesterb bwesterb commented Jul 31, 2026

Copy link
Copy Markdown
Member

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Submitting the highest bucket number crashes histogram aggregation

A bucket number equal to the number of buckets is accepted instead of rejected (measurement > uint64(h.length) at vdaf/prio3/histogram/histogram.go:126) before the value is used to fill in the bucket list, so a caller can crash the program with an out-of-range access.
Impact: Any client or server call using the last-plus-one bucket value panics instead of getting a clean error, aborting the process.

Off-by-one bound in histogram measurement encoding

Encode allocates a vector of exactly h.Valid.MeasurementLen == length elements (vdaf/prio3/histogram/histogram.go:130) and then writes out[measurement]. Since the guard only rejects measurement > length, passing measurement == length (e.g. New(2, 4, 3, ctx) then Shard(4, ...)) indexes past the end and panics with "index out of range". Per the Prio3Histogram spec the measurement must satisfy 0 <= measurement < length, so the comparison should be >=. This is in the same input-validation area the PR hardens, and REVIEW.md requires no panic on attacker-controlled input plus max-value edge-case coverage.

(Refers to lines 126-128)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟨 Histogram bucket index at the upper bound triggers a panic instead of an error

vdaf/prio3/histogram/histogram.go:126 accepts measurement == length and then writes out[measurement] into a vector of exactly length elements (vdaf/prio3/histogram/histogram.go:130-131), causing an out-of-range panic on caller-supplied input. REVIEW.md/AGENTS.md require inputs to be validated at API boundaries and forbid panics on attacker-controlled input.

(Refers to lines 126-128)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@bwesterb
bwesterb merged commit 76cf862 into main Aug 5, 2026
14 checks passed
@bwesterb
bwesterb deleted the bas/zk26 branch August 5, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants