diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index 6750b14e..03e6db51 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -14,8 +14,8 @@ keep it that way while findings are open. | 1 | Threat model and attack-surface map | Done 2026-10-23 | | 2 | Unauthenticated routes, login, tokens, password reset, 2FA | Done 2026-10-23 | | 3 | Customer contact users: what they can reach and read | Done 2026-10-23 | -| 4 | Report rendering (templates, the server-side browser, report data and download routes) | Next | -| 5 | File handling: attachments, uploads, exports, logs, backups, restore, translations, logo | | +| 4 | Report rendering (templates, the server-side browser, report data and download routes) | Done 2026-10-23 | +| 5 | File handling: attachments, uploads, exports, logs, backups, restore, translations, logo | Next | | 6 | Raw SQL: data lists, saved filters, search, KPI, schedule, pick lists | | | 7 | Staff roles: the authenticated routes without a role check in the action (list below) | | | 8 | Email and notifications (`direct-smtp`, templates, delivery) | | @@ -188,12 +188,15 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca |---|---|---|---| | 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 | +| F19 | Critical | **confirmed** (import) | Any logged-in user, customer contacts included, can import report templates, whose JavaScript runs in the server's browser | +| F21 | Critical | candidate | Download file names from the URL are not checked: `report/download` and `export/download` may serve files outside the temp folder on Windows | | F18 | Critical | candidate | Pick-list search text is put into SQL without escaping; any logged-in user can reach it | | F13 | High | **confirmed** | Customer contacts can open any customer's work order (customer view, customer report, attachments) by id | | F14 | High | **confirmed** | Customer contacts can read, and by code change, delete and create, any customer's service requests | | F15 | High | **confirmed** | Customer contacts can list every customer (and vendors, users, parts...) with any pick-list columns, phones and emails included | | F16 | High | **confirmed** | `schedule/svc` and `schedule/user` have no role check: anyone logged in sees the whole service schedule | | F17 | High | candidate | Notification subscriptions accept any `UserId`: a contact could subscribe on a staff user's behalf with delivery to its own email | +| F20 | High | candidate | Report templates receive the renderer's token (SuperUser for customer notification reports) and the report browser can reach any network address | | 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 | @@ -204,6 +207,7 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | 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 | +| F22 | Low | confirmed | Minor report and job route gaps: template export by any download key, job status, logs and cancel by GUID without an owner check, browser without sandbox | **Release decisions.** John, 2026-10-23: **F13 to F17 are urgent and are fixed in 8.3.0**, as "next release" cases (case numbers to come). Hosted and self-hosted perpetual installations @@ -416,6 +420,67 @@ texts as SQL parameters (preferred) or escape them as the data-list builder does rest of the pick-list builder in pass 6. +### F19 Report template import open to every login (pass 4, confirmed) + +`POST report/upload` (`ReportController.UploadAsync`) has `[Authorize]` only, and +`ReportBiz.ImportAsync` checks nothing either; creating or editing a template through the +normal routes needs `Change` on `Report` (BizAdmin, BizAdminRestricted). A template holds +JavaScript (`JsPrerender`, `JsHelpers`) and a Handlebars template, which `ReportBiz` loads into +the headless browser on the server at render time. **Confirmed**: the seeded contact +`Customer` imported a copy of a stock template (202, new report 104, deleted afterwards). + +Consequences (from code; the rendering side was deliberately not tested): +- `report/render-job` requires read rights on `Report` (everyone) and on the template's + record type, which the uploader chooses (a contact can read CSRs), so any login can run + its own JavaScript in the server's browser: it can reach the local API and any address the + server can reach (internal network, cloud metadata endpoints) and put the results into the + PDF it downloads. +- An imported template also appears in staff report lists; when staff or SuperUser render it, + it runs with their token (see F20). + +Fix: in `UploadAsync`, require `HasCreateRole(Report)` like `POST report`; F20's fixes limit +what a template can do even for legitimate authors. + +### F20 What a report template can reach (pass 4, candidate) + +The page gets the renderer's own bearer token in `AYMETA.ayClientMetaData.Authorization` (for +`ayGetFromAPI`/`ayPostToAPI`/`ayPutToAPI`), and nothing restricts the page's network access +(no request interception in `ReportBiz`). So a template's script can send the token of +whoever renders it to any outside address; that token is the user's current session token, +usable from anywhere. Customer notifications with an attached report +(`CoreJobCustomerNotify`) render with `AuthController.GenRpt`: a SuperUser (id 1) token with an +`rpl` claim, accepted from 127.0.0.1 for the render timeout. A template author (including the +"restricted" `BizAdminRestricted` role) can therefore act as SuperUser, or as any colleague who +runs the report. Fix options: intercept page requests (PuppeteerSharp +`SetRequestInterceptionAsync`) and allow only the local API URL and `data:`/`blob:`; render +notification reports as a dedicated low-rights identity rather than SuperUser; consider a +short-lived token limited to read routes for template API calls. + +### F21 File names from the URL in download routes (pass 4, candidate, code reading) + +`report/download/{fileName}` and `export/download/{fileName}` accept any user's download key +and serve `FileUtil.GetFullPathForTemporaryFile(fileName)`, which is +`Path.Combine(TemporaryFilesFolder, fileName)` with no check. On Windows, a backslash in the +route value is not a separator for routing but is one for `Path.Combine`, and an absolute +name replaces the folder entirely; the result is passed to `PhysicalFile`. Any login +(customer contacts included) could then read files outside the temp folder, such as +`config.json` (database password, JWT secret). Linux is likely not affected (`/` stays encoded +as `%2F` in route values); IIS request filtering may block some forms. Not tested live by +choice. The same unchecked pattern is in `log-file/{logname}`, `log-file/download/{logname}` +and the backup download and delete routes (ops roles; pass 5). Fix: accept only a bare file +name (`Path.GetFileName(name) == name`, no `..`), and check the resolved full path is inside +the folder, in one helper used by every such route. + +### F22 Minor report and job gaps (pass 4) + +`report/export/{id}` gives any report template to any valid download key (templates are not +secret, but staff-only templates go to customer contacts). `job-operations/status|progress| +logs/{gid}` and `report/request-cancel/{gid}` check no owner; job ids are random GUIDs, so +this needs a leaked id. The browser runs with `--no-sandbox` (Chrome's own sandbox off), so a +browser bug reached through a template is not contained; Linux runs as `www-data`, Windows LAN +as the logged-in user. Fix: owner check on job routes; consider enabling the sandbox where +the platform allows. + ### F2 Windows folder permissions (from the 8.3.0 installer test) `lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program @@ -489,3 +554,4 @@ roles that should not see the schedule (pass 7). | 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 | | 2026-10-23 | 3 Customer contacts | Mapped the `Customer` role in `BizRoles.cs` and every customer-reachable route. Tested on the local dev server as seeded contact `Customer` (customer 1), read-only: F13, F14 (read), F15, F16 confirmed. F17 and F18 from code (F17 not tested because it sends email; F18 not tested by choice). OK: data lists (server sets the customer filter), KPIs (no customer roles), saved filters and notification subscription reads (scoped to the user), memos (no customer access) | +| 2026-10-23 | 4 Report rendering | Read `ReportController`, `ReportBiz` (render path, data, browser launch), `ay-report.js` helpers, `CoreJobCustomerNotify` and `AuthController.GenRpt`. Tested on the local dev server: **F19 confirmed** (contact imported a stock template copy, deleted afterwards). F20, F21 and F22 from code; rendering of a crafted template and the download path were deliberately not tested. OK: `report/data` and `render-job` check read rights on the record type; template API calls use the renderer's own token (except notification reports, F20); report output files have random names. Pass 5 picks up F21's other routes |