Repository navigation
Conversation
relic takes both lengths as an int, and md_xmd validates only the output and tag lengths, never the message length. A message at or beyond 2 GiB therefore narrowed negative, slipped every check and was widened back to an enormous unsigned length inside SHA256Input, reading far past the end of the buffer. The Python binds guard their two from_message entry points, but sign, verify and aggregate_verify reach the same narrowing by other routes, as do the Rust and Go binds, whose shims pass a caller-supplied length straight through to Sign and Verify. Guard at the three points where the value actually narrows instead of at each caller: * G1Element::FromMessage (src/elements.cpp) * G2Element::FromMessage (src/elements.cpp, non-legacy path only) * PrivateKey::SignG2 (src/privatekey.cpp, message and tag lengths) Dash Core executes these paths but cannot reach them with an unsafe length: every call site in src/bls/ wraps a uint256, so the length is always 32. This is hardening for the library's public API, not a fix for a reachable defect in Dash Core. The legacy paths hash a fixed-length digest and are unaffected. The guards run before the pointer is dereferenced, so the tests can describe an oversized buffer without allocating one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit 88e1261) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The direct C++ guards correctly reject message and tag lengths that cannot be represented by relic's signed length parameters. However, the newly thrown exceptions are not contained by the Rust and Go C ABI shims, so oversized binding inputs terminate the process instead of being safely rejected; some scheme paths also allocate and copy oversized messages before reaching the guards.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/privatekey.cpp`:
- [BLOCKING] src/privatekey.cpp:314-316: Contain oversized-length exceptions at the Rust and Go ABI boundaries
These new guards throw `std::invalid_argument`, and the analogous guard in `G2Element::FromMessage` is reached by verification. The Rust shims in `rust-bindings/bls-dash-sys/c-bindings/schemes.cpp` and the Go shims in `go-bindings/schemes.cpp` invoke Sign, Verify, AggregateVerify, and FastAggregateVerify without `try`/`catch` or an error result. A message longer than `INT_MAX` therefore lets a C++ exception escape an `extern "C"` call, which aborts across Rust's non-unwind ABI and terminates through cgo rather than safely rejecting the input. The PR explicitly identifies these bindings as affected callers, so every affected shim must contain the exception and propagate a result the safe wrappers can handle, with Rust and Go binding tests covering oversized lengths.
In `src/schemes.cpp`:
- [SUGGESTION] src/schemes.cpp:457-460: Validate final hash-input lengths before scheme preprocessing
The sink-level guards are not the first operation on every route the PR says is protected. AugSchemeMPL Sign, Verify, and AggregateVerify reserve and copy each complete message into augmented vectors before CoreMPL reaches `SignG2` or `FromMessage`; BasicSchemeMPL aggregate verification similarly copies messages into a set before delegating. An oversized caller-controlled message can therefore force another multi-gigabyte allocation and copy before it is rejected, while the augmented prefix can also make an individually in-range message exceed `INT_MAX`. Validate every final hash-input length using checked prefix-plus-message arithmetic before any reserve or copy, and prevalidate all aggregate messages before constructing the set or augmented vectors.
| if (len > (size_t)std::numeric_limits<int>::max() || | ||
| dst_len > (size_t)std::numeric_limits<int>::max()) { | ||
| throw std::invalid_argument("PrivateKey::SignG2: length exceeds INT_MAX"); |
There was a problem hiding this comment.
🔴 Blocking: Contain oversized-length exceptions at the Rust and Go ABI boundaries
These new guards throw std::invalid_argument, and the analogous guard in G2Element::FromMessage is reached by verification. The Rust shims in rust-bindings/bls-dash-sys/c-bindings/schemes.cpp and the Go shims in go-bindings/schemes.cpp invoke Sign, Verify, AggregateVerify, and FastAggregateVerify without try/catch or an error result. A message longer than INT_MAX therefore lets a C++ exception escape an extern "C" call, which aborts across Rust's non-unwind ABI and terminates through cgo rather than safely rejecting the input. The PR explicitly identifies these bindings as affected callers, so every affected shim must contain the exception and propagate a result the safe wrappers can handle, with Rust and Go binding tests covering oversized lengths.
source: ['codex']
|
Superseded by #132 |
relic takes both lengths as an int, and md_xmd validates only the output and tag lengths, never the message length. A message at or beyond 2 GiB therefore narrowed negative, slipped every check and was widened back to an enormous unsigned length inside SHA256Input, reading far past the end of the buffer.
The Python binds guard their two from_message entry points, but sign, verify and aggregate_verify reach the same narrowing by other routes, as do the Rust and Go binds, whose shims pass a caller-supplied length straight through to Sign and Verify. Guard at the three points where the value actually narrows instead of at each caller:
Dash Core executes these paths but cannot reach them with an unsafe length: every call site in src/bls/ wraps a uint256, so the length is always 32. This is hardening for the library's public API, not a fix for a reachable defect in Dash Core.
The legacy paths hash a fixed-length digest and are unaffected.
The guards run before the pointer is dereferenced, so the tests can describe an oversized buffer without allocating one.