159 lines
12 KiB
Markdown
159 lines
12 KiB
Markdown
# PiCloud Security Audit — External Integrations
|
||
|
||
**Scope:** Outbound HTTP from scripts (`http::*`), inbound email webhooks, outbound SMTP, third-party calls (SSRF / replay / auth), Postgres connection, Caddy admin, cluster RPC, `pic` CLI ↔ Manager, auto-HTTPS.
|
||
**Tree state:** `main` at v1.1.9, commit `05ed9b0`.
|
||
**Method:** End-to-end trace of `http::get(url)` from Rhai bridge → `HttpServiceImpl::request` → `reqwest::Client` (with `dns_resolver(SsrfResolver)`); plus targeted review of email-inbound HMAC, email-outbound SMTP, CLI TLS, and infra topology.
|
||
|
||
## Counts
|
||
|
||
| Severity | Count |
|
||
|---|---|
|
||
| Critical | 0 |
|
||
| High | 1 |
|
||
| Medium | 3 |
|
||
| Low | 3 |
|
||
| Info | 4 |
|
||
|
||
The SSRF defense is unusually thorough for an MVP — full deny-list, hostname-vs-literal-IP split, resolver runs at every connect (including redirect hops), DNS-rebinding tested, cross-origin `Authorization` scrub tested. The remaining material gap is operational: **scripts can send unlimited outbound email through the operator's SMTP relay** because the only token-bucket lives in `users_service` (account-flow emails), not in `EmailServiceImpl`.
|
||
|
||
---
|
||
|
||
## High
|
||
|
||
### F-EXT-H-001 — `email::send` from scripts is not rate-limited; operator's SMTP relay is a free amplifier
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/manager-core/src/email_service.rs`
|
||
|
||
`EmailServiceImpl::send` (lines 257–271) checks authz, size cap, and address validity — **no rate limit, no per-app daily cap, no recipient count cap**. A token-bucket *does* exist (`/home/fabi/PiCloud/crates/manager-core/src/users_service.rs:139–216`, `EmailRateLimiter`) but it is wired only into the user-account flows (verification, password reset). Scripts calling `email::send(...)` through the SDK never traverse `users_service` — they hit `EmailServiceImpl` directly.
|
||
|
||
A public, anonymous HTTP-triggered script with the `AppEmailSend` capability granted to the script (the default for the `Editor` role on the app) can therefore:
|
||
|
||
- Send any number of mails per second (up to the global script-concurrency cap of 32).
|
||
- Include any number of `to:` / `cc:` / `bcc:` recipients per message — `build_message` (lines 276–321) iterates the lists unbounded.
|
||
- Reuse the operator's SMTP credentials (`PICLOUD_SMTP_*`) for spam, abuse, or relay quota exhaustion. The provider's reputation is the operator's, not the script's.
|
||
|
||
Combined with no per-message recipient cap, one request can fan out to thousands of `bcc:` addresses ("BCC bomb"). The SMTP relay will eventually rate-limit, but the abuse will burn through the operator's daily allowance first.
|
||
|
||
**Fix:**
|
||
1. Move `EmailRateLimiter` (or a sibling instance) into `EmailServiceImpl`. Apply it inside `send` before transport handoff. Per-app daily cap **must remain per-app, not global** (agent 8's note in the scope was correct).
|
||
2. Add a per-message recipient cap: `to.len() + cc.len() + bcc.len() <= MAX_RECIPIENTS_PER_MESSAGE` (e.g. 50, env-overridable).
|
||
3. Surface as `EmailError::RateLimited` / `EmailError::TooManyRecipients` so scripts can handle it.
|
||
|
||
**Severity rationale:** This is one capability away from "the platform sends spam in the operator's name." Reputation damage + cost amplification. Bumped to High (not Critical) because `AppEmailSend` is gated on app membership for authed principals — but the moment a script is anonymous-HTTP-exposed and grants itself email, the gate evaporates (the service skips authz when `cx.principal == None`).
|
||
|
||
---
|
||
|
||
## Medium
|
||
|
||
### F-EXT-M-001 — Email-inbound webhook accepts unsigned POSTs when `inbound_secret` is `None`
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/manager-core/src/email_inbound_api.rs:149–156`
|
||
|
||
```rust
|
||
if let (Some(ct), Some(nonce)) = (
|
||
target.inbound_secret_encrypted.as_ref(),
|
||
target.inbound_secret_nonce.as_ref(),
|
||
) {
|
||
let secret = decrypt_secret(&s.master_key, ct, nonce)?;
|
||
verify_signature(&headers, &body, secret.as_bytes(), &s.nonce_dedup)?;
|
||
}
|
||
```
|
||
|
||
If the trigger row has no `inbound_secret`, **signature verification is silently skipped**. Anyone who guesses or scrapes the trigger UUID can POST arbitrary email events into the outbox, which the dispatcher executes. The trigger UUID is a UUIDv7-ish secret but it leaks in admin API logs, dashboard URLs, and Mailgun/Postmark provider dashboards.
|
||
|
||
Agent 8 already flagged this as "the receiver accepts unsigned" — confirming here from the protocol shape. The remediation is to **make `inbound_secret` mandatory at trigger creation** (`triggers_api.rs:589`) or to refuse to mount the route unless a secret is present.
|
||
|
||
The replay protection (F-S-010) is correctly closed: `(ts, sha256(body))` LRU with 600 s TTL, 300 s tolerance window, constant-time `mac.verify_slice`. Signature is HMAC-SHA256 over `ts || "." || body`. All good — but only when a secret exists.
|
||
|
||
### F-EXT-M-002 — Inbound webhook nonce dedup is process-local; cluster mode (v1.3+) will silently degrade
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/manager-core/src/email_inbound_api.rs:67–89`
|
||
|
||
`InboundNonceDedup` is a `Mutex<HashMap>` in the process address space. In single-node MVP this is fine. In cluster mode with multiple manager instances behind Caddy, a replayed POST can land on a different node and the dedup misses. The replay window is short (~300 s) so impact is limited, but the comment at line 97–99 (`"Per-process is fine for single-node MVP and dev"`) acknowledges the gap. Flag now so it isn't forgotten when cluster mode lands.
|
||
|
||
**Fix when cluster ships:** push the dedup tuple into a Postgres table with a `(trigger_id, ts, body_hash) PRIMARY KEY` and a 10-minute partial cleanup job, or into Redis.
|
||
|
||
### F-EXT-M-003 — `PICLOUD_SMTP_PASSWORD` lives in an env-derived `String` and stays in memory for process lifetime
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/manager-core/src/email_service.rs:104, 175`
|
||
|
||
`SmtpConfig::password: String` is read from env and cloned into `Credentials::new(user, password)` inside `LettreEmailTransport::build`. Lettre's `Credentials` does not zero-on-drop. A heap dump (e.g. core dump shipped to a bug tracker) would expose the relay password.
|
||
|
||
This is acceptable for the operator-provided env-var model, but worth noting: the same secret-handling pattern as `users.password_hash` should eventually use `secrecy::SecretString` or equivalent to at least redact in `Debug`. `SmtpConfig` *does* derive `Debug` (line 87), which means `tracing::debug!` of the struct anywhere would print the password verbatim. Searched — no current call site prints it, but the foot-gun is loaded.
|
||
|
||
**Fix:** wrap `password` in `secrecy::SecretString` (already in tree for `MasterKey`), drop the `Debug` derive from `SmtpConfig`, or implement `Debug` manually with `[REDACTED]`.
|
||
|
||
---
|
||
|
||
## Low
|
||
|
||
### F-EXT-L-001 — Bounded set of opt-in HTTP options is good; `script_id` flows to outbound `User-Agent`
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/manager-core/src/http_service.rs:371–380`
|
||
|
||
Default UA is `picloud/<version> (script:<script_id>)`. Operator may not want to expose script IDs to third parties (e.g. for telemetry minimization). The script_id is a UUID — not directly sensitive, but it does fingerprint the platform. Consider making the UA opaque (`picloud/<version>`) in prod or stripping the `(script:...)` segment behind a config flag.
|
||
|
||
### F-EXT-L-002 — `validate_url` blocks ports 22/25/465/587 but not 23/110/143/993/995/3306/6379
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/manager-core/src/http_service.rs:343–345`
|
||
|
||
```rust
|
||
if matches!(port, 22 | 25 | 465 | 587) {
|
||
return Err(HttpError::BlockedPort(port));
|
||
}
|
||
```
|
||
|
||
The deny-list covers SSH + the canonical SMTP family. It misses telnet (23), POP3 (110/995), IMAP (143/993), MySQL (3306), Redis (6379), MongoDB (27017), memcached (11211). All of these are reachable from a script if the target's IP is public — typical for the cloud-VPC case where an internal service is on a public IP but firewalled.
|
||
|
||
The SSRF resolver handles the private-IP case, so the practical exploit is narrow: a public IP running e.g. an exposed Redis on 6379. Still, an allow-list (only 80/443/8080-8090/HTTPS-ALPN-ports) would be safer than the current deny-list. Mark as Low because the realistic exploit requires a misconfigured target.
|
||
|
||
### F-EXT-L-003 — Postgres connection: `DATABASE_URL` is required but `sslmode` is unset by default
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/picloud/src/lib.rs:607–618`
|
||
|
||
`PgPoolOptions::new().connect(url)` honors whatever `sslmode=` is in `DATABASE_URL`. Default for sqlx/Postgres is `prefer` — it tries TLS, falls back to plaintext silently. In the dev docker-compose Postgres is bound to a private network, so plaintext is fine — but in a real prod deploy where Postgres lives on a managed service, an operator who forgets `?sslmode=require` may transmit credentials cleartext on the LAN.
|
||
|
||
**Fix (doc, not code):** the production docker-compose / deploy guide should call this out. Optionally: parse `DATABASE_URL` at startup and warn if `sslmode` is missing or `disable`/`prefer` when host is not `localhost`/`postgres`/`127.0.0.1`.
|
||
|
||
---
|
||
|
||
## Info
|
||
|
||
### F-EXT-I-001 — SSRF defense is solid: confirmed end-to-end trace
|
||
|
||
Traced `http::get("http://169.254.169.254/")` from Rhai:
|
||
|
||
1. `sdk/http.rs:78` → `invoke(...)` builds `HttpRequest { url: "http://169.254.169.254/", ... }`.
|
||
2. `block_on` (line 383) → `HttpServiceImpl::request` (`http_service.rs:166`).
|
||
3. → `run` (line 210) → `validate_url` (line 217) parses host as `Host::Ipv4`, calls `policy.check(IpAddr::V4(169.254.169.254))`.
|
||
4. `ssrf.rs:check_v4` line 77: `[169, 254, ..] => Err("link-local")`.
|
||
5. Returned to script as `HttpError::Ssrf("link-local")`.
|
||
|
||
For hostnames (e.g. `metadata.google.internal`), `validate_url` passes through (it's a `Host::Domain`), the resolver in `ssrf.rs:181-220` does the lookup, sees the resolved IP is 169.254.x.x, filters it out, and if all addresses are denied, returns the SSRF marker error which `map_reqwest_err` (line 401) → `ssrf_reason` (line 419) walks the chain for. **Tested at `http_service.rs:732, 750` with both literal-IP and hostname-via-`localhost` paths.**
|
||
|
||
Redirect hops re-resolve through the same DNS resolver (because `redirect(Policy::none())` + manual follow at line 225–280 re-creates each request through the same client), so redirect-to-SSRF is blocked. Cross-origin `Authorization` scrub is implemented at lines 261–265 and tested at lines 868–892. DNS-rebinding is tested at `ssrf.rs:411–443`.
|
||
|
||
### F-EXT-I-002 — Cluster-mode RPC: no inter-service auth today
|
||
|
||
The three split binaries (`picloud-manager`, `picloud-orchestrator`, `picloud-executor`) are skeletons. The `ExecutorClient` trait abstraction is in place for the swap to remote HTTP, but cluster mode itself is unbuilt. Authentication between services is a v1.3+ design problem; nothing to flag at this stage beyond "don't ship cluster without it."
|
||
|
||
### F-EXT-I-003 — Caddy admin: dev has `admin off`, prod uses Caddy's localhost-only default
|
||
|
||
**Files:** `caddy/Caddyfile:20`, `caddy/Caddyfile.prod` (no `admin` block).
|
||
|
||
Dev caddyfile sets `admin off` — closed. Prod inherits Caddy's default `admin localhost:2019`. As long as the container's port 2019 is not published in `docker-compose.prod.yml` (verified: only the front-facing 80/443 are exposed), the admin API is unreachable from the LAN. Good. Worth adding `admin off` to the prod caddyfile too, for defense in depth.
|
||
|
||
### F-EXT-I-004 — `pic` CLI uses default rustls TLS; no `--insecure` flag exists
|
||
|
||
**File:** `/home/fabi/PiCloud/crates/picloud-cli/src/client.rs:33–43`
|
||
|
||
`reqwest::Client::builder()` with no `danger_accept_invalid_certs` call. Bearer token transmitted in `Authorization` header (line 53), never URL. Audit-clean.
|
||
|
||
---
|
||
|
||
## What was NOT found / out of scope
|
||
|
||
- **HTTP-async (`http_async::request`)**: the scope brief mentioned this surface, but a workspace-wide search for `http_async` / `HttpAsync` returned zero hits — the dispatcher (`dispatcher.rs`) does not invoke any HTTP client and there's no `http_async` SDK module. Either the feature is deferred (post-v1.1.9) or already collapsed into the same `http_service` path — both are fine.
|
||
- **Bounce handling**: no bounce-processing code exists. Nothing to leak.
|
||
- **SMTP injection via CRLF**: agent 4's scope. Not re-audited here.
|