Skip to content

Document signing secret-key validity precondition#12

Open
fegge wants to merge 2 commits into
mainfrom
fix/sign-accepts-invalid-secret-keys
Open

Document signing secret-key validity precondition#12
fegge wants to merge 2 commits into
mainfrom
fix/sign-accepts-invalid-secret-keys

Conversation

@fegge

@fegge fegge commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Document that signing APIs require a valid serialized ML-DSA secret key
  • Point callers that import serialized keys to pk_from_sk for validation
  • Add the maintainer rationale for skipping InvalidPrivateKey signing vectors in the Wycheproof client

Validation

  • clang-format -i mldsa/mldsa_native.h mldsa/src/sign.h
  • ruff format test/wycheproof/wycheproof_client.py
  • python3 -m py_compile test/wycheproof/wycheproof_client.py
  • ruff check test/wycheproof/wycheproof_client.py
  • git diff --check
  • make run_func -j4
  • python3 test/wycheproof/wycheproof_client.py --data-dir /tmp/ptp-wycheproof-sign-docs

Note: make run_wycheproof against the default local cache failed because test/wycheproof/.wycheproof-data already contains the newer randomized signing vectors covered by a separate finding. The fresh data-dir run above validates this branch against its pinned vectors. Full repo scripts/format and scripts/lint were not run because this environment lacks shfmt and nixpkgs-fmt.

hanno-becker and others added 2 commits June 26, 2026 15:15
Verify that each assembly kernel preserves the callee-saved registers
its platform calling convention requires:

- AArch64 (AAPCS64):   x19-x28, x29/FP, lower 64 bits of d8-d15.
- x86_64 (System V):   rbx, rbp, r12-r15. No SIMD register callee-saved.
- Armv8.1-M (AAPCS32): r4-r11 plus MVE Q4-Q7 (= D8-D15).

A per-arch assembly call stub loads a random register state, calls the
kernel, and captures the result; a checker then confirms the callee-saved
set is intact. Pointer arguments are backed by correctly-sized buffers
whose layout comes from a per-kernel YAML block; scripts/autogen turns
that YAML into the per-kernel checks. A self-test of hand-written
corrupters confirms the checker actually fires before kernel verdicts
are trusted.

Run via `make run_abicheck OPT=1` or `scripts/tests abicheck`. It needs no
library build, so `scripts/tests all` runs it by default.

Ported from mlkem-native, with the following deviations:

- No PowerPC64/ELFv2 backend, which mldsa-native does not ship.
- Fresh ABI YAML for the ML-DSA kernel set and signatures.
- Fixed the FIPS202 Keccak YAML Name fields to match the exported
  symbols (e.g. keccak_f1600_x1_scalar_aarch64_asm, not
  keccak_f1600_x1_scalar_asm), which the generator maps to mld_<Name>.
- The build force-undefines MLD_CONFIG_NO_{KEYPAIR,SIGN,VERIFY}_API so
  every kernel is present; many ML-DSA kernels are otherwise gated out
  by reduced-API configs, unlike any mlkem kernel.
- Adds MLD_SYS_CAP_MVE to sys.h (mlkem already has MLK_SYS_CAP_MVE), as
  the Armv8.1-M check needs it for its runtime capability gate.

AArch64 ABI YAML lives in dev/aarch64_clean/src/, not dev/aarch64_opt/src/,
which scripts/autogen regenerates from clean.

See test/abicheck/README.md for details.

Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ fegge
❌ hanno-becker
You have signed the CLA already but the status is still pending? Let us recheck it.

@fegge

fegge commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Upstream issue: pq-code-package#1216
Upstream PR: pq-code-package#1221

@fegge

fegge commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Replacement upstream PR with DCO-signed commits from an upstream repository branch: pq-code-package#1229

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.

3 participants