Fix P-521 ECDSA verification and add curve coverage tests#1956
Conversation
hashForCurve matched "P-512" instead of "P-521", making the P-521 path unreachable. verifyECDSASignature used hash length (64 for SHA-512) to determine signature component size, but P-521 COSE signatures use 66-byte components (ceil(521/8)). Derive component size from the curve order instead. Add verify_test.go with sign/verify roundtrips for all four NIST curves, rejection tests (wrong key, wrong payload, tampered sig, wrong length, DER format), and an explicit test proving P-521 key size != hash size.
|
👋 nadahalli, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
There was a problem hiding this comment.
Pull request overview
This PR fixes ECDSA COSE signature verification for P-521 keys in the Nitro attestation verifier by correctly selecting the P-521 hash path and by sizing COSE signature components based on curve bit size (r||s component width), not hash digest length. It also adds a dedicated test suite to prevent regressions across supported NIST curves.
Changes:
- Fix
hashForCurveto match"P-521"(previously"P-512"), making the P-521 branch reachable. - Fix
verifyECDSASignatureto split COSE signatures usingceil(curveBitSize/8)component sizing (notlen(hash)), rejecting incorrect signature lengths early. - Add
verify_test.gowith roundtrip and negative tests across P-224/P-256/P-384/P-521 plus a targeted P-521 regression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/teeattestation/nitro/verify.go | Corrects curve/hash selection for P-521 and fixes COSE ECDSA signature component sizing based on curve bit size. |
| pkg/teeattestation/nitro/verify_test.go | Adds comprehensive curve coverage and regression tests for COSE ECDSA verification, including P-521 sizing edge case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| r, s, err := ecdsa.Sign(rand.Reader, key, hash) | ||
| require.NoError(t, err) | ||
|
|
||
| keySize := (key.Curve.Params().BitSize + 7) / 8 |
There was a problem hiding this comment.
coseSign re-computes the COSE component size with (key.Curve.Params().BitSize + 7) / 8, duplicating the production helper curveKeySize. To avoid divergence if sizing logic changes again, consider calling curveKeySize(&key.PublicKey) here (or factoring a shared helper used by both prod + tests).
| keySize := (key.Curve.Params().BitSize + 7) / 8 | |
| keySize := curveKeySize(&key.PublicKey) |
| // marshalDER produces a minimal ASN.1 DER encoding of an ECDSA signature. | ||
| func marshalDER(r, s *big.Int) []byte { | ||
| encodeInt := func(v *big.Int) []byte { | ||
| b := v.Bytes() | ||
| if len(b) > 0 && b[0]&0x80 != 0 { | ||
| b = append([]byte{0x00}, b...) | ||
| } | ||
| return append([]byte{0x02, byte(len(b))}, b...) |
There was a problem hiding this comment.
The test builds a DER-encoded signature via a custom marshalDER implementation. Since the Go stdlib already supports DER ECDSA signatures (e.g., ecdsa.SignASN1 / asn1.Marshal), using that would simplify the test and reduce the chance of subtle DER encoding bugs (length handling, INTEGER=0 edge cases, etc.).
Verify the ECDSA signature pipeline (CBOR parse, Sig_structure build, verifyECDSASignature) against official test vectors from the COSE Working Group Examples repository (normative suite for RFC 9052/9053): - ES256: RFC 8152 Appendix C.2.1 (sign-pass-03) - ES384: ecdsa-sig-02 - ES512: ecdsa-sig-03 (exercises the P-521 fix from previous commit) - Tampered payload: sign-fail-02 - Modified protected header: sign-fail-06 - Wrong key: valid ES256 vector verified with P-384 key
| // curveKeySize returns the byte length of one ECDSA signature component | ||
| // (r or s) for the given curve: ceil(bitSize / 8). This is the correct |
There was a problem hiding this comment.
super nit: maybe do float division -> ciel -> cast to int in case this helper is ever required for other curves with odd bit sizes
There was a problem hiding this comment.
(n + 7) / 8 with integer division is the standard idiom for ceil(n / 8).
There was a problem hiding this comment.
yeah nvm, stupid suggestion.
Summary
hashForCurvematching"P-512"instead of"P-521", which made the P-521 code path unreachableverifyECDSASignatureusing hash length to size signature components; P-521 COSE signatures use 66-byte components (ceil(521/8)), not 64 (SHA-512 digest size). Component size is now derived from the curve order viacurveKeySizeverify_test.gowith sign/verify roundtrips for all four NIST curves (P-224, P-256, P-384, P-521), negative tests (wrong key, wrong payload, tampered signature, wrong length, DER encoding rejected), and a targeted regression test proving P-521 key size != hash size