Skip to content

fix(crypto): add safety check for odd lengths in vector_karatsuba - #3557

Closed
kimyee1 wants to merge 2 commits into
0xMiden:nextfrom
kimyee1:fix/karatsuba-safety-check
Closed

fix(crypto): add safety check for odd lengths in vector_karatsuba#3557
kimyee1 wants to merge 2 commits into
0xMiden:nextfrom
kimyee1:fix/karatsuba-safety-check

Conversation

@kimyee1

@kimyee1 kimyee1 commented Aug 9, 2026

Copy link
Copy Markdown

Rationale

The vector_karatsuba function is only used for Falcon signature generation, which always operates on power-of-two lengths (e.g., 512). However, the function is accessible, and calling it with odd lengths (like 9 or 17) currently causes a panic due to out-of-bounds indexing or silent math errors.

Following the maintainer's recommendation in #3550 to "make the power-of-two precondition explicit," this PR adds a safety check to fail fast with a clear error message.

What I changed

  • Added assert!(n.is_power_of_two()) at the entry of vector_karatsuba.
  • This prevents silent math errors or confusing index panics for unsupported lengths.
  • Added a regression test karatsuba_panics_on_non_power_of_two to prove the guard works.

Test plan

  • Ran cargo test -p miden-crypto locally.
  • Verified that the new test correctly catches the odd-length case and successfully panics.

Fixes #3550

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

Automated check (CONTRIBUTING.md)

Findings:

Next steps:

  • Link a relevant issue (e.g., "Fixes Implement SHA256 in Miden Assembly #123") and ensure it is assigned to you.
  • See CONTRIBUTING.md for expectations.
  • If this is a false positive, comment: /quality-review.

@kimyee1
kimyee1 force-pushed the fix/karatsuba-safety-check branch from 93640f3 to 199e56d Compare August 9, 2026 22:31
@kimyee1

kimyee1 commented Aug 10, 2026

Copy link
Copy Markdown
Author

All checks are now green after applying the nightly rustfmt formatting. 🟢

Quick recap: vector_karatsuba previously assumed power-of-two lengths, so odd-length inputs could panic or silently corrupt results. This PR makes the precondition explicit with an assertion, and adds a #[should_panic] regression test to prove the guard catches it.

Happy to adjust the error message or test placement if you prefer a different shape.

cc @huitseeker

@Al-Kindi-0

Copy link
Copy Markdown
Contributor

We should close in favor of #3629, which resolves #3487 with a complete documentation, testing, and hardening pass over the Falcon math module, covering the change proposed here.

@bobbinth bobbinth closed this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants