Skip to content

Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support#3286

Open
sfarestam-iproov wants to merge 11 commits into
aws:mainfrom
sfarestam-iproov:add-brainpool-curves
Open

Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support#3286
sfarestam-iproov wants to merge 11 commits into
aws:mainfrom
sfarestam-iproov:add-brainpool-curves

Conversation

@sfarestam-iproov

Copy link
Copy Markdown

Issues:

Resolves #2939

Description of changes:

AWS-LC currently does not support Brainpool elliptic curves. This PR adds EC group support for the five Brainpool r1 prime curves defined in RFC 5639: brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, and brainpoolP512r1.

The implementation follows the same pattern as secp256k1 — generic Montgomery arithmetic via EC_GFp_mont_method(), with no hand-optimized assembly. The only structural addition is ec_group_set_a_mont() for setting an arbitrary Montgomery-form a coefficient, since Brainpool curves have a ≠ -3 (unlike NIST curves) and a ≠ 0 (unlike secp256k1). The existing ec_GFp_mont_dbl() already handles this case correctly via its a_is_minus3 == 0 code path.

NIDs (925, 927, 929, 931, 933) and OIDs were already registered in nid.h and obj_dat.h. All domain parameters are sourced from RFC 5639 Sections 3.3–3.7, with OIDs from Section 4.1.

This PR covers EC primitive support (key generation, ECDSA, ECDH). TLS negotiation support (RFC 7027 / RFC 8734) can follow in a separate PR.

