From 06d4f688b30b43151bcc8f0653e8287a06987dc7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 13 Jun 2026 04:41:52 +0000 Subject: [PATCH 1/3] ci: add CodeQL static-analysis workflow for C# Adds .github/workflows/codeql.yml running CodeQL against the C# code on self-hosted runners (zero GitHub-hosted Actions minutes). Triggers on push/PR to main, a weekly schedule, and workflow_dispatch. Uses manual build-mode (CPM + .slnx makes autobuild unreliable) and wraps the Release build in the EXCLUSIVE box-wide flock, matching fuzz.yml's CPU-heavy discipline. https://claude.ai/code/session_01GSRLZXowGYeRieqaevEtBb --- .github/workflows/codeql.yml | 76 ++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 .github/workflows/codeql.yml diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 00000000..e7552450 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,76 @@ +name: codeql + +# CodeQL static analysis for the C# code. Runs the github/codeql-action +# JS actions on our own self-hosted runners — zero GitHub-hosted Actions +# minutes consumed (this project has no hosted-runner budget; every job +# targets [self-hosted, Linux, X64], same discipline as ci/fuzz/interop). +# +# Findings surface in the repo Security > Code scanning tab (SARIF upload), +# so on top of push/PR we run a weekly scheduled sweep to re-scan main +# against any newly-shipped CodeQL queries even when the code is quiet. + +on: + push: + branches: [main] + pull_request: + branches: [main] + schedule: + # Weekly, Mondays at 03:37 UTC. Deliberately off the half-hour and + # distinct from the 02:17 fuzz nightly so the two CPU-heavy workflows + # never collide on the shared self-hosted box. + - cron: '37 3 * * 1' + workflow_dispatch: + +concurrency: + group: codeql-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + # CodeQL uploads its results as SARIF to the code-scanning API. + security-events: write + # The codeql-action reads workflow/run metadata when uploading. + actions: read + +env: + # GitHub will force JavaScript actions onto Node.js 24 from + # June 2nd, 2026 and remove Node.js 20 from the runner on + # September 16th, 2026. Opting in early both silences the + # deprecation warning and lets us catch any Node 24 breakage + # well ahead of the forced cutover. + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + +jobs: + analyze: + name: analyze (csharp) + runs-on: [self-hosted, Linux, X64] + # Self-hosted Linux runner — zero GitHub Actions minutes consumed. + steps: + - uses: actions/checkout@v6 + + - name: initialize CodeQL + uses: github/codeql-action/init@v3 + with: + languages: csharp + # Manual build mode: autobuild can't reliably restore this repo + # (Central Package Management + a .slnx solution), so we drive the + # build ourselves between init and analyze. + build-mode: manual + + - name: report .NET + run: dotnet --version + + - name: build + # The compile CodeQL traces to build its C# database. This is a full + # Release build of the whole solution → CPU-heavy, so it takes the + # EXCLUSIVE box-wide flock (same discipline as fuzz.yml's smoke run): + # never overlap a concurrent CPU-heavy workflow on the shared + # self-hosted host (§7.2.2). flock passes the command's exit through. + run: | + flock -x /tmp/pdn-ci-heavy.lock \ + dotnet build Packet.NET.slnx --configuration Release + + - name: analyze CodeQL + uses: github/codeql-action/analyze@v3 + with: + category: /language:csharp From 966088e4e66e4b72e5069af1a028a0c7ea2b80c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 13 Jun 2026 04:48:47 +0000 Subject: [PATCH 2/3] docs: WebAuthn/passkey ceremony security review (no exploitable findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Focused security review of the passkey registration + assertion ceremonies (Fido2NetLib 4.0.0-beta.16): challenge lifecycle, origin/RP-id pinning, sign-count clone detection, UV/UP enforcement, credential<->user binding, fail-closed error handling, and log hygiene. Verdict: no clear, exploitable bug — no source changed. All security-critical properties are correctly defended and already pinned by existing tests. Four observations flagged for the owner (UV=Preferred policy knob; username-scoped assert/begin existence-oracle by design; unused GetAllCredentialIds; message-substring clone classification) — all policy/clarity, none a defect. [skip-plan] https://claude.ai/code/session_01GSRLZXowGYeRieqaevEtBb --- docs/webauthn-review-2026-06-13.md | 171 +++++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 docs/webauthn-review-2026-06-13.md diff --git a/docs/webauthn-review-2026-06-13.md b/docs/webauthn-review-2026-06-13.md new file mode 100644 index 00000000..67a1f118 --- /dev/null +++ b/docs/webauthn-review-2026-06-13.md @@ -0,0 +1,171 @@ +# WebAuthn / passkey ceremony review — 2026-06-13 + +A focused security review of the WebAuthn / passkey registration and authentication +ceremonies, picking up the thread left open by `docs/security-review-2026-06-13.md` +§"Residual / not done this pass" ("WebAuthn / passkey ceremony was not deep-reviewed +beyond the challenge-cache bounds"). The ceremonies are built on `Fido2.AspNet` +(`Fido2NetLib` 4.0.0-beta.16). + +This is the written findings report. **No source behaviour was changed** — the surface +is sound; the items below are observations and deliberate-by-design notes, none of which +is a clear exploitable bug. See the "Considered and rejected" section for what was looked +at and why it is not a finding. + +## Scope + +| File | What was checked | +|---|---| +| `src/Packet.Node/Api/PdnWebAuthnApi.cs` | register + assert begin/complete endpoints, credential list/delete, principal binding, error handling | +| `src/Packet.Node/Api/WebAuthnFido2Builder.cs` | RP id pinning, accepted-origin computation (Host-spoof resistance) | +| `src/Packet.Node.Core/Auth/WebAuthnChallengeCache.cs` | challenge single-use / expiry / key-binding / bounded memory | +| `src/Packet.Node.Core/Auth/WebAuthnCredentialRecord.cs` | record shape, sign-count semantics | +| `src/Packet.Node.Core/Auth/IWebAuthnCredentialStore.cs` + `SqliteWebAuthnCredentialStore.cs` | credential persistence, ownership guard, counter update, duplicate handling | +| `src/Packet.Node/Program.cs` | DI wiring + the `read`-gate on the register/management group | +| `tests/Packet.Node.Tests` (WebAuthn*) | existing coverage / harness conventions | + +## Verdict + +**No exploitable findings.** Each of the high-value WebAuthn failure modes was checked +and is correctly defended: + +1. **Challenge lifecycle — sound.** `WebAuthnChallengeCache.Take` removes-and-returns + via `ConcurrentDictionary.TryRemove` (atomic single-use); a replayed `complete` finds + nothing (`PdnWebAuthnApi.cs:153`, `:344`). Each entry carries an absolute expiry off + the injected `TimeProvider` (`WebAuthnChallengeCache.cs:105`, `:120`) — no wall-clock, + 5-minute default TTL. The verifier always compares the authenticator's signed challenge + against the server-stashed options' challenge, never a client-echoed value + (`PdnWebAuthnApi.cs:200`, `:369`). Registration is keyed to the enrolling **user** + (`reg:`); assertion is keyed to a fresh 256-bit CSPRNG session handle + (`assert:`, `PdnWebAuthnApi.cs:540-541`) — one user's pending ceremony cannot + be completed as another. `PruneExpired` (`:131`) bounds memory against abandoned + ceremonies; it is called opportunistically on every begin. + +2. **Origin & RP ID validation — sound.** RP id is fixed from config and never derived + from the request (`WebAuthnFido2Builder.cs:48`). When the operator pins + `AllowedOrigins`, **only** those are trusted — the request's own (spoofable) origin is + deliberately not added (`WebAuthnFido2Builder.cs:71-78`), so a forged `Host` header + cannot widen the accepted set. The zero-config path trusts the actual serving origin + + loopback only, which is the intended localhost-first behaviour. `WebAuthnConfigValidator` + rejects an IP-literal RP id (`NodeConfigValidator.cs:603`). Pinned by + `WebAuthnFido2BuilderTests` including the explicit spoofed-Host case. + +3. **Signature counter (clone detection) — sound.** The stored counter is passed as + `MakeAssertionParams.StoredSignatureCounter` (`PdnWebAuthnApi.cs:470`); Fido2NetLib + throws `Fido2VerificationException` on a regression, which is caught and rejected + (`:202-216`). A second, belt-and-braces monotonic check runs in our own code + (`:220-224`) for authenticators that use counters (`> 0`). On success the advanced + counter is persisted via `UpdateSignCount` (`:228`, `SqliteWebAuthnCredentialStore.cs:185`). + The `0 → 0` case (platform/passkey authenticators that never increment) is correctly + treated as "no counter, no regression". + +4. **User verification / presence — spec-compliant.** UP is always enforced by the + library. UV is requested as `Preferred` for both registration and assertion + (`PdnWebAuthnApi.cs:300`, `:112`). `Preferred` means UV is *not* mandatory — this is a + deliberate, spec-valid policy choice for a localhost-first node, not a defect. (See + Observation O-1.) + +5. **Credential ↔ user binding — sound.** + - *Assertion:* the credential is resolved by its raw id (`GetByCredentialId`, + `:183`); the `IsUserHandleOwnerOfCredentialIdCallback` (`:473-478`) ties the + authenticator's returned user handle to the stored username with a constant-time + compare (`CryptographicOperations.FixedTimeEquals`). Registration forces + `ResidentKey=Required` (`:299`), so every credential is discoverable and returns a + user handle, meaning the callback always runs. The minted token is for the + credential's *stored* owner — no cross-user acceptance. + - *Registration:* `IsCredentialIdUniqueToUserCallback` (`:372-373`) plus the + `credential_id` PRIMARY KEY (`SqliteWebAuthnCredentialStore.cs:39`) make a credential + id globally unique — a duplicate `Add` returns `false` on SQLITE_CONSTRAINT (`:111`) + and the enrolment fails closed (`PdnWebAuthnApi.cs:392-396`). No overwrite of another + user's credential is possible. + - *Delete:* the `username` predicate in the SQL is the ownership guard + (`SqliteWebAuthnCredentialStore.cs:213-214`) — a caller can only remove their own + passkey, regardless of the id supplied. + +6. **Error handling — fails closed.** Every failure path (`WebAuthnUnavailable`, + malformed request, unparseable response, no pending challenge, unknown credential, + owner-gone, verification exception, counter regression) returns a rejection; there is no + fall-through to an authenticated state. `Fido2VerificationException` is the only caught + exception around the verify and it always rejects (`:202`, `:376`). The assert failures + all return a single generic 401 (`AssertionRejected`) so no step is a behavioural oracle. + +7. **Secrets / PII in logs — clean.** Audit logs carry username + client IP + a short + reason string (`AuthLog.*`), never the challenge, the attestation/assertion blob, the + public key, or the credential id. The store's `WebAuthnCredentialSummary` projection + omits the public key entirely (`WebAuthnCredentialRecord.cs:64`). The principal username + always comes from the authenticated principal, never the request body + (`PdnWebAuthnApi.cs:527-538`). + +## Observations (not bugs — flagged for the owner, not changed) + +These are deliberate design positions or low-value polish items. Per the review's +"report-don't-change for anything ambiguous or architecturally significant" rule, none was +modified. + +- **O-1 (informational): UV is `Preferred`, not `Required`.** Both ceremonies request + `UserVerification = Preferred` (`PdnWebAuthnApi.cs:112`, `:300`). This is spec-valid and + appropriate for the localhost-first threat model, but it means a passkey can authenticate + with user-presence only (no PIN/biometric) if the authenticator chooses. If a real-domain + operator wants to *require* UV (e.g. the `pdn.m0lte.uk`-from-a-phone tier), that would be a + config knob on `WebAuthnConfig` plumbed into both `GetAssertionOptionsParams.UserVerification` + and the `AuthenticatorSelection`. **Flagged, not changed** — tightening the default would be + a behaviour change and is a policy decision for the operator, not a bug. + +- **O-2 (low / by-design): username-scoped `assert/begin` is a passkey-existence oracle.** + `assert/begin` with a `username` returns that user's credential descriptors (ids + + transports) in `allowCredentials`; an unknown user — or one with no passkeys — returns an + empty list (`PdnWebAuthnApi.cs:99-106`). The inline comment claims this does not leak + whether a username exists; that is true only for users *without* passkeys. A user **with** + enrolled passkeys is distinguishable (non-empty `allowCredentials`, and the credential ids + are exposed pre-auth). This is **inherent to the standard non-discoverable `allowCredentials` + assertion flow** — it is how username-scoped WebAuthn login works, and the endpoint must be + pre-auth. The mitigation already in place is that the username-less / discoverable path + (empty `allowCredentials`) is the default and leaks nothing. **Flagged, not changed:** + removing the username-scoped allow-list would break non-discoverable / cross-device + authenticators, an architecturally significant behaviour change. Worth a one-line doc note + that the username field is an enrolment-existence oracle by design, and worth tightening the + code comment at `:96-98` (which overstates the indistinguishability). + +- **O-3 (nit): `IWebAuthnCredentialStore.GetAllCredentialIds` is unused by the endpoints.** + The XML doc says it is for building the discoverable-assertion allow-list and enforcing + global uniqueness, but the discoverable path deliberately uses an empty allow-list (correct — + enumerating every user's credentials into a pre-auth response would be a privacy leak), and + uniqueness is enforced via `GetByCredentialId` in the `IsCredentialIdUniqueToUserCallback`. + The method is only exercised by its own unit tests. Harmless dead-ish surface; **not a + security issue.** Could be removed or its doc-comment corrected in a future tidy. + +- **O-4 (informational): `CounterRegressed` classifies by exception message substring.** + The audit-log branch that distinguishes a clone from a generic verification failure matches + `ex.Message.Contains("counter")` (`PdnWebAuthnApi.cs:483-484`). This is **log-classification + only** — both branches reject — so a library message change would at worst mislabel an audit + line, never weaken the rejection. The independent belt-and-braces check at `:220` is the + durable counter guard. **Not a finding;** noted because it is the one place coupled to a + library string. + +## Considered and rejected (not findings) + +- *Could a non-discoverable credential bypass the user-handle binding (callback only runs + when a user handle is present)?* No — registration forces `ResidentKey=Required`, so every + credential is discoverable and returns a user handle; the callback always runs in this flow. +- *Counter belt-and-braces is weaker than the library (only fires when `stored > 0`).* By + design — the library is the primary check and covers the `stored==0, new>0` first-increment + and `new<=stored` regression cases; our check is the redundant guard, not the sole one. +- *Challenge TTL too long?* 5 minutes is generous head-room for a few-seconds ceremony but is + a tightly bounded, single-use, key-bound window — not a meaningful replay surface. +- *Sign-count `uint` ↔ SQLite signed-`long` round-trip.* Lossless (a uint always fits a signed + 64-bit), `unchecked((uint)row.SignCount)` reverses it; no truncation reachable. +- *Store faults opening an auth bypass.* Faults degrade to null/empty/false (lookups fail + closed → "unknown credential" → 401; writes fail closed). A best-effort `UpdateSignCount` + fault cannot fail an otherwise-good assertion, but the security-relevant counter check has + already run in the verify path before that write, so a missed counter *persist* (not check) + is the documented, acceptable degradation, not a clone bypass. + +## Outcome + +No source changed, no new tests required: there is no clear unambiguous bug to pin. The +existing test coverage (`WebAuthnChallengeCacheTests`, `WebAuthnFido2BuilderTests`, +`SqliteWebAuthnCredentialStoreTests`, `WebAuthnApiTests`, plus the CDP-virtual-authenticator +E2E `scripts/passkey-e2e.mjs`) already pins the security-critical properties — single-use, +expiry, key-binding, Host-spoof resistance, ownership-scoped delete, and the no-oracle +generic rejection. The owner may wish to act on O-1 (a `requireUserVerification` knob) and +O-2 (correct the over-strong comment at `PdnWebAuthnApi.cs:96-98`); both are policy/clarity, +not defects. From 654e07e96629e939fce901f70713e7bed09be64b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 13 Jun 2026 04:50:04 +0000 Subject: [PATCH 3/3] docs: plan amendment-log entry for CodeQL + WebAuthn review tracks Ledger entries for the two follow-on security tracks integrated in this PR: the CodeQL static-analysis workflow and the WebAuthn ceremony review (no findings). Per the repo hard rule, the plan is updated in the same PR. https://claude.ai/code/session_01GSRLZXowGYeRieqaevEtBb --- docs/plan.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/plan.md b/docs/plan.md index 79ab8547..c4d6cac3 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1070,6 +1070,10 @@ What changed, why, where to look for details. ``` +### 2026-06-13 — CodeQL static analysis (C#) added; WebAuthn ceremony review (no findings) + +Two follow-on security tracks after the #405 hardening pass. **(1) CodeQL** — new `.github/workflows/codeql.yml` runs the C# CodeQL analysis on `[self-hosted, Linux, X64]` (zero hosted-runner minutes, per the project rule) on push/PR to main + a weekly cron + dispatch; `build-mode: manual` (CPM + the `.slnx` solution defeat autobuild) with the Release build wrapped in the exclusive `flock -x /tmp/pdn-ci-heavy.lock` like `fuzz.yml`. NB: SARIF upload to the Security tab needs GitHub code scanning enabled on the repo (GHAS on a private repo) — flagged for the owner; the workflow itself is correct regardless. **(2) WebAuthn / passkey ceremony review** — read the registration + assertion path (`PdnWebAuthnApi`, `WebAuthnFido2Builder`, `WebAuthnChallengeCache`, the credential store) end to end: **no exploitable findings.** Challenge is single-use (atomic `TryRemove`) + TTL'd off the injected clock + user-bound; RP-id/origins are pinned from config (a spoofed Host can't widen them); sign-count clone-detection is checked and persisted; credential↔user binding is enforced with a constant-time compare and global credential-id uniqueness; everything fails closed behind a single generic 401. Four non-defect observations recorded (UV=`Preferred` vs `Required` is a policy knob; the username-scoped `assert/begin` is an inherent passkey-existence oracle in the non-discoverable flow; an unused store method; clone-vs-generic log classification keys off an exception substring). Full write-up in [`docs/webauthn-review-2026-06-13.md`](webauthn-review-2026-06-13.md); the session's earlier tracks are in [`docs/security-review-2026-06-13.md`](security-review-2026-06-13.md). + ### 2026-06-13 — Web auth core security review (no findings) — written up Read the security-critical web-auth components end to end (`JwtTokenService`, `RefreshTokenService`, `TotpService`, `PasswordHasher`, `ScopeRequirementHandler` / `AuthScopes.Satisfies`, and the `PdnAuthApi` login flow). **No exploitable issues** — the surface is well-built: HS256 with `ValidAlgorithms` pinned (blocks alg-confusion), zero clock skew; opaque hashed one-time-use refresh tokens with reuse-detection; the TOTP strictly-greater replay guard with constant-time compare; Argon2id at OWASP params with fixed-time verify; correct rank-based scope implication with no fail-open; and a login flow that throttles before the Argon2 verify, pays full Argon2 cost on unknown users (no enumeration timing oracle), and returns a generic 401. Two non-bug observations recorded (TOTP high-water-mark persistence is the caller's responsibility; RHP auth is cleartext-by-protocol so network exposure remains the real control). Full write-up plus the whole session's security work in [`docs/security-review-2026-06-13.md`](security-review-2026-06-13.md).