fix(signature): validate pubkey length and prefix byte

[oritwoen]

validate_pubkey_hex only checked for hex characters, so "ff", "deadbeef", or any hex string passed validation. Downstream, verify_key_matches_pubkey compares against properly encoded EC points and never matches, so the tool reports vulnerabilities as "unrecoverable" with no hint that the input pubkey is malformed.

Now rejects early with a clear message if the pubkey isn't 66 chars (compressed, 02/03 prefix) or 130 chars (uncompressed, 04 prefix).

Closes #22

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 91de75fd-3bd8-4b17-b556-8a02a1e80da0

📥 Commits

Reviewing files that changed from the base of the PR and between 4197b9b and 285f2f5.

📒 Files selected for processing (2)

• src/attack/polynonce.rs
• src/signature.rs

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: cubic · AI code reviewer
• GitHub Check: test

🧰 Additional context used

📓 Path-based instructions (4)

src/attack/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Implement trait Attack when adding a new attack type for ECDSA vulnerability analysis

Files:

• src/attack/polynonce.rs

src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.rs: Import PrimeField trait from k256 when using Scalar::from_repr()
Never suppress type errors using as any or equivalent type casts

Files:

• src/attack/polynonce.rs
• src/signature.rs

src/signature.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/signature.rs: Use SignatureInput struct for IO/serialization and Signature struct for domain models in two-layer architecture
Normalize public keys to lowercase and strip 0x prefix before grouping by r and pubkey
Set confidence field to 1.0 when public key is known, 0.8 when public key is None

Files:

• src/signature.rs

src/{provider,signature}.rs

📄 CodeRabbit inference engine (AGENTS.md)

Represent all r/s/z values as decimal strings in JSON/CSV input formats

Files:

• src/signature.rs

🧠 Learnings (13)

📓 Common learnings

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to tests/**/*.rs : Verify recovered keys mathematically through signature round-trip validation

Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:59.187Z
Learning: In the vusi project (src/attack/polynonce.rs), ECDSA signatures are validated at the signature parsing layer before reaching attack implementations, so `s=0` checks are not needed in the polynonce attack code.

📚 Learning: 2026-01-15T17:42:46.869Z

Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:46.869Z
Learning: In src/attack/polynonce.rs, rely on the upstream ECDSA validation performed in the signature parsing layer. Do not add s=0 checks in the polynonce attack code, since degenerate signatures are rejected before reaching this module. Treat signatures as already validated regarding s bounds; add tests that reflect this assumption and avoid duplicating signature validity checks in this file.

Applied to files:

• src/attack/polynonce.rs

📚 Learning: 2026-01-15T17:42:59.187Z

Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:59.187Z
Learning: The polynonce attack implementation in vusi (src/attack/polynonce.rs) uses exactly 4 signatures for the linear case (degree=1) elimination polynomial construction. The `.take(4)` behavior is intentional and documented.

Applied to files:

• src/attack/polynonce.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to tests/**/*.rs : Verify recovered keys mathematically through signature round-trip validation

Applied to files:

• src/attack/polynonce.rs
• src/signature.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/signature.rs : Normalize public keys to lowercase and strip `0x` prefix before grouping by r and pubkey

Applied to files:

• src/attack/polynonce.rs
• src/signature.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/attack/*.rs : Implement `trait Attack` when adding a new attack type for ECDSA vulnerability analysis

Applied to files:

• src/attack/polynonce.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/signature.rs : Use `SignatureInput` struct for IO/serialization and `Signature` struct for domain models in two-layer architecture

Applied to files:

• src/attack/polynonce.rs
• src/signature.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/signature.rs : Set `confidence` field to 1.0 when public key is known, 0.8 when public key is None

Applied to files:

• src/attack/polynonce.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/math.rs : Use `parse_scalar_decimal_strict` for decimal to Scalar conversion with strict validation (no leading zeros, no values ≥ secp256k1 order n)

Applied to files:

• src/signature.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/provider.rs : Do not parse raw transactions; input must be pre-parsed r/s/z signature components

Applied to files:

• src/signature.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/{provider,signature}.rs : Represent all r/s/z values as decimal strings in JSON/CSV input formats

Applied to files:

• src/signature.rs

📚 Learning: 2026-01-15T18:08:47.413Z

Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to tests/**/*.rs : Use test vectors from real Bitcoin transactions for integration tests

Applied to files:

• src/signature.rs

📚 Learning: 2026-01-15T17:42:59.187Z

Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:59.187Z
Learning: In the vusi project (src/attack/polynonce.rs), ECDSA signatures are validated at the signature parsing layer before reaching attack implementations, so `s=0` checks are not needed in the polynonce attack code.

Applied to files:

• src/signature.rs

🧬 Code graph analysis (1)

src/signature.rs (1)

src/attack/biased_nonce.rs (1)

• make_sig (527-537)

🔇 Additional comments (4)

src/attack/polynonce.rs (1)

446-449: Test fixtures now match the parser contract.

Good change. Using valid compressed pubkeys here keeps these tests focused on polynonce behavior instead of failing in SignatureInput validation first.

Also applies to: 472-472, 487-487, 525-527, 557-557

src/signature.rs (3)

97-120: Length + prefix checks close the malformed-hex acceptance path.

This now fails early for bad pubkeys with explicit messages, which is the right fix for the downstream “unrecoverable” false path.

---

194-197: Updated test pubkeys are consistent with normalized secp256k1 encoding.

Using valid 66-char compressed keys in these tests removes fixture noise and keeps grouping/normalization assertions targeted.