Motivation: These curves are required by ICAO Doc 9303 (ePassport standard, 30+ countries), BSI TR-03116-4 (German federal regulation), and are blocking the pyca/cryptography project from accepting Brainpool improvements (pyca/cryptography#14905).

Call-outs:

  • FIPS boundary: The code is in crypto/fipsmodule/ec/. If adding curves inside the FIPS module is a concern for recertification, the implementation can be moved to crypto/ec_extra/. Please advise on the preferred placement.
  • Arbitrary a coefficient: Added a small helper ec_group_set_a_mont() (6 lines) to copy a Montgomery-form a value. This is the only new function beyond the existing curve template pattern.
  • make_tables.go changes: Added a curveWithA wrapper type and writeCurveDataWithA() function to emit MontA constants for curves where a is not -3 or 0. The standard elliptic.CurveParams in Go doesn't carry an A field (it assumes a = -3).

Testing:

  • BrainpoolKeygenSignVerify: generates a key, signs a digest with ECDSA, verifies the signature, and checks that a corrupted signature is rejected — for all 5 curves.
  • ECPKParmatersBio: round-trip i2d_ECPKParameters_bio / d2i_ECPKParameters_bio for all 5 curves.
  • GetNamedCurve: dispatches Brainpool curve names for file-based test vectors.
  • Scalar base multiplication test vectors are not included in this PR (generating them requires a Go Brainpool library); they can be added as a follow-up.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

// Sign a digest.
std::vector<uint8_t> sig(ECDSA_size(key.get()));
unsigned sig_len;
ASSERT_TRUE(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'sig_len' is not initialized [cppcoreguidelines-init-variables]

crypto/fipsmodule/ec/ec_test.cc:2829:

- len;
+ len = 0;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — initialized to 0. Thanks.

…1, brainpoolP512r1 EC group support

Add all five Brainpool r1 prime curves from RFC 5639 using the generic
Montgomery arithmetic path (EC_GFp_mont_method), following the same
pattern as secp256k1. No hand-optimized assembly is needed.

The only structural addition beyond the secp256k1 template is
ec_group_set_a_mont() for setting an arbitrary Montgomery-form a
coefficient (Brainpool curves have a != -3 and a != 0). The existing
ec_GFp_mont_dbl() already handles this case correctly.

NIDs were already registered in nid.h. This change adds the EC_GROUP
definitions, the precomputed Montgomery constants (via make_tables.go),
the NID-to-group switch cases, and the public API functions.

Domain parameters sourced from:
 - brainpoolP224r1: RFC 5639, Section 3.3
 - brainpoolP256r1: RFC 5639, Section 3.4
 - brainpoolP320r1: RFC 5639, Section 3.5
 - brainpoolP384r1: RFC 5639, Section 3.6
 - brainpoolP512r1: RFC 5639, Section 3.7
OIDs from: RFC 5639, Section 4.1

These curves are needed for:
- ICAO 9303 ePassport certificate processing (30+ countries)
- BSI TR-03116-4 compliance (German federal regulation)
- pyca/cryptography Brainpool support (blocked on aws-lc, see aws#2939)

Resolves aws#2939 (EC primitive support; TLS negotiation can follow separately)
The kAllGroups array in ec_asn1.c is used by EC_KEY_parse_curve_name
and EC_get_builtin_curves to look up curves by OID. Without the
Brainpool entries, i2d/d2i_ECPKParameters_bio round-trips fail with
EC_R_UNKNOWN_GROUP.
@codecov-commenter

codecov-commenter commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.89617% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.20%. Comparing base (e8ea407) to head (df161b2).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crypto/fipsmodule/ec/ec_test.cc 78.72% 10 Missing ⚠️
crypto/fipsmodule/evp/evp.c 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3286      +/-   ##
==========================================
+ Coverage   78.18%   78.20%   +0.02%     
==========================================
  Files         693      694       +1     
  Lines      123893   124075     +182     
  Branches    17205    17215      +10     
==========================================
+ Hits        96861    97030     +169     
- Misses      26114    26127      +13     
  Partials      918      918              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The EVP_PKEY_{get,set}1_tls_encodedpoint functions had a hardcoded
allowlist of only four NIST curves. The underlying point encoding
(EC_POINT_oct2point, EC_KEY_key2buf) is curve-agnostic, so this
restriction was unnecessarily narrow. Add all remaining built-in
curves (secp256k1 and the five Brainpool curves) to fix strongswan
ECDH interoperability.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov

Copy link
Copy Markdown
Author

The strongswan-x86_64 CI failure is caused by our changes and is now fixed in 43d698e.

Root cause: evp_pkey_tls_encodedpoint_ec_curve_supported() in crypto/fipsmodule/evp/evp.c had a hardcoded allowlist of only four NIST curves (P-224, P-256, P-384, P-521). strongswan's openssl plugin uses EVP_PKEY_get1_tls_encodedpoint() / EVP_PKEY_set1_tls_encodedpoint() for its DH key exchange implementation. Before this PR, EC_GROUP_new_by_curve_name(NID_brainpoolP256r1) returned NULL so strongswan skipped the test vector. With Brainpool support now present, strongswan detects the curve and attempts an ECDH exchange, but the public key export/import fails at the allowlist.

Fix: Added NID_secp256k1 and all five Brainpool NIDs to the allowlist. The underlying point encoding (EC_POINT_oct2point, EC_KEY_key2buf) is curve-agnostic — the restriction was just overly conservative. The actual ECDH computation path (EVP_PKEY_derive) has no curve restrictions.

Note: the clang (Windows) failure is unrelated — it's a flaky ALPS-Decline-Server-Old-DTLS-TLS12 test failing due to Windows socket exhaustion (system lacked sufficient buffer space).

@nebeid

nebeid commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Thank you for widening the curve allowlist in evp_pkey_tls_encodedpoint_ec_curve_supported

Suggestion: add a direct unit test so this gate is guarded explicitly rather than incidentally. The existing ECTLSEncodedPoint test already does hardcoded per-curve set1/get1 round-trips for the NIST curves; extending it to cover secp256k1 and at least one Brainpool curve would pin the allowlist and prevent future drift.

Extend the existing EVP_PKEY_{get,set}1_tls_encodedpoint round-trip
test to cover secp256k1 (Wycheproof vector) and brainpoolP256r1
(RFC 7027 Section A.1), pinning the curve allowlist added in the
previous commit and exercising ECDH derivation for both curves.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov

Copy link
Copy Markdown
Author

Done in 18fe067 — extended the ECTLSEncodedPoint test to cover secp256k1 (Wycheproof ecdh_secp256k1_test.json tcId 1) and brainpoolP256r1 (RFC 7027 Section A.1). Both entries exercise the full set1EVP_PKEY_deriveget1 round-trip, pinning the allowlist.

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.

Feature Request: Support for Brainpool Curves (RFC 5639) in TLS 1.2 and TLS 1.3

3 participants