From d82956385bf11fcb89402d038f9fd5b2e3021105 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Fri, 11 Sep 2026 07:38:21 +0000 Subject: [PATCH] record S-1/N-1 remediation status + the N-4 parenthetical correction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - review 001: §Status carries the remediation status (S-1 + N-1 landed in e86b8ba, OQ-TLS-09 open, probe permanent) and the N-4 correction summary; §S-1 remediation-task line updated; §N-1 marked remediated; §N-4 gains the corrected mechanism chain (rustls 0.23.41/0.23.44 send [RawPublicKey] iff the resolver's only_raw_public_keys() is true — no "offers both types" behavior exists; a raw-key client fails IncorrectCertificateTypeExtension against this verifier; the S-1 probe presents the SPKI under the default X.509 offer instead) - task fix-accept-any-cert-verifier-posture: drop the placeholder fill-in lines (Notes/Summary carry the actual record) - task docs-pin-c1-c4-n3-n4: N-4 work item rewritten from the corrected chain so the implementing agent does not re-derive it; references point at the rustls source lines and the probe's resolver choice --- docs/reviews/001-implementation-review.md | 78 ++++++++++++++++++- tasks/docs-pin-c1-c4-n3-n4.md | 57 ++++++++++---- tasks/fix-accept-any-cert-verifier-posture.md | 4 - 3 files changed, 117 insertions(+), 22 deletions(-) diff --git a/docs/reviews/001-implementation-review.md b/docs/reviews/001-implementation-review.md index f8eb913..4073399 100644 --- a/docs/reviews/001-implementation-review.md +++ b/docs/reviews/001-implementation-review.md @@ -8,6 +8,31 @@ logged, N-2 retracted).** Findings filed from the `review-impl` post-port review (2026-09-10), an independent pass on top of the already-green phase gate (`tasks/review-impl.md`, the same day). +**Remediation status (2026-09-11): S-1 (+N-1) remediated** — the +no-pop doc note is on `AcceptAnyCertVerifier`, the N-1 companion fix is +on `FingerprintPinVerifier`, OQ-TLS-09 records the ownership question +(open), and the impersonation probe is permanent +(`tests/impersonation_posture.rs`, tcp-gated, both cert types) — +commit `e86b8ba`, task +`tasks/fix-accept-any-cert-verifier-posture.md` (completed). The +remaining findings are tracked in the six remediation tasks from the +001-decomposition (commit `d7db6b1`). + +**One correction to this review, found during the S-1 remediation:** the +N-4 parenthetical "alknet's own client resolver offers both types" is +**inaccurate** — rustls 0.23.41 and 0.23.44 both send +`client_certificate_types = [RawPublicKey]` iff the *resolver's* +`only_raw_public_keys()` is true (client/hs.rs:331-334 in 0.23.41, +:355-358 in 0.23.44); there is no "offers both types" behavior in +either version. The raw-key impersonation probe works because the +attacker presents the SPKI under the *default* X.509 offer +(`only_raw_public_keys() == false`), which the server accepts unparsed +(`(false, _, true)` arm of `process_cert_type_extension` — server/hs.rs +0.23.44:274-288). A rustls-driven raw-key client resolver (offering +`[RawPublicKey]` only) would fail `IncorrectCertificateTypeExtension` +against this verifier — see N-4 below for the corrected mechanism, and +write the N-4 doc note from the rustls sources, not the parenthetical. + This review's focus (per the user's direction): the crate stands on its own as the publication target — correctness, security, and test coverage — with the alknet sources consulted only as behavior-preservation @@ -240,10 +265,16 @@ vulnerable *now*. endpoint model wants mandatory pop, that's a cheap, additive companion type — decide before the first consumer. -**Remediation task:** none exists yet; if adopted, land as -`tasks/fix-accept-any-cert-verifier-posture.md` alongside the ADR/OQ -above. (This review does not change code — the finding is doc + ADR -work.) +**Remediation task:** ~~none exists yet;~~ **landed** — +`tasks/fix-accept-any-cert-verifier-posture.md` (completed, commit +`e86b8ba`): the S-1 doc note on `AcceptAnyCertVerifier`, the N-1 +companion fix, OQ-TLS-09 (open, three options a/b/c), and the +impersonation probe permanent as `tests/impersonation_posture.rs` +(tcp-gated, both cert types — handshake completes, app data flows, the +server extracts the victim's fingerprint; a future pop change must +fail/update that test with the doc + OQ together). server.md and +client.md carry the same posture. This review does not change code — +the finding is doc + ADR work. ## C-1 [minor, correctness] — `generate_self_signed_cert()` produces a cert valid 1975→4096 (rcgen's default), i.e. effectively never-expiring, and no doc states it @@ -468,6 +499,12 @@ the server-side verifier (S-1). One clarifying line ("this verifier checks proof-of-possession; the *server-side* AcceptAnyCertVerifier does not — see that type's docs") closes the loop. +**Remediated (commit `e86b8ba`)**: the phrasing is replaced (the cert +is presented fresh each handshake; the signature check defeats use of +a stolen/observed cert by a party without the private key) and the +cross-reference to `AcceptAnyCertVerifier` exists on the type, in +client.md, and server.md. + ## N-2 — NON-FINDING (retracted): the client config's `dangerous()` verifier path was suspected of allowing an unverified-CA downgrade — it does not Filed as a suspect ("`with_custom_certificate_verifier` + `None` @@ -522,6 +559,39 @@ explicitly: **a raw-key client must include `X509` in its verifier rejects it.** (alknet's own client resolver offers both types, which is why the extracted code works in production.) +**Correction to the parenthetical above (found during the S-1 +remediation, 2026-09-11):** "alknet's own client resolver offers both +types" is **inaccurate**. rustls 0.23.41 and 0.23.44 both send +`client_certificate_types = [RawPublicKey]` (only) whenever the +*resolver's* `only_raw_public_keys()` is true (client/hs.rs:331-334 in +0.23.41, :355-358 in 0.23.44) — no version offers both types. The +consequence chain, verified from the rustls sources: + +1. A rustls-driven client with a raw-key resolver (e.g. + `RawKeyClientCertResolver` with an Ed25519 SPKI, + `only_raw_public_keys() == true`) offers `[RawPublicKey]` only. +2. Against `AcceptAnyCertVerifier` + (`requires_raw_public_keys() == false`), the `(false, true, false)` + arm of `process_cert_type_extension` (server/hs.rs:274-288) fires → + `IncorrectCertificateTypeExtension` → **handshake failure**. +3. So a raw-key *client* cannot handshake against this verifier at + all (fail-closed, not a downgrade — but a real interop trap the + doc note must state). +4. The S-1 raw-key impersonation probe sidesteps this: the attacker + presents the victim's SPKI under the *default* X.509 offer + (`only_raw_public_keys() == false` → no cert-type extension → + `(false, false, false)` → default X.509), and the server passes the + SPKI bytes through unparsed (a custom `verify_client_cert` receives + opaque bytes) — which is why the probe shape pins + `only_raw_public_keys() == false` on its fixed resolver. + +The doc note for task 6 (`docs-pin-c1-c4-n3-n4`) should be written +from this chain (and the rustls sources), not from the review's +original parenthetical: the accurate statement is that +`requires_raw_public_keys()` must stay `false` for X.509-or-raw +*server* offers, and that a raw-key *client* resolver makes rustls +offer `[RawPublicKey]` only, which this verifier rejects. + ## N-5 — NON-FINDING (verified): the dropped `zeroize::ZeroizeOnDrop` marker impl does not change memory-erasure behavior The port review recorded the omission as accepted diff --git a/tasks/docs-pin-c1-c4-n3-n4.md b/tasks/docs-pin-c1-c4-n3-n4.md index 9e56048..a6a3d37 100644 --- a/tasks/docs-pin-c1-c4-n3-n4.md +++ b/tasks/docs-pin-c1-c4-n3-n4.md @@ -42,18 +42,39 @@ question): reasoning in the type's doc (one sentence) and close the question — revisit only with a concrete consumer demanding it. 4. **N-4** — the cert-type negotiation interaction needs an explicit - doc note on `AcceptAnyCertVerifier`: `requires_raw_public_keys()` - stays `false` (both cert types accepted), and — the subtle part, - verified against rustls 0.23.44 `validate_client_cert_type_extension` - in the decomposition session — **a client offering ONLY - `RawPublicKey` fails the handshake** with - `IncorrectCertificateTypeExtension`; it must offer both - `[X509, RawPublicKey]` (or the verifier would have to set - `requires_raw_public_keys() == true`). The extracted alknet client - resolver offers both types, which is why production works. Without - the note, the next reader may "fix" the verifier to `true` and - break X.509 clients, or mis-diagnose the raw-only-client failure as - a bug. + doc note on `AcceptAnyCertVerifier`. **Read the corrected mechanism + first** — the review's §N-4 original parenthetical ("alknet's own + client resolver offers both types, which is why production works") + is inaccurate; the review's §Status block and §N-4 now carry the + correction (found during the S-1 remediation, commit `e86b8ba`). + The accurate chain, from the rustls sources (0.23.41 AND 0.23.44): + - rustls sends `client_certificate_types = [RawPublicKey]` (only) + iff the *resolver's* `only_raw_public_keys()` is true + (client/hs.rs:331-334 in 0.23.41, :355-358 in 0.23.44) — no + "offers both types" behavior exists in either version. + - Against `AcceptAnyCertVerifier` + (`requires_raw_public_keys() == false`), a client offering only + `[RawPublicKey]` hits the `(false, true, false)` arm of + `process_cert_type_extension` (server/hs.rs:274-288 in 0.23.44) + → `IncorrectCertificateTypeExtension` → handshake failure. + - A client with an X.509-typed resolver (or no resolver) offers no + cert-type extension / `[X509]` → the `(false, _, true)` or + `(false, false, false)` arm → default X.509 → works, and the + server passes presented bytes through unparsed to a custom + `verify_client_cert`. + - So `requires_raw_public_keys()` must stay `false` (do not "fix" + it to `true` — that would break X.509 clients), and the doc note + must state that a raw-key *client* resolver makes rustls offer + `[RawPublicKey]` only, which this verifier rejects + (fail-closed, not a downgrade). Note the S-1 doc on the same + type (landed, commit `e86b8ba`) already carries the no-pop + section; add the negotiation note as a sibling section, and note + that a raw-key *server* presentation (`RawKeyCertResolver`, + `only_raw_public_keys() == true` on the server cert side) is a + different knob (the `server_certificate_types` extension) that + is unaffected. Without the note, the next reader may "fix" the + verifier to `true` and break X.509 clients, or mis-diagnose the + raw-only-client failure as a bug. ## Work @@ -87,10 +108,18 @@ question): ## References - docs/reviews/001-implementation-review.md §C-1, §C-4, §N-3, §N-4 + (§N-4 — read the correction block; the original parenthetical is + inaccurate), and the §Status block (the correction summary) - src/server.rs:226-256, src/client.rs:225-236, src/credentials.rs:35-39, src/identity.rs (SelfSigned doc) -- rustls 0.23.44 `server/hs.rs` `validate_client_cert_type_extension` - (the negotiation table N-4 documents) +- rustls 0.23.44 `server/hs.rs` `process_cert_type_extension` + (server/hs.rs:263-288 — the `(requires_raw_keys, offers_raw, + offers_x509)` negotiation table N-4 documents) and + `client/hs.rs:350-358` (the `[RawPublicKey]`-only offer rule) — + both pinned in rustls 0.23.41 too (client/hs.rs:326-334) +- tests/impersonation_posture.rs (the S-1 probe — its fixed resolver + pins `only_raw_public_keys() == false` for the X.509-typed offer; + do not confuse that with a raw-key client resolver) ## Notes diff --git a/tasks/fix-accept-any-cert-verifier-posture.md b/tasks/fix-accept-any-cert-verifier-posture.md index 726b136..ba9c107 100644 --- a/tasks/fix-accept-any-cert-verifier-posture.md +++ b/tasks/fix-accept-any-cert-verifier-posture.md @@ -109,8 +109,6 @@ finding, not a code-change mandate. What's missing: ## Notes -> Agent fills this during implementation. - - The probe used a tokio duplex pair + `TlsConnector`/`TlsAcceptor` (tokio-rustls, `tcp`-gated) rather than raw `complete_io` — same shape, cleaner await-based flow. The attacker client uses a @@ -139,8 +137,6 @@ finding, not a code-change mandate. What's missing: ## Summary -> Agent fills this on completion. - **Landed (this task):** 1. `src/server.rs` — `AcceptAnyCertVerifier` doc rewritten: a