Also applies to: 221-221, 229-229, 277-277, 285-285, 303-305, 313-313, 332-333, 341-341, 432-436, 441-441, 460-460, 478-480

---

492-534: Validator branch coverage is solid for this change.

These tests hit all three important outcomes: invalid length, invalid compressed prefix, and valid uncompressed key.

---

📝 Walkthrough

Walkthrough

Added stricter validation to validate_pubkey_hex to reject pubkeys that don't match secp256k1 standards: compressed keys must be 66 hex chars with 02/03 prefix, uncompressed must be 130 chars with 04 prefix. Updated test data across both files to use valid test constants.

Changes

Cohort / File(s)	Summary
Pubkey Validation Enhancement src/signature.rs	Added length and prefix validation to validate_pubkey_hex. Now rejects short hex strings like "ff" or "deadbeef" with explicit error messages instead of silently passing invalid keys downstream. Added test cases for short keys, wrong prefix (compressed), and valid uncompressed keys (04-prefixed).
Test Constants & Data src/signature.rs, src/attack/polynonce.rs	Introduced test pubkey constants (TEST_PK_A, TEST_PK_B in signature.rs; TEST_PK_C, TEST_PK_D, TEST_PK_E in polynonce.rs) using valid-length hex strings. Replaced hardcoded placeholders ("02abcdef", "02aaaaaa") throughout tests to use these constants for consistency and correctness.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat: polynonce attack detection #9 — Updates test pubkeys in signature.rs and polynonce.rs, directly overlapping with the test constant changes in this PR.
• feat: biased nonce attack via HNP lattice reduction #10 — Modifies signature.rs struct fields (kp field on SignatureInput) and updates test scaffolding across the same files.

Poem

🔐 Hex strings no longer slip through the gate,
Sixty-six or one-thirty—must validate.
Prefixes checked, false negatives defeated,
Invalid keys caught before they're cheated.
Early errors beat silent lies.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title directly matches the main change: stricter pubkey validation now enforces length and prefix byte requirements, which aligns with the fix description.
Description check	✅ Passed	Description clearly explains the problem (hex-only validation too loose), the impact (silent downstream failures), and the fix (length + prefix checks). Closes #22.
Linked Issues check	✅ Passed	Changes fully address #22 requirements: rejects pubkeys that aren't 66 chars (02/03 prefix) or 130 chars (04 prefix), validates prefix bytes, and fails early with clear error messages.
Out of Scope Changes check	✅ Passed	Changes are scoped to validation logic in signature.rs and test updates in both files. Test constant additions (TEST_PK_A, TEST_PK_B, etc.) support the validation changes and don't introduce unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/pubkey-length-validation

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/pubkey-length-validation

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

_{Requires human review: This PR modifies input validation logic for public keys, which is classified as a high-impact change requiring human review per the provided guidelines.}

[cubic-dev-ai]

1 issue found across 2 files

Confidence score: 3/5

• There is a concrete validation gap in src/signature.rs: SEC1 parsing only checks length/prefix, so invalid curve points are accepted instead of being rejected at input validation time.
• Because this is severity 6/10 with high confidence (9/10), it introduces moderate regression risk around error handling and can surface misleading "unrecoverable" failures to users.
• This looks fixable and contained to input validation logic, but it is user-facing enough to warrant caution before merge.
• Pay close attention to src/signature.rs - tighten SEC1 point validation so malformed points are classified as bad input, not unrecoverable errors.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/signature.rs">

<violation number="1" location="src/signature.rs:97">
P2: This only checks SEC1 length and prefix, so invalid curve points still pass and later get reported as "unrecoverable" instead of bad input.</violation>
</file>

Architecture diagram

sequenceDiagram
    participant Client as SignatureInput
    participant Sig as Signature::try_from()
    participant Val as validate_pubkey_hex()
    participant Attack as Attack Engine
    participant Downstream as verify_key_matches_pubkey

    Note over Client,Val: Validation Phase

    Client->>Sig: Convert raw input to Signature
    Sig->>Val: NEW: validate_pubkey_hex(pubkey)
    
    Val->>Val: Verify ASCII hex characters
    
    alt Length is 66 (Compressed)
        Val->>Val: NEW: Check for 02 or 03 prefix
    else Length is 130 (Uncompressed)
        Val->>Val: NEW: Check for 04 prefix
    else Invalid Length
        Val-->>Sig: CHANGED: bail! (Invalid length error)
    end

    alt Prefix mismatch
        Val-->>Sig: CHANGED: bail! (Invalid prefix error)
    else Success
        Val-->>Sig: Ok(())
    end

    Note over Sig,Downstream: Execution Phase

    alt Validation Failed
        Sig-->>Client: Return Error (Early Exit)
    else Validation Passed
        Sig-->>Client: Return Signature
        Client->>Attack: detect(signatures)
        Attack->>Downstream: verify_key_matches_pubkey()
        Note right of Downstream: Downstream logic now receives<br/>guaranteed valid EC point formats.
        Downstream-->>Attack: Match/No Match
    end

Loading

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: This only checks SEC1 length and prefix, so invalid curve points still pass and later get reported as "unrecoverable" instead of bad input.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/signature.rs, line 97:

<comment>This only checks SEC1 length and prefix, so invalid curve points still pass and later get reported as "unrecoverable" instead of bad input.</comment>

<file context>
@@ -93,6 +93,32 @@ fn validate_pubkey_hex(pubkey: &str) -> Result<()> {
         anyhow::bail!("Invalid pubkey: must be hexadecimal");
     }
+
+    match pubkey.len() {
+        66 => {
+            if !pubkey.starts_with("02") && !pubkey.starts_with("03") {
</file context>