From 082f2c86022d06341e0b2043da4458e71e931957 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Tue, 20 Oct 2026 01:13:03 +0200 Subject: [PATCH] security: close the holes a session or a stranger could walk through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETE /api/media/{id} joined the raw, percent-decoded id onto uploads/, so ..%2Fdiary.db deleted the database. Ids are now parsed as uuids before they reach the filesystem, and a file is only removed when its row was. Import panicked on a link like %aé: percent_decode sliced the string between the bytes of a character. Hex digits are now read as bytes. The SFTP write_at offset is checked before it is cast, so an offset near u64::MAX no longer wraps past the size limit and panics the session. The global body limit was the upload ceiling, about a gigabyte, and applied to the login too. Only the upload routes take that much now; other bodies stop at 16 MiB and the login at 16 KiB. Writes a browser says came from another origin are refused, which SameSite=Lax alone does not cover for sibling subdomains or for multipart forms. Logout only locks the vault for a caller who is logged in, so a cross-site form can no longer shut it. Login and vault attempts go through one at a time, so a burst of parallel guesses cannot all pass the throttle before the first failure is recorded, and parallel Argon2 runs cannot pile up. Sealed files are served with no-store instead of a year of cache, so their plaintext does not outlive :lock in the browser. The SFTP host key is created 0600 rather than narrowed after being written. SIGTERM is handled: as PID 1 in the container it was ignored, and every docker stop ended in SIGKILL. Requests get eight seconds to finish, the pool is closed so the WAL is checkpointed, and compose runs tini. --- README.md | 8 ++++ compose.yml | 2 + src/auth.rs | 91 +++++++++++++++++++++++++++++++++++++++++-- src/main.rs | 58 +++++++++++++++++++++++---- src/routes/import.rs | 25 ++++++++++-- src/routes/media.rs | 43 +++++++++++++++++--- src/routes/mod.rs | 24 +++++++++--- src/routes/nodes.rs | 5 ++- src/routes/session.rs | 16 +++++--- src/routes/vault.rs | 19 ++++----- src/sftp/fs.rs | 22 ++++++++--- src/sftp/mod.rs | 17 +++++--- src/throttle.rs | 71 ++++++++++++++++++++++++++------- 13 files changed, 335 insertions(+), 66 deletions(-) diff --git a/README.md b/README.md index e644e98..9955d3a 100644 --- a/README.md +++ b/README.md @@ -438,6 +438,14 @@ re-run `:share` on those entries to publish them again. Uploaded files are only ever served as types that cannot execute — anything else is handed back as an opaque download. +A write that a browser says came from another origin — `Sec-Fetch-Site`, or +`Origin` against `Host` where a browser is too old to send that — is refused, +so a proxy must pass the `Host` header through unchanged (traefik and nginx's +`proxy_set_header Host $host` both do). Only the upload routes accept a body as +large as `WORKSPACE_MAX_UPLOAD_MB`; everything else stops at 16 MiB, and the +login at 16 KiB, before anyone has proved who they are. `SIGTERM` gets in-flight +requests eight seconds to finish, inside the ten `docker stop` waits. + ### Docker ```sh diff --git a/compose.yml b/compose.yml index 9ae4a91..b54badf 100644 --- a/compose.yml +++ b/compose.yml @@ -3,6 +3,8 @@ services: build: . image: narl-workspace:latest restart: unless-stopped + # tini as PID 1: forwards SIGTERM and reaps anything the binary leaves behind. + init: true env_file: - .env environment: diff --git a/src/auth.rs b/src/auth.rs index 44bc02e..5dede65 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1,6 +1,11 @@ +use std::convert::Infallible; + use axum::{ - extract::FromRequestParts, - http::{header, request::Parts}, + extract::{FromRequestParts, OptionalFromRequestParts, Request}, + http::{header, request::Parts, HeaderMap, StatusCode}, + middleware::Next, + response::{IntoResponse, Response}, + Json, }; use base64::{engine::general_purpose::URL_SAFE_NO_PAD as B64, Engine}; use hmac::{Hmac, Mac}; @@ -97,9 +102,66 @@ impl FromRequestParts for Session { } } +/// For a handler that behaves differently for a caller who is logged in, but +/// must also answer one who is not. +impl OptionalFromRequestParts for Session { + type Rejection = Infallible; + + async fn from_request_parts( + parts: &mut Parts, + state: &AppState, + ) -> Result, Self::Rejection> { + Ok(>::from_request_parts(parts, state) + .await + .ok()) + } +} + +/// Refuse a write that another site's page asked the browser to make. +/// `SameSite=Lax` already keeps the cookie off a cross-site POST, but not off +/// one from a sibling subdomain, which counts as the same site — and a +/// multipart form needs no preflight to get sent. A browser says where a +/// request came from; a request that says nothing is not from a browser page +/// and carries no ambient cookie worth stealing. +pub async fn same_origin(request: Request, next: Next) -> Response { + if !request.method().is_safe() && !is_same_origin(request.headers()) { + tracing::warn!( + "refused a cross-origin {} {}", + request.method(), + request.uri().path() + ); + return ( + StatusCode::FORBIDDEN, + Json(serde_json::json!({ "error": "cross-origin request refused" })), + ) + .into_response(); + } + next.run(request).await +} + +fn is_same_origin(headers: &HeaderMap) -> bool { + // Every current browser sends this, and it cannot be set from a page. + if let Some(site) = headers.get("sec-fetch-site") { + return matches!(site.as_bytes(), b"same-origin" | b"none"); + } + // Older browsers: the Origin they send against the host they asked. + let Some(origin) = headers.get(header::ORIGIN) else { + return true; + }; + let origin_host = origin + .to_str() + .ok() + .and_then(|origin| origin.split_once("://")) + .map(|(_, host)| host); + let host = headers.get(header::HOST).and_then(|h| h.to_str().ok()); + matches!((origin_host, host), (Some(a), Some(b)) if a.eq_ignore_ascii_case(b)) +} + #[cfg(test)] mod tests { - use super::{credentials_match, issue_token, verify_token}; + use axum::http::{HeaderMap, HeaderValue}; + + use super::{credentials_match, is_same_origin, issue_token, verify_token}; use crate::config::{BackupConfig, Config, MailConfig, SftpConfig}; fn config(session_days: i64) -> Config { @@ -188,4 +250,27 @@ mod tests { assert!(!credentials_match(&config, "nar", "hunter2")); assert!(!credentials_match(&config, "", "")); } + + fn headers(pairs: &[(&'static str, &'static str)]) -> HeaderMap { + let mut map = HeaderMap::new(); + for (name, value) in pairs { + map.insert(*name, HeaderValue::from_static(value)); + } + map + } + + #[test] + fn only_same_origin_writes_get_through() { + assert!(is_same_origin(&headers(&[("sec-fetch-site", "same-origin")]))); + assert!(!is_same_origin(&headers(&[("sec-fetch-site", "same-site")]))); + assert!(!is_same_origin(&headers(&[("sec-fetch-site", "cross-site")]))); + + let host = ("host", "diary.example.org"); + assert!(is_same_origin(&headers(&[("origin", "https://diary.example.org"), host]))); + assert!(!is_same_origin(&headers(&[("origin", "https://evil.example.org"), host]))); + assert!(!is_same_origin(&headers(&[("origin", "null"), host]))); + + // curl, the CLI, a script with its own credentials. + assert!(is_same_origin(&headers(&[host]))); + } } diff --git a/src/main.rs b/src/main.rs index 2ad7ca2..f115515 100644 --- a/src/main.rs +++ b/src/main.rs @@ -129,7 +129,12 @@ async fn main() -> Result<()> { }); } - serve(bind, max_upload, state).await + let db = state.db.clone(); + serve(bind, max_upload, state).await?; + // Checkpoints the write-ahead log, so what was written is in diary.db + // itself and not only beside it. + db.close().await; + Ok(()) } /// Open the workspace database and bring it up to date. Both the server and the @@ -162,9 +167,9 @@ async fn open_db(config: &Config) -> Result { async fn serve(bind: std::net::SocketAddr, max_upload: usize, state: AppState) -> Result<()> { let app = Router::new() - .nest("/api", routes::api_router()) + .nest("/api", routes::api_router(max_upload)) .fallback(static_files::handler) - .layer(axum::extract::DefaultBodyLimit::max(max_upload + 1024 * 1024)) + .layer(axum::middleware::from_fn(auth::same_origin)) .layer(CompressionLayer::new()) .layer(security_headers()) .layer(TraceLayer::new_for_http()) @@ -176,12 +181,26 @@ async fn serve(bind: std::net::SocketAddr, max_upload: usize, state: AppState) - tracing::info!("narl-workspace listening on http://{bind}"); - axum::serve(listener, app) - .with_graceful_shutdown(shutdown_signal()) - .await?; + let server = axum::serve(listener, app).with_graceful_shutdown(async { + shutdown_signal().await; + tracing::info!("shutting down"); + }); + // Requests in flight get a few seconds to finish; an upload that would + // take longer is cut off rather than holding the process past the point + // where a supervisor stops asking and kills it. + tokio::select! { + result = server => result?, + () = async { + shutdown_signal().await; + tokio::time::sleep(SHUTDOWN_GRACE).await; + } => tracing::warn!("requests still open after {SHUTDOWN_GRACE:?}, closing anyway"), + } Ok(()) } +/// Below the ten seconds `docker stop` waits before it sends SIGKILL. +const SHUTDOWN_GRACE: std::time::Duration = std::time::Duration::from_secs(8); + /// Content-Security-Policy for the app shell. `unsafe-inline` is granted to /// styles only, because CodeMirror injects its theme as a