From 26173b3926d00feaae15bb3c141f2cbcf0234494 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Fri, 23 Oct 2026 19:24:45 -0700 Subject: [PATCH] Security review pass 2: login, tokens, password reset, 2FA, anonymous routes F5 (critical, confirmed on the local server): the 2FA step accepts an empty or missing temp token, so users with 2FA on can be logged into without a password. F1 confirmed: with the placeholder secret a user's current token can be rebuilt. F8 confirmed. New findings F6, F7, F9 to F12. Review only, no code changes. Co-Authored-By: Claude Opus 5.5 --- devdocs/modernization/SECURITY-REVIEW.md | 136 +++++++++++++++++++++-- 1 file changed, 124 insertions(+), 12 deletions(-) diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index 5ad06f80..c36b8757 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -12,8 +12,8 @@ keep it that way while findings are open. | Pass | Area | Status | |---|---|---| | 1 | Threat model and attack-surface map | Done 2026-10-23 | -| 2 | Unauthenticated routes, login, tokens, password reset, 2FA | Next | -| 3 | Customer contact users: what they can reach and read | | +| 2 | Unauthenticated routes, login, tokens, password reset, 2FA | Done 2026-10-23 | +| 3 | Customer contact users: what they can reach and read | Next | | 4 | Report rendering (templates, the server-side browser, report data and download routes) | | | 5 | File handling: attachments, uploads, exports, logs, backups, restore, translations, logo | | | 6 | Raw SQL: data lists, saved filters, search, KPI, schedule, pick lists | | @@ -186,23 +186,134 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | ID | Severity | Status | Title | |---|---|---|---| -| F1 | High | candidate | Placeholder JWT secret in the LAN/IIS and Linux server config templates | -| F2 | Medium | candidate | Windows installers give all local users modify rights on the program and data folders | +| F5 | **Critical** | **confirmed** | 2FA login step accepts an empty or missing temp token: the password is skipped for users with 2FA on | +| F1 | Critical (installs that kept it) | **confirmed** | Placeholder JWT secret in the LAN/IIS and Linux server config templates | | F3 | High | candidate | Any logged-in user, customer contacts included, can send arbitrary email through the company's SMTP server | +| F6 | Medium | confirmed | No lockout or rate limit on login or the 2FA step; no password rules | +| F9 | Medium | confirmed | Default SuperUser password, and an anonymous route that says whether it is still set | +| F2 | Medium | candidate | Windows installers give all local users modify rights on the program and data folders | +| F7 | Low | confirmed | Password change or reset does not end existing sessions, and makes the download key non-expiring | +| F8 | Low | confirmed | Latest-token check skipped for any URL ending in `/auth` or `notify/hello` | | F4 | Low | candidate | Any logged-in user can send an in-app notification to any user | +| F10 | Low | confirmed | Anyone can record acceptance of the licence agreement (`license/lc`) | +| F11 | Low | confirmed | API root page shows licensee, version, licence terms (and DB ID when unlicensed) to anyone | +| F12 | Low | confirmed | Password hashing: PBKDF2-SHA512 with 10,000 iterations | -### F1 Placeholder JWT secret (from the 8.3.0 installer test) +### F5 2FA step skips the password (pass 2, confirmed on the local server) + +`AuthController.TfaAuthenticate` (`POST auth/tfa-authenticate`, anonymous) finds the user +only by `TempToken == pin.TempToken` (plus active, login allowed, 2FA on), then checks the +6-digit code. The temp token is the only proof that the password was given in step one, but: + +- after a successful 2FA login it is set to `""` (`AuthController.cs:193`), so sending + `"tempToken": ""` matches every 2FA user who has completed a 2FA login; +- a user who has just turned 2FA on has it `null`; sending no temp token matches them; +- `TempToken` is not required in `TFAPinParam`, and a wrong code does not clear it. + +So anyone, with no login and no password, can send codes to this route and log in as +whichever 2FA user the query returns first. TOTP accepts 3 codes at any moment +(TwoFactorAuth.Net default discrepancy 1), the only throttle is the 3 s failure delay, +which parallel requests get around, and there is no lockout (F6) and no nginx rate limit on +hosted droplets. At a few hundred guesses per second that is, on average, well under an +hour; even at 20 per second, a few hours. Users who turn 2FA on are the more privileged +ones, so 2FA makes those accounts weaker than a password alone. + +**Proof** (local dev server, seeded `Inventory` user, 2FA turned off again afterwards): +2FA just turned on, no password, temp token omitted, correct code: 200 and a token for +Inventory. After one normal 2FA login, no password, temp token `""`, correct code: 200 and a +token. Wrong code: 401. (Script kept outside the repo; the test needs the user's code, which +an attacker guesses.) + +**Fix** (proposed for 8.3.0): reject a null or empty temp token before the query; clear the +temp token (set it to null) on success and on every failed code, so each guess needs the +password again; optionally expire the temp token after a few minutes. Small change in one +method; client unaffected (it always sends the `tt` it received). + +### F1 Placeholder JWT secret (from the 8.3.0 installer test; confirmed in pass 2) `dist/assets/lan-install-config.json` and `dist/assets/linux-server/config.json` ship `AYANOVA_JWT_SECRET` set to a placeholder that is also printed in the manual. The manual tells users to change it (Linux: "optional but recommended"). An installation that keeps it -signs logins with a publicly known secret: anyone who knows it can mint a token for any user -id, including SuperUser, as far as the signature goes. What else such a forged token needs -(the per-user latest-token check, `CurrentAuthToken`) is to be confirmed in pass 2, as it may -limit this considerably. Hosted droplets are not affected (the setup script generates a -random secret since `64d8f62`; droplets created earlier: to check). Fix options: generate a -random secret at install time; refuse to boot with the placeholder; or treat the placeholder -as unset (random per boot). +signs logins with a publicly known secret. + +**Pass 2: the latest-token check does not save it.** Tokens are deterministic: the payload is +only `exp` (login time + 5 days, in seconds), `iss` and `id`, with no random part +(`AuthController.cs:280-291`); two logins in the same second give identical tokens. So with +the secret, someone with no login can rebuild a user's *current* token by trying each second +of the last 5 days: at most 432,000 requests per user, each rejected at once (no delay). It +works for any user who logged in in the last 5 days, SuperUser included. **Proof**: with the +local dev secret, a forged token for `Inventory` was accepted after 6 tries and was identical +to the real one. In addition, routes ending in `/auth` (F8) accept a forged token without +any search, and behind a reverse proxy on the same machine that does not set +`X-Forwarded-For`, a forged token with an `rpl` claim and `Host: 127.0.0.1` skips the check +too (`IsLocalExtension`: remote address equals local address). + +Hosted droplets set up with the current script are not affected (random secret since +`64d8f62`); **droplets created before that: to check (John)**. Self-hosted IIS and Linux +servers that are reachable from the internet and kept the placeholder are fully open. + +Fix options: treat the placeholder (and any secret shorter than 32 characters) as unset, so +a random secret is used per boot, and log a warning (users would log in again after each +restart); or generate a random secret at install time (Windows installer, Linux packages); +or refuse to boot. Adding a random value (`jti`) to the token is cheap hardening as well. + +### F6 No lockout or rate limit on login and 2FA (pass 2) + +`POST auth` and `POST auth/tfa-authenticate` delay 3 s on failure but count nothing: no +per-user or per-address lockout, and parallel requests are not slowed. Hosted nginx has no +`limit_req`. There is no password rule beyond "not empty" (seeded trial users use the login +as password). Fix options: count failures per login and per temp token with a short lockout; +`limit_req` on `/api/v8/auth` in the hosted nginx template. + +### F7 Password change does not end sessions; download key loses its expiry (pass 2) + +`UserBiz.ChangePasswordAsync` (used by change-password and reset-password) does not replace +`CurrentAuthToken`, so a session someone else holds keeps working up to 5 days after the +owner changes the password. It also sets `DlKeyExpire = null`, apparently meant to be +`PasswordResetCodeExpire`; with a null expiry `ValidateDownloadTokenAndReturnUserAsync` +never treats the key as expired, until the next login replaces it. Setting `AllowLogin` to +false does not end a session either (only deactivating does). Fix: on password change, set +`CurrentAuthToken` and `DlKey` to new random values and clear `PasswordResetCodeExpire`. + +### F8 Latest-token check bypassed by the URL suffix (pass 2, confirmed) + +The middleware skips the `CurrentAuthToken` comparison when the path ends with `/auth` or +`notify/hello` (`Startup.cs:409-413`, `EndsWith`), meant for the login and pre-login routes. +Any route whose last segment is free text can be called with a replaced or revoked token +(e.g. `tag-list/batch-add/auth`, `form-custom/auth`). Confirmed: a replaced token got E2004 +on `form-field-reference/Customer` and passed the middleware on `form-field-reference/auth`. +The middleware also does not check `Active`. Impact is small on its own (only those routes, +with the user's current roles), larger with F1. Fix: compare the exact route +(`/api/v8/auth`, `/api/v8/notify/hello`), or skip the check by endpoint metadata +(`[AllowAnonymous]`) instead of by path. + +### F9 Default SuperUser password, advertised anonymously (pass 2) + +A new database has SuperUser `superuser` / `l3tm3in` (`PrimeData.cs`). Anonymous +`GET notify/hello` returns `sudf: true` while that is still the password, and the login form +then fills it in. Anyone can therefore check an internet-facing server for it in one request +and log in as SuperUser. Each call also runs a PBKDF2 hash, a cheap way to load the server. +Whether hosted droplets set their own SuperUser password at setup: to check in pass 10. +Fix options: only return `sudf` from local or LAN addresses, or force a password change on +first SuperUser login; at least warn in the UI and the manual. + +### F10 Anonymous licence-agreement consent (pass 2) + +`POST license/lc` with body `"iaccepttheagreement"` records the EULA consent; the comment says +"SuperUser only", but the route is anonymous. Legal rather than technical: the consent record +does not prove who accepted. Fix: require SuperUser. + +### F11 Anonymous API root page discloses licence details (pass 2) + +`GET /api/v8/` returns an HTML page with the exact version, "Registered to", licence type, +expiry dates and licensed features; unlicensed, the server DB ID. Helps an attacker pick +targets by version. Fix: show it only to logged-in users, or show the version only. + +### F12 Password hashing strength (pass 2) + +PBKDF2-HMAC-SHA512, 10,000 iterations, 32-byte random salt (`Hasher.cs`). Current guidance +(OWASP) is 210,000 iterations for SHA-512. Only matters if the database or a backup leaks. +Raising it needs re-hashing at next login (store the iteration count). Roadmap item, not 8.3.0. ### F2 Windows folder permissions (from the 8.3.0 installer test) @@ -236,3 +347,4 @@ by a customer contact. Pass 8. | Date | Pass | Result | |---|---|---| | 2026-10-23 | 1 Threat model and map | Scope agreed with John. Middleware, JWT validation, role model, download keys, CORS and Swagger read. Route map generated (460 actions; script kept in `security/routemap.py`). No route lacks authentication by accident. 53 logged-in routes have no role check in the action: 3 resolved (erase data, trial seed: OK; direct SMTP: F3), the rest assigned to later passes. Local-request exception not reachable through the hosted nginx. Findings F1 to F4 recorded as candidates | +| 2026-10-23 | 2 Login, tokens, reset, 2FA, anonymous routes | Read `AuthController`, the token middleware, `Hasher`, download-key check, password reset, TOTP setup, and every anonymous route (`notify/hello`, `license/lc`, `translation/subset`, `logo`, API root, health). Tested on the local dev server with the seeded `Inventory` user: **F5 critical, confirmed** (2FA step without password); **F1 confirmed** (current token rebuilt from the secret in 6 tries); F8 confirmed. New: F6, F7, F9 to F12. OK: reset codes (32 random characters, 48 h, cleared on use, same reply for unknown codes), login replies do not reveal whether a login exists, roles read from the database on every request, deactivation ends the session, TOTP secret 160 bits. Leads for pass 4: `AuthController.GenRpt` issues SuperUser (id 1) tokens with an `rpl` claim for report rendering; pass 5: the logo is served with the content type given at upload |