docs: add WPA3-SAE (PSA Crypto API 1.4) design proposal #837

Open
krish2718 wants to merge 1 commit from krish2718/wpa3-sae-design-note into development
krish2718 commented 2026-07-12 15:39:42 -05:00 (Migrated from github.com)

This adds a Phase 0 design proposal (docs/proposed/wpa3-sae.md) for
implementing the WPA3-SAE PAKE from the PSA Certified Crypto API 1.4
(§10.13.12–13) in the built-in driver.

It is a design note for discussion — no implementation code — intended
to reach agreement on scope, API surface, new primitives, constant-time
strategy, and PR phasing before any code is written. Relates to #709.

What it covers

  • Scope: hash-to-element only; ECC groups 19/20/21; FFDH via
    PSA_DH_FAMILY_RFC3526; the FIXED and GDH algorithm variants and the
    WPA3_SAE_H2E KDF. Protocol-level concerns (anti-clogging, group
    negotiation, SAE-PK, mesh) are out of scope — they belong in the Wi-Fi stack.
  • API surface from the 1.4 spec, with section citations.
  • New primitives: RFC 9380 hash-to-curve (Simplified SWU), a constant-time
    modular square root, and the SAE engine.
  • Constant-time strategy, per secret-dependent step (Dragonblood is the
    motivating threat).
  • Test plan and a proposed P1–P5 PR phasing.

Open questions for maintainers (see §9 of the note)

  1. FFDH group set — the 1.4 spec only defines PSA_DH_FAMILY_RFC3526
    (IANA 15–18) for SAE FFDH tokens; confirm this is the intended set.
  2. Looping variant — proposal is to not implement it (no constant-time
    PWE; H2E supersedes it) and return PSA_ERROR_NOT_SUPPORTED.
  3. Built-in vs driver-only — proposal is a full built-in implementation
    (SPAKE2+ is currently API-encoding-only).
This adds a Phase 0 design proposal (`docs/proposed/wpa3-sae.md`) for implementing the WPA3-SAE PAKE from the PSA Certified Crypto API 1.4 (§10.13.12–13) in the built-in driver. It is a **design note for discussion — no implementation code** — intended to reach agreement on scope, API surface, new primitives, constant-time strategy, and PR phasing before any code is written. Relates to #709. ### What it covers - **Scope**: hash-to-element only; ECC groups 19/20/21; FFDH via `PSA_DH_FAMILY_RFC3526`; the `FIXED` and `GDH` algorithm variants and the `WPA3_SAE_H2E` KDF. Protocol-level concerns (anti-clogging, group negotiation, SAE-PK, mesh) are out of scope — they belong in the Wi-Fi stack. - **API surface** from the 1.4 spec, with section citations. - **New primitives**: RFC 9380 hash-to-curve (Simplified SWU), a constant-time modular square root, and the SAE engine. - **Constant-time strategy**, per secret-dependent step (Dragonblood is the motivating threat). - **Test plan** and a proposed **P1–P5 PR phasing**. ### Open questions for maintainers (see §9 of the note) 1. **FFDH group set** — the 1.4 spec only defines `PSA_DH_FAMILY_RFC3526` (IANA 15–18) for SAE FFDH tokens; confirm this is the intended set. 2. **Looping variant** — proposal is to *not* implement it (no constant-time PWE; H2E supersedes it) and return `PSA_ERROR_NOT_SUPPORTED`. 3. **Built-in vs driver-only** — proposal is a full built-in implementation (SPAKE2+ is currently API-encoding-only). ### Related - Spec example errata found during this work are filed as GlobalPlatform/psa-api#372.
gilles-peskine-arm (Migrated from github.com) requested changes 2026-07-20 06:35:22 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

Thank you for providing a detailed design plan.

I've reviewed it as someone who is familiar with PSA APIs in general and with TF-PSA-Crypto as a product, but not with the PAKE API and not with the WPA3 algorithm family. In particular, I have not confronted the proposal with the WPA3 and related specifications.

Thank you for providing a detailed design plan. I've reviewed it as someone who is familiar with PSA APIs in general and with TF-PSA-Crypto as a product, but not with the PAKE API and not with the WPA3 algorithm family. In particular, I have not confronted the proposal with the WPA3 and related specifications.
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 05:50:33 -05:00

Also a new flag ALG_IS_WPA3_SAE in tests/suites/test_suite_psa_crypto_metadata.function

Also a new flag `ALG_IS_WPA3_SAE` in `tests/suites/test_suite_psa_crypto_metadata.function`
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 05:51:38 -05:00

Adding new files in an existing directory should not require any changes to **/CMakeLists.txt.

Adding new files in an existing directory should not require any changes to `**/CMakeLists.txt`.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:05:04 -05:00

Note: changelog entries only for things visible in the API, not for internal elements that are not reachable from the API yet.

Note: changelog entries only for things visible in the API, not for internal elements that are not reachable from the API yet.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:06:34 -05:00

Specifically uncrustify 0.75.1. But it's almost K&R2 so the details shouldn't be a problem.

Specifically uncrustify 0.75.1. But it's almost K&R2 so the details shouldn't be a problem.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:08:32 -05:00

The framework PR will be merged first, and needs to work both with the current code and with the new code (but this shouldn't be an issue when adding a new feature). It only needs to be coordinated in the sense that we like the CI on the using PR to pass before we merge the framework PR, because that gives assurance that the framework PR isn't missing something.

The framework PR will be merged first, and needs to work both with the current code and with the new code (but this shouldn't be an issue when adding a new feature). It only needs to be coordinated in the sense that we like the CI on the using PR to pass before we merge the framework PR, because that gives assurance that the framework PR isn't missing something.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:11:58 -05:00

There is already a square root implementation in mbedtls_ecp_sw_derive_y() in ecp.c. Prefer refactoring this to make it usable for SAE.

There is already a square root implementation in `mbedtls_ecp_sw_derive_y()` in `ecp.c`. Prefer refactoring this to make it usable for SAE.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:24:54 -05:00

Please avoid landing test vectors before the corresponding algorithm. In particular, merging tests that are not executed is technically possible, but it's a hassle.

Please avoid landing test vectors before the corresponding algorithm. In particular, merging tests that are not executed is technically possible, but it's a hassle.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:26:27 -05:00

As far as I can tell from reading this document, but without being familiar with the mechanisms that are implemented, it seems to me that sae.c (+ unit tests) and hash_to_curve.c (+ unit tests) should be separate PR. I'm not sure about the other bits.

As far as I can tell from reading this document, but without being familiar with the mechanisms that are implemented, it seems to me that `sae.c` (+ unit tests) and `hash_to_curve.c` (+ unit tests) should be separate PR. I'm not sure about the other bits.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:27:10 -05:00

Constant-time annotations don't involve much code. They should preferably be present from the start.

Fuzzing, on the other hand, can be added as a separate later PR.

Constant-time annotations don't involve much code. They should preferably be present from the start. Fuzzing, on the other hand, can be added as a separate later PR.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:28:08 -05:00

Documentation should be provided as early as possible, not as an afterthought. Writing good documentation often helps remove ambiguities from the specification and helps with the test plan.

Documentation should be provided as early as possible, not as an afterthought. Writing good documentation often helps remove ambiguities from the specification and helps with the test plan.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:29:19 -05:00

PSA_DH_FAMILY_RFC3526 should be added in a PR that doesn't add H2E/SAE code.

`PSA_DH_FAMILY_RFC3526` should be added in a PR that doesn't add H2E/SAE code.
@ -0,0 +144,4 @@
- `drivers/builtin/include/mbedtls/private/crypto_adjust_config_enable_builtins.h`
— derive `MBEDTLS_PSA_BUILTIN_ALG_WPA3_SAE_*` / `MBEDTLS_PSA_BUILTIN_PAKE`,
pull in ECP/bignum (and, P3, FFDH/RFC3526) modules.
- `core/check_crypto_config.h` — dependency `#error` checks.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 05:53:33 -05:00

Also scripts/data_files/config-options-current.txt

Also `scripts/data_files/config-options-current.txt`
@ -0,0 +181,4 @@
**Framework submodule (separate repo)**
- `framework/scripts/mbedtls_framework/crypto_knowledge.py` — register the new
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 05:55:48 -05:00

Also probably a block in framework/tests/src/psa_exercise_key.c for the new KDF.

Also probably a block in `framework/tests/src/psa_exercise_key.c` for the new KDF.
@ -0,0 +294,4 @@
and would put SAE on the critical path of a bignum API extension. See §9, open
question 5.
Existing building blocks we rely on: the constant-time module
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:19:04 -05:00

The library has several internal bignum interfaces, from lowest level to highest level: bignum_core.h, bignum_mod_raw.h, bignum_mod.h, mbedtls/private/bignum.h. Lower-level interfaces generally have less overhead and are easier to make constant-time, but have a higher risk of memory management mistakes.

The existing code in ecp.c mostly relies on mbedtls/private/bignum.h because that was the only one available when ecp.c was originally written. However, for new code, bignum_mod.h is generally preferred, with bignum_mod_raw.h or bignum_core.h available if bignum_mod.h is not flexible enough or induces too much overhead.

Given that the new code for this feature will have a large adherence to the existing code in ecp.c, it's not obvious to me which abstraction level to prefer.

The library has several internal bignum interfaces, from lowest level to highest level: `bignum_core.h`, `bignum_mod_raw.h`, `bignum_mod.h`, `mbedtls/private/bignum.h`. Lower-level interfaces generally have less overhead and are easier to make constant-time, but have a higher risk of memory management mistakes. The existing code in `ecp.c` mostly relies on `mbedtls/private/bignum.h` because that was the only one available when `ecp.c` was originally written. However, for new code, `bignum_mod.h` is generally preferred, with `bignum_mod_raw.h` or `bignum_core.h` available if `bignum_mod.h` is not flexible enough or induces too much overhead. Given that the new code for this feature will have a large adherence to the existing code in `ecp.c`, it's not obvious to me which abstraction level to prefer.
@ -0,0 +336,4 @@
for `…_GDH(SHA_256)` and for `…_FIXED(SHA_384)`), and the exact extent of
`PSA_ALG_WPA3_SAE_ANY` — which algorithms it does and does not permit,
including that it does not permit non-SAE algorithms.
- **Opacity (acceptance criteria)**: (a) PMK from `get_shared_key()` with
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 05:58:24 -05:00

Also test algorithm policies: non-wildcard policies require an exact match, and test the extent of PSA_ALG_WPA3_SAE_ANY.

Also test algorithm policies: non-wildcard policies require an exact match, and test the extent of `PSA_ALG_WPA3_SAE_ANY`.
@ -0,0 +402,4 @@
merge at a major/minor release per the API-extension policy. The framework
submodule change is a separate PR — see §3.
## 9. Open questions for maintainers
gilles-peskine-arm (Migrated from github.com) commented 2026-07-20 06:22:53 -05:00

The design choices made here match Arm's preferences.

The design choices made here match Arm's preferences.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:19 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:19 -05:00

Added.

Added.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:21 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:21 -05:00

Right, it globs src/*.c — dropped that line.

Right, it globs `src/*.c` — dropped that line.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:23 -05:00
@ -0,0 +144,4 @@
- `drivers/builtin/include/mbedtls/private/crypto_adjust_config_enable_builtins.h`
— derive `MBEDTLS_PSA_BUILTIN_ALG_WPA3_SAE_*` / `MBEDTLS_PSA_BUILTIN_PAKE`,
pull in ECP/bignum (and, P3, FFDH/RFC3526) modules.
- `core/check_crypto_config.h` — dependency `#error` checks.
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:23 -05:00

Added.

Added.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:25 -05:00
@ -0,0 +181,4 @@
**Framework submodule (separate repo)**
- `framework/scripts/mbedtls_framework/crypto_knowledge.py` — register the new
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:25 -05:00

Added.

Added.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:27 -05:00
@ -0,0 +336,4 @@
for `…_GDH(SHA_256)` and for `…_FIXED(SHA_384)`), and the exact extent of
`PSA_ALG_WPA3_SAE_ANY` — which algorithms it does and does not permit,
including that it does not permit non-SAE algorithms.
- **Opacity (acceptance criteria)**: (a) PMK from `get_shared_key()` with
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:27 -05:00

Added — exact-match on non-wildcard policies (FIXED(SHA_256) key rejected for GDH(SHA_256) and for FIXED(SHA_384)), plus what PSA_ALG_WPA3_SAE_ANY does and does not cover.

Added — exact-match on non-wildcard policies (`FIXED(SHA_256)` key rejected for `GDH(SHA_256)` and for `FIXED(SHA_384)`), plus what `PSA_ALG_WPA3_SAE_ANY` does and does not cover.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:30 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:29 -05:00

Noted. P1/P2 are internal-only, so no entry there; the ChangeLog fragment goes with the PSA-integration PR.

Noted. P1/P2 are internal-only, so no entry there; the ChangeLog fragment goes with the PSA-integration PR.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:32 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:32 -05:00

Pinned 0.75.1 in the note.

Pinned 0.75.1 in the note.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:34 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:34 -05:00

Thanks, that's clearer than what I had — reworded: framework merges first, written to work with both old and new code, and CI green on the consuming PR as the signal it isn't missing anything.

Thanks, that's clearer than what I had — reworded: framework merges first, written to work with both old and new code, and CI green on the consuming PR as the signal it isn't missing anything.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:36 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:36 -05:00

Missed that one — will refactor it rather than add a second sqrt. Three deltas for the SSWU side, none changing existing behaviour: the operand becomes a parameter (SSWU needs the sqrt of an arbitrary field element, not ecp_sw_rhs()); it returns a CT is_square flag (cheapest by squaring the result back, not a second exp_mod); and root selection stays in the callers, since sw_derive_y selects on a public parity bit and SSWU selects on secret data. Written up as §4.1, planned at bignum_core.h level — ecp.c can call it by growing to grp->P.n and passing Y->p, Y->n.

Missed that one — will refactor it rather than add a second sqrt. Three deltas for the SSWU side, none changing existing behaviour: the operand becomes a parameter (SSWU needs the sqrt of an arbitrary field element, not `ecp_sw_rhs()`); it returns a CT `is_square` flag (cheapest by squaring the result back, not a second `exp_mod`); and root selection stays in the callers, since `sw_derive_y` selects on a public parity bit and SSWU selects on secret data. Written up as §4.1, planned at `bignum_core.h` level — ecp.c can call it by growing to `grp->P.n` and passing `Y->p, Y->n`.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:38 -05:00
@ -0,0 +294,4 @@
and would put SAE on the critical path of a bignum API extension. See §9, open
question 5.
Existing building blocks we rely on: the constant-time module
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:38 -05:00

I went through the headers on this and I think the choice is narrower than it first looks. bignum_mod.h fits SSWU conceptually — one fixed modulus, a long run of ops — but as it stands it can't express it:

  • no exponentiation, and the square root is w^((p+1)/4) mod p. exp_mod exists only as mbedtls_mpi_exp_mod() / mbedtls_mpi_core_exp_mod();
  • no conditional assign — CMOV is mbedtls_mpi_mod_raw_cond_assign(), a layer down, and SSWU needs it throughout;
  • no comparison — lt_ct / check_zero_ct are in bignum_core.h;
  • residues may be in Montgomery form, so RFC 9380 sgn0 (a parity test on the canonical value) reads the wrong thing without converting out.

So it's really bignum_core.h vs extending bignum_mod.h first. I'd lean bignum_core.h for hash_to_curve.c and the shared sqrt (it has exp_mod, cond_assign, lt_ct, check_zero_ct and explicit mont conversion, at fixed limb counts), and keep sae.c on mbedtls_mpi, since it's mostly orchestrating mbedtls_ecp_* and anything else just buys conversions at every boundary.

Fixed-limb matters here beyond style: mbedtls_mpi normalizes its limb count, so a secret value that's numerically small runs shorter and leaks its magnitude — same class as Dragonblood — so I'd rather not have it near the field arithmetic. (mbedtls_mpi_exp_mod() is fine on that point; it grows to N->n first.)

Happy to extend bignum_mod.h instead if you'd prefer the library to move that way, but I'd rather that be its own piece of work than a dependency of this one. Written up as §4.3 / open question 5 — leaving this thread open for your call.

I went through the headers on this and I think the choice is narrower than it first looks. `bignum_mod.h` fits SSWU conceptually — one fixed modulus, a long run of ops — but as it stands it can't express it: - no exponentiation, and the square root is `w^((p+1)/4) mod p`. `exp_mod` exists only as `mbedtls_mpi_exp_mod()` / `mbedtls_mpi_core_exp_mod()`; - no conditional assign — CMOV is `mbedtls_mpi_mod_raw_cond_assign()`, a layer down, and SSWU needs it throughout; - no comparison — `lt_ct` / `check_zero_ct` are in `bignum_core.h`; - residues may be in Montgomery form, so RFC 9380 `sgn0` (a parity test on the canonical value) reads the wrong thing without converting out. So it's really `bignum_core.h` vs extending `bignum_mod.h` first. I'd lean `bignum_core.h` for `hash_to_curve.c` and the shared sqrt (it has exp_mod, cond_assign, lt_ct, check_zero_ct and explicit mont conversion, at fixed limb counts), and keep `sae.c` on `mbedtls_mpi`, since it's mostly orchestrating `mbedtls_ecp_*` and anything else just buys conversions at every boundary. Fixed-limb matters here beyond style: `mbedtls_mpi` normalizes its limb count, so a secret value that's numerically small runs shorter and leaks its magnitude — same class as Dragonblood — so I'd rather not have it near the field arithmetic. (`mbedtls_mpi_exp_mod()` is fine on that point; it grows to `N->n` first.) Happy to extend `bignum_mod.h` instead if you'd prefer the library to move that way, but I'd rather that be its own piece of work than a dependency of this one. Written up as §4.3 / open question 5 — leaving this thread open for your call.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:40 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:40 -05:00

Dropped the vectors-only phase — tests now land in the PR with the code they exercise.

Dropped the vectors-only phase — tests now land in the PR with the code they exercise.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:42 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:42 -05:00

Agreed, split. P1 = sqrt refactor + hash_to_curve.c (RFC 9380 App J vectors), P2 = sae.c (hostap vectors). P2 depends on P1; nothing else does.

Agreed, split. P1 = sqrt refactor + `hash_to_curve.c` (RFC 9380 App J vectors), P2 = `sae.c` (hostap vectors). P2 depends on P1; nothing else does.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:45 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:45 -05:00

Done — CT annotations are a per-phase rule now, not a phase; fuzzing is its own later PR.

Done — CT annotations are a per-phase rule now, not a phase; fuzzing is its own later PR.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:47 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:47 -05:00

Agreed — docs land with the code that introduces them.

Agreed — docs land with the code that introduces them.
krish2718 (Migrated from github.com) reviewed 2026-07-23 12:51:49 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 12:51:49 -05:00

Done — it's a standalone phase now, independent of the SAE work, can land in any order relative to it.

Done — it's a standalone phase now, independent of the SAE work, can land in any order relative to it.
krish2718 commented 2026-07-23 12:52:06 -05:00 (Migrated from github.com)

Thanks, this was a useful pass — all 16 points are in the updated note (force-pushed). Substantive changes:

  • §7 phasing reworked: no vectors-only PR (tests land with the code they exercise), hash_to_curve.c and sae.c split into separate PRs, PSA_DH_FAMILY_RFC3526 as a standalone phase with no SAE code, constant-time annotations and documentation as per-phase rules rather than a trailing hardening pass, fuzzing last.
  • §4.1: the modular square root is now a refactor of mbedtls_ecp_sw_derive_y() rather than new code.
  • new §4.3 + open question 5: bignum abstraction level.

On that last one — the answer came out more definite than "open question" suggests, so flagging it here too: bignum_mod.h has no exponentiation, no conditional assign and no comparison, and its Montgomery representation breaks RFC 9380 sgn0, so the real choice is bignum_core.h vs extending bignum_mod.h first. We'd propose the former; detail in the thread on §4. That's the one thread I've left open for your call — the rest of the review is addressed and resolved.

The three maintainer-facing open questions (§9: FFDH group set, omitting Looping, built-in vs encoding-only) are unchanged and still open.

Unrelated: the spec example errata (#372) merged, targeted for 1.5.1 — the note now reflects that.

Thanks, this was a useful pass — all 16 points are in the updated note (force-pushed). Substantive changes: - **§7 phasing** reworked: no vectors-only PR (tests land with the code they exercise), `hash_to_curve.c` and `sae.c` split into separate PRs, `PSA_DH_FAMILY_RFC3526` as a standalone phase with no SAE code, constant-time annotations and documentation as per-phase rules rather than a trailing hardening pass, fuzzing last. - **§4.1**: the modular square root is now a refactor of `mbedtls_ecp_sw_derive_y()` rather than new code. - **new §4.3 + open question 5**: bignum abstraction level. On that last one — the answer came out more definite than "open question" suggests, so flagging it here too: `bignum_mod.h` has no exponentiation, no conditional assign and no comparison, and its Montgomery representation breaks RFC 9380 `sgn0`, so the real choice is `bignum_core.h` vs extending `bignum_mod.h` first. We'd propose the former; detail in the thread on §4. That's the one thread I've left open for your call — the rest of the review is addressed and resolved. The three maintainer-facing open questions (§9: FFDH group set, omitting Looping, built-in vs encoding-only) are unchanged and still open. Unrelated: the spec example errata (#372) merged, targeted for 1.5.1 — the note now reflects that.
gilles-peskine-arm (Migrated from github.com) requested changes 2026-07-23 14:24:10 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

Thanks for the update. I'm generally happy with the plan now, except that I'm unsure about bignum_core vs bignum_mod: the arguments given against bignum_mod feel very weak to me. It's hard for me to judge because I'm not at all familiar with the algorithm.

Do you have a reading suggestion (code or math+English) that would make it clear how the representation of numbers matter? Are all the numbers involved in the same Z_p, or is there a mix? At which steps are conversions to/from Montgomery form needed?

Note: when I say I'm happy with the plan, that's as a general plan for implementing some calculations as a new cryptographic mechanism in TF-PSA-Crypto. I'm not familiar with the WPA3-SAE family of algorithms, so I can't vouch for difficulties or incompleteness for implementing the desired algorithms.

Thanks for the update. I'm generally happy with the plan now, except that I'm unsure about `bignum_core` vs `bignum_mod`: the arguments given against `bignum_mod` feel very weak to me. It's hard for me to judge because I'm not at all familiar with the algorithm. Do you have a reading suggestion (code or math+English) that would make it clear how the representation of numbers matter? Are all the numbers involved in the same $Z_p$, or is there a mix? At which steps are conversions to/from Montgomery form needed? Note: when I say I'm happy with the plan, that's as a general plan for implementing some calculations as a new cryptographic mechanism in TF-PSA-Crypto. I'm not familiar with the WPA3-SAE family of algorithms, so I can't vouch for difficulties or incompleteness for implementing the desired algorithms.
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
gilles-peskine-arm (Migrated from github.com) commented 2026-07-23 13:18:27 -05:00

It's ok for this time because all the changes are in a single new file, but for future reference: please avoid force-pushing to a pull request after the first review has started. Make new changes in separate commits. Each commit should only do one thing (no big commit “address all the review comments”).

It's ok for this time because all the changes are in a single new file, but for future reference: please avoid force-pushing to a pull request after the first review has started. Make new changes in separate commits. Each commit should only do one thing (no big commit “address all the review comments”).
gilles-peskine-arm (Migrated from github.com) commented 2026-07-23 13:53:04 -05:00

“the one Gilles noted” won't make sense outside of the context of the review of this pull request.

“the one Gilles noted” won't make sense outside of the context of the review of this pull request.
@ -0,0 +262,4 @@
- **No conditional assign.** CMOV is `mbedtls_mpi_mod_raw_cond_assign()`, one
layer down; SSWU needs it throughout.
- **No comparison.** The CT predicates (`mbedtls_mpi_core_lt_ct`,
`mbedtls_mpi_core_check_zero_ct`) live in `bignum_core.h`.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-23 13:50:55 -05:00

It would be easy to add comparison and conditional assignment, and not hard to add exponentiation. So I don't think those missing functions should weigh much in the decision.

It would be easy to add comparison and conditional assignment, and not hard to add exponentiation. So I don't think those missing functions should weigh much in the decision.
@ -0,0 +266,4 @@
- **Montgomery representation gets in the way of `sgn0`.** Residues may be held
in Montgomery form depending on the modulus `int_rep`, so a bit-level parity
test — which RFC 9380 `sgn0` needs — reads the wrong value unless converted
out first.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-23 13:52:35 -05:00

A conversion step can be done. And conversion has to be done manually anyway when using bignum_core. How is bignum_mod worse in this respect?

A conversion step can be done. And conversion has to be done manually anyway when using bignum_core. How is bignum_mod worse in this respect?
@ -0,0 +400,4 @@
0.75.1** and the in-tree `.uncrustify.cfg`.
- **API stability**: this is additive (new macros/types/functions); it can only
merge at a major/minor release per the API-extension policy. The framework
submodule change is a separate PR — see §3.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-23 14:00:38 -05:00

To be precise, the changes inside the framework are a separate PR since it's a separate repository. Updating the framework submodule in the TF-PSA-Crypto repository should be done in the TF-PSA-Crypto PR that needs it, but the need may go away if another TF-PSA-Crypto PR updates the framework for its own needs first.

To be precise, the changes inside the framework are a separate PR since it's a separate repository. Updating the framework submodule in the TF-PSA-Crypto repository should be done in the TF-PSA-Crypto PR that needs it, but the need may go away if another TF-PSA-Crypto PR updates the framework for its own needs first.
@ -0,0 +435,4 @@
extend `bignum_mod.h` first. We propose `bignum_core.h` for
`hash_to_curve.c` and the shared square root, and `mbedtls_mpi` for
`sae.c`. If extending `bignum_mod.h` is wanted instead, we would rather do
it as its own piece of work than on this feature's critical path.
gilles-peskine-arm (Migrated from github.com) commented 2026-07-23 14:11:48 -05:00

Many functions in the bignum modules and in the ecp module have automatically generated tests (framework/scripts/generate_bignum_tests.py, framework/scripts/generate_ecp_tests.py). If this work introduces new functions that should have automatically generated tests, we'll need to arrange. to add new tested functions

Because the framework code and the crypto code can't be updated atomically, the first time those scripts need to cover a new test function, this will require preliminary work to split the script into two parts:

  • framework/scripts/mbedtls_framwork/xxx_generator.py having almost all the code.
  • A small harness tests/scripts/generate_xxx.py that calls an entry point in the framework and passes it arguments that describe which test functions should be covered.
  • For a transition period, framework/scripts/generate_xxx.py still exists. It calls tests/scripts/generate_xxx.py from the consuming branch if it exists, and otherwise generates test cases for the historical set of test functions.

See scripts/maintainer/generate_mldsa_tests.py and framework/util/mbedtls_maintainer/mldsa_test_generator.py for an example of this split. Note that the file paths are different for mldsa because it needs extra python packages, and that would break some validation scripts that we run on framework/scripts, scripts and tests/scripts; that doesn't apply for bignum and ecp.

Many functions in the bignum modules and in the ecp module have automatically generated tests (`framework/scripts/generate_bignum_tests.py`, `framework/scripts/generate_ecp_tests.py`). If this work introduces new functions that should have automatically generated tests, we'll need to arrange. to add new tested functions Because the framework code and the crypto code can't be updated atomically, the first time those scripts need to cover a new test function, this will require preliminary work to split the script into two parts: * `framework/scripts/mbedtls_framwork/xxx_generator.py` having almost all the code. * A small harness `tests/scripts/generate_xxx.py` that calls an entry point in the framework and passes it arguments that describe which test functions should be covered. * For a transition period, `framework/scripts/generate_xxx.py` still exists. It calls `tests/scripts/generate_xxx.py` from the consuming branch if it exists, and otherwise generates test cases for the historical set of test functions. See `scripts/maintainer/generate_mldsa_tests.py` and `framework/util/mbedtls_maintainer/mldsa_test_generator.py` for an example of this split. Note that the file paths are different for mldsa because it needs extra python packages, and that would break some validation scripts that we run on `framework/scripts`, `scripts` and `tests/scripts`; that doesn't apply for bignum and ecp.
krish2718 (Migrated from github.com) reviewed 2026-07-23 15:44:19 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 15:44:18 -05:00

Assitant (LLM) leakage :), will fix it thanks.

Assitant (LLM) leakage :), will fix it thanks.
krish2718 (Migrated from github.com) reviewed 2026-07-23 16:01:34 -05:00
@ -0,0 +294,4 @@
and would put SAE on the critical path of a bignum API extension. See §9, open
question 5.
Existing building blocks we rely on: the constant-time module
krish2718 (Migrated from github.com) commented 2026-07-23 16:01:34 -05:00

You're right, and two of my three arguments don't hold up:

  • "missing functions" — fair, add/sub/mul/inv are there and cmp / cond-assign / exp are easy to add, so that shouldn't weigh much.
  • the limb-normalization / magnitude-leak point — that's an argument against legacy mbedtls_mpi, not against bignum_mod. bignum_mod residues are fixed-size (limbs == modulus limbs), so it doesn't distinguish the two. I conflated two axes; scratch that one.

So it's really about ergonomics and blast radius, not capability.

Your questions:

All in the same Z_p, or a mix? For hash_to_curve.c it's essentially one field. Every step of hash_to_field + simple-SWU + the square root is arithmetic mod p, the curve's base-field prime — mul/add/sub/inv/sqrt/sgn0, all mod p; cofactor clearing is a no-op (h=1 for P-256/384/521). The only non-field integers are the exponents fed to exp_mod (e.g. (p+1)/4), which are bit-strings not residues, and the final point add PT = P1 + P2, a curve-group op best handed to mbedtls_ecp. Scalars mod the group order n show up in the SAE protocol (commit scalar etc.) but those live in sae.c, not here. So: single modulus, long run of ops — which is exactly the bignum_mod sweet spot, so your instinct is the right one.

Where are Montgomery conversions needed? With bignum_mod in Montgomery int_rep: convert in once when the field element is read (mbedtls_mpi_mod_read), then every mul/add/sub/inv stays in Montgomery — no per-op conversion — and convert out only at (a) sgn0, since RFC 9380's sign is the parity of the canonical representative, and (b) the final handoff to point arithmetic. exp_mod for the sqrt wants the base in Montgomery plus a precomputed RR, which the modulus object already carries. With bignum_core you do the identical conversions by hand and manage RR yourself — so on conversions the two are a wash, which was your point on the other thread.

So I'll drop the objection: bignum_mod is a good fit for hash_to_curve.c and the shared sqrt. The one tradeoff left is scope, not correctness — see my reply on the generated-tests comment.

Reading, if useful: RFC 9380 §4.1 (hash_to_field, sgn0) and §6.6.2 (simple SWU) for what the numbers are; the bignum_mod.h header doc block (int_rep / residue model) plus HAC §14.3.2 (Montgomery reduction) for why representation matters — short version: Montgomery form stores x·R mod p so reduction is shifts instead of division; it's stable under +/-/×, but any step that reads the integer itself (parity, comparison, serialization) has to convert back first, which is why sgn0 is the one place we drop out.

You're right, and two of my three arguments don't hold up: - "missing functions" — fair, `add`/`sub`/`mul`/`inv` are there and cmp / cond-assign / exp are easy to add, so that shouldn't weigh much. - the limb-normalization / magnitude-leak point — that's an argument against legacy `mbedtls_mpi`, not against `bignum_mod`. `bignum_mod` residues are fixed-size (limbs == modulus limbs), so it doesn't distinguish the two. I conflated two axes; scratch that one. So it's really about ergonomics and blast radius, not capability. Your questions: **All in the same Z_p, or a mix?** For `hash_to_curve.c` it's essentially one field. Every step of `hash_to_field` + simple-SWU + the square root is arithmetic mod `p`, the curve's base-field prime — mul/add/sub/inv/sqrt/sgn0, all mod `p`; cofactor clearing is a no-op (h=1 for P-256/384/521). The only non-field integers are the exponents fed to `exp_mod` (e.g. `(p+1)/4`), which are bit-strings not residues, and the final point add `PT = P1 + P2`, a curve-group op best handed to `mbedtls_ecp`. Scalars mod the group order `n` show up in the SAE protocol (commit scalar etc.) but those live in `sae.c`, not here. So: single modulus, long run of ops — which is exactly the `bignum_mod` sweet spot, so your instinct is the right one. **Where are Montgomery conversions needed?** With `bignum_mod` in Montgomery `int_rep`: convert in once when the field element is read (`mbedtls_mpi_mod_read`), then every mul/add/sub/inv stays in Montgomery — no per-op conversion — and convert out only at (a) `sgn0`, since RFC 9380's sign is the parity of the *canonical* representative, and (b) the final handoff to point arithmetic. `exp_mod` for the sqrt wants the base in Montgomery plus a precomputed `RR`, which the modulus object already carries. With `bignum_core` you do the identical conversions by hand and manage `RR` yourself — so on conversions the two are a wash, which was your point on the other thread. So I'll drop the objection: `bignum_mod` is a good fit for `hash_to_curve.c` and the shared sqrt. The one tradeoff left is scope, not correctness — see my reply on the generated-tests comment. Reading, if useful: RFC 9380 §4.1 (`hash_to_field`, `sgn0`) and §6.6.2 (simple SWU) for what the numbers are; the `bignum_mod.h` header doc block (`int_rep` / residue model) plus HAC §14.3.2 (Montgomery reduction) for why representation matters — short version: Montgomery form stores `x·R mod p` so reduction is shifts instead of division; it's stable under +/-/×, but any step that reads the integer itself (parity, comparison, serialization) has to convert back first, which is why `sgn0` is the one place we drop out.
krish2718 (Migrated from github.com) reviewed 2026-07-23 16:01:36 -05:00
@ -0,0 +262,4 @@
- **No conditional assign.** CMOV is `mbedtls_mpi_mod_raw_cond_assign()`, one
layer down; SSWU needs it throughout.
- **No comparison.** The CT predicates (`mbedtls_mpi_core_lt_ct`,
`mbedtls_mpi_core_check_zero_ct`) live in `bignum_core.h`.
krish2718 (Migrated from github.com) commented 2026-07-23 16:01:36 -05:00

Agreed — conceded on the main thread; that argument doesn't carry weight, the functions are cheap to add.

Agreed — conceded on the main thread; that argument doesn't carry weight, the functions are cheap to add.
krish2718 (Migrated from github.com) reviewed 2026-07-23 16:01:37 -05:00
@ -0,0 +266,4 @@
- **Montgomery representation gets in the way of `sgn0`.** Residues may be held
in Montgomery form depending on the modulus `int_rep`, so a bit-level parity
test — which RFC 9380 `sgn0` needs — reads the wrong value unless converted
out first.
krish2718 (Migrated from github.com) commented 2026-07-23 16:01:37 -05:00

Right — conversions are manual either way; I walked through where they land on the main thread. Not a point in bignum_core's favour.

Right — conversions are manual either way; I walked through where they land on the main thread. Not a point in `bignum_core`'s favour.
krish2718 (Migrated from github.com) reviewed 2026-07-23 16:01:39 -05:00
@ -0,0 +435,4 @@
extend `bignum_mod.h` first. We propose `bignum_core.h` for
`hash_to_curve.c` and the shared square root, and `mbedtls_mpi` for
`sae.c`. If extending `bignum_mod.h` is wanted instead, we would rather do
it as its own piece of work than on this feature's critical path.
krish2718 (Migrated from github.com) commented 2026-07-23 16:01:39 -05:00

This is the real deciding factor now that capability isn't. If we go bignum_mod we're adding exp_mod (and probably cmp / cond-assign) as first-class bignum_mod functions, which per your note means doing the generator split — a mbedtls_framework/..._generator.py plus a thin tests/scripts/generate_*.py harness, transition shim and all — following the mldsa example. bignum_core already has all of these, so that path adds no bignum API and touches no generator.

So the choice is: grow bignum_mod properly (better long-term home, but the first code PR then also owns a bignum-module extension + the generator split), or keep the first SSWU PR tight on bignum_core and leave bignum_mod growth as separate future work. I lean toward doing it properly on bignum_mod since you'd want those functions eventually anyway — but you're better placed to say whether you want that coupled into the first SSWU PR or kept out of it. Either way I'll write it up to match.

This is the real deciding factor now that capability isn't. If we go `bignum_mod` we're adding `exp_mod` (and probably cmp / cond-assign) as first-class `bignum_mod` functions, which per your note means doing the generator split — a `mbedtls_framework/..._generator.py` plus a thin `tests/scripts/generate_*.py` harness, transition shim and all — following the mldsa example. `bignum_core` already has all of these, so that path adds no bignum API and touches no generator. So the choice is: grow `bignum_mod` properly (better long-term home, but the first code PR then also owns a bignum-module extension + the generator split), or keep the first SSWU PR tight on `bignum_core` and leave `bignum_mod` growth as separate future work. I lean toward doing it properly on `bignum_mod` since you'd want those functions eventually anyway — but you're better placed to say whether you want that coupled into the first SSWU PR or kept out of it. Either way I'll write it up to match.
krish2718 (Migrated from github.com) reviewed 2026-07-23 16:01:40 -05:00
@ -0,0 +400,4 @@
0.75.1** and the in-tree `.uncrustify.cfg`.
- **API stability**: this is additive (new macros/types/functions); it can only
merge at a major/minor release per the API-extension policy. The framework
submodule change is a separate PR — see §3.
krish2718 (Migrated from github.com) commented 2026-07-23 16:01:40 -05:00

Understood — I'll fix §3 to say it precisely: the framework changes are a separate PR (separate repo), and the submodule bump lands in whichever TF-PSA-Crypto PR first needs it, so the "merged first" framing was too rigid.

Understood — I'll fix §3 to say it precisely: the framework changes are a separate PR (separate repo), and the submodule bump lands in whichever TF-PSA-Crypto PR first needs it, so the "merged first" framing was too rigid.
krish2718 (Migrated from github.com) reviewed 2026-07-23 16:01:58 -05:00
@ -0,0 +1,441 @@
WPA3-SAE (PSA Certified Crypto API 1.4) — design note
krish2718 (Migrated from github.com) commented 2026-07-23 16:01:58 -05:00

Removed in 986f205.

Removed in 986f205.
krish2718 commented 2026-07-23 16:06:15 -05:00 (Migrated from github.com)

@gilles-peskine-arm — I answered your three questions (representation/reading, single-field vs mix, where the Montgomery conversions land) on the bignum thread rather than here so they wouldn't get buried: https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/837#discussion_r3641366480

Short version: it's a single field (F_p) throughout hash_to_curve.c, conversions are in-once and out-only-at-sgn0, and I've dropped the objection — bignum_mod is a fine fit. The one open point is scope: grow bignum_mod (which pulls in the generator split) vs keep the first PR tight on bignum_core. Your call.

@gilles-peskine-arm — I answered your three questions (representation/reading, single-field vs mix, where the Montgomery conversions land) on the bignum thread rather than here so they wouldn't get buried: https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/837#discussion_r3641366480 Short version: it's a single field (F_p) throughout `hash_to_curve.c`, conversions are in-once and out-only-at-`sgn0`, and I've dropped the objection — `bignum_mod` is a fine fit. The one open point is scope: grow `bignum_mod` (which pulls in the generator split) vs keep the first PR tight on `bignum_core`. Your call.
gilles-peskine-arm (Migrated from github.com) approved these changes 2026-07-24 08:17:35 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM at 986f205106, with the caveat that I am not familiar with WPA3-SAE and I have not checked the plan against the specification of the mechanisms.

On bignum_core vs bignum_mod, I could go either way based on my knowledge. I'll ask a colleague who's more familiar with ECC math and the planned evolution of bignum and ECP modules.

LGTM at 986f205106f7a8602dae59afcb6ef91c9a5b315a, with the caveat that I am not familiar with WPA3-SAE and I have not checked the plan against the specification of the mechanisms. On `bignum_core` vs `bignum_mod`, I could go either way based on my knowledge. I'll ask a colleague who's more familiar with ECC math and the planned evolution of bignum and ECP modules.
krish2718 commented 2026-07-28 03:24:36 -05:00 (Migrated from github.com)

@gilles-peskine-arm Should I add any other reviewer to get this merged?

@gilles-peskine-arm Should I add any other reviewer to get this merged?
gilles-peskine-arm commented 2026-07-28 04:32:17 -05:00 (Migrated from github.com)

We do need a second reviewer. But I have no idea when someone will be available or who it will be.

By the way, given the size of the contribution, have you made arrangements with our product management to have the review of the code scheduled? We currently have very little bandwidth since most of the team is triaging and fixing LLM-assisted vulnerability reports.

We do need a second reviewer. But I have no idea when someone will be available or who it will be. By the way, given the size of the contribution, have you made arrangements with our product management to have the review of the code scheduled? We currently have very little bandwidth since most of the team is triaging and fixing LLM-assisted vulnerability reports.
krish2718 commented 2026-08-02 13:30:27 -05:00 (Migrated from github.com)

We do need a second reviewer. But I have no idea when someone will be available or who it will be.

Ah, any process to followup here or a list of people to nudge :)?

By the way, given the size of the contribution, have you made arrangements with our product management to have the review of the code scheduled? We currently have very little bandwidth since most of the team is triaging and fixing LLM-assisted vulnerability reports.

No, but if you can guide me, I am happy to connect and take this forward with the product management.

> We do need a second reviewer. But I have no idea when someone will be available or who it will be. Ah, any process to followup here or a list of people to nudge :)? > By the way, given the size of the contribution, have you made arrangements with our product management to have the review of the code scheduled? We currently have very little bandwidth since most of the team is triaging and fixing LLM-assisted vulnerability reports. No, but if you can guide me, I am happy to connect and take this forward with the product management.
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'krish2718/wpa3-sae-design-note:krish2718/wpa3-sae-design-note'
git switch 'krish2718/wpa3-sae-design-note'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'krish2718/wpa3-sae-design-note'
git switch 'krish2718/wpa3-sae-design-note'
git rebase 'development'
git switch 'development'
git merge --ff-only 'krish2718/wpa3-sae-design-note'
git switch 'krish2718/wpa3-sae-design-note'
git rebase 'development'
git switch 'development'
git merge --no-ff 'krish2718/wpa3-sae-design-note'
git switch 'development'
git merge --squash 'krish2718/wpa3-sae-design-note'
git switch 'development'
git merge --ff-only 'krish2718/wpa3-sae-design-note'
git switch 'development'
git merge 'krish2718/wpa3-sae-design-note'
git push origin 'development'
Sign in to join this conversation.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!837
No description provided.