From ab9f1d89b2b094fa8d8b92e1d7df0404ce8686e1 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 27 Jun 2026 15:19:34 +0200 Subject: [PATCH] fix(upload): server-side MIME/ext allowlist + atomic transactional write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C2: stored-XSS via the application/* MIME bypass is closed — the declared Content-Type and client filename are no longer trusted. New classify() derives both the canonical MIME and a safe extension purely from magic bytes against a strict allowlist (jpeg/png/webp/heic/mp4/mov/webm); anything infer can't positively identify (incl. HTML/SVG script payloads) is rejected. Unit tests cover accept + reject paths. M12: the stored extension can no longer carry '/' or arbitrary client text — it's one of the allowlist's safe constants, ending directory-pollution and the export DoS. M5/M6: quota is now reserved with a single conditional UPDATE that row-locks (no TOCTOU overshoot) and the reservation + row INSERT run in one transaction; the file is written before commit and unlinked on any rollback, so a crash mid-write can no longer leak quota bytes or orphan a row. Upload::create is now executor-generic to run inside the tx. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/src/handlers/upload.rs | 222 +++++++++++++++++++++++---------- backend/src/models/upload.rs | 14 ++- 2 files changed, 168 insertions(+), 68 deletions(-) diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index a0fed5d..add61d4 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -16,6 +16,35 @@ use crate::state::AppState; const MAX_CAPTION_LENGTH: usize = 2000; +/// Derive the canonical MIME type and a *safe* file extension purely from the +/// file's magic bytes, against a strict allowlist. The client-declared +/// `Content-Type` and the client filename are never trusted: the old code +/// skipped validation entirely for `application/*` and copied the extension +/// verbatim from the filename, which allowed storing `{uuid}.html`/`.svg` with +/// a script payload (stored XSS) and `/`-bearing extensions (path pollution + +/// export DoS). +/// +/// `infer` cannot fingerprint HTML/SVG, so the reject-by-default arm is exactly +/// what closes the XSS vector — anything not positively identified as an allowed +/// raster image or video is refused. Returns `(canonical_mime, safe_ext, is_video)`. +fn classify(data: &[u8]) -> Result<(&'static str, &'static str, bool), AppError> { + let kind = infer::get(data) + .ok_or_else(|| AppError::BadRequest("Dateityp konnte nicht erkannt werden.".into()))?; + match kind.mime_type() { + "image/jpeg" => Ok(("image/jpeg", "jpg", false)), + "image/png" => Ok(("image/png", "png", false)), + "image/webp" => Ok(("image/webp", "webp", false)), + // infer reports HEIC/HEIF as image/heif + "image/heif" | "image/heic" => Ok(("image/heic", "heic", false)), + "video/mp4" => Ok(("video/mp4", "mp4", true)), + "video/quicktime" => Ok(("video/quicktime", "mov", true)), + "video/webm" => Ok(("video/webm", "webm", true)), + other => Err(AppError::BadRequest(format!( + "Dateityp nicht erlaubt: {other}. Erlaubt sind JPEG, PNG, WebP, HEIC, MP4, MOV, WebM." + ))), + } +} + pub async fn upload( State(state): State, auth: AuthUser, @@ -62,8 +91,6 @@ pub async fn upload( let max_video_mb: i64 = config::get_i64(&state.pool, "max_video_size_mb", 500).await; let mut file_data: Option> = None; - let mut file_name: Option = None; - let mut content_type: Option = None; let mut caption: Option = None; let mut hashtags_csv: Option = None; @@ -71,8 +98,9 @@ pub async fn upload( let name = field.name().unwrap_or_default().to_string(); match name.as_str() { "file" => { - file_name = field.file_name().map(|s| s.to_string()); - content_type = field.content_type().map(|s| s.to_string()); + // The client-declared filename and Content-Type are deliberately + // ignored — the stored MIME and extension are derived from the + // file's magic bytes (see `classify`). file_data = Some( field.bytes().await .map_err(|e| AppError::BadRequest(format!("Datei konnte nicht gelesen werden: {e}")))? @@ -96,7 +124,6 @@ pub async fn upload( } let data = file_data.ok_or_else(|| AppError::BadRequest("Keine Datei hochgeladen.".into()))?; - let mime = content_type.unwrap_or_else(|| "application/octet-stream".to_string()); let size = data.len() as i64; // Validate caption length. Counted in chars (code points) to match the @@ -111,25 +138,13 @@ pub async fn upload( } } - // Validate file MIME type using magic bytes - let detected_mime = infer::get(&data); - if let Some(detected) = detected_mime { - let detected_type = detected.mime_type(); - // Ensure detected type is compatible with declared MIME type - let declared_category = mime.split('/').next().unwrap_or(""); - let detected_category = detected_type.split('/').next().unwrap_or(""); + // Derive canonical MIME + safe extension from magic bytes against a strict + // allowlist. The client Content-Type and filename are never trusted. + let (canonical_mime, safe_ext, is_video) = classify(&data)?; + let canonical_mime = canonical_mime.to_string(); - // Only reject if categories don't match (e.g., image vs video) - if declared_category != "application" && declared_category != detected_category { - return Err(AppError::BadRequest(format!( - "Dateiinhalt entspricht nicht dem deklarierten Typ. Erwartet: {}, erkannt: {}", - mime, detected_type - ))); - } - } - - // Validate file size - let max_bytes = if mime.starts_with("video/") { + // Validate file size against the per-category limit. + let max_bytes = if is_video { max_video_mb * 1024 * 1024 } else { max_image_mb * 1024 * 1024 @@ -141,59 +156,100 @@ pub async fn upload( ))); } - // Per-user storage quota — dynamic formula based on available disk space and the - // number of active uploaders. Gated by master + per-area toggles so the admin can - // disable it on trusted instances. - let quota_on = config::get_bool(&state.pool, "quota_enabled", true).await; - let storage_quota_on = config::get_bool(&state.pool, "storage_quota_enabled", true).await; - if quota_on && storage_quota_on { - let estimate = compute_storage_quota(&state).await; - if let Some(limit) = estimate.limit_bytes { - let prospective_total = user.total_upload_bytes.saturating_add(size); - if prospective_total > limit { - return Err(AppError::TooManyRequests( - "Du hast dein Upload-Limit für dieses Event erreicht.".into(), - None, - )); - } - } - } - - // Determine file extension - let ext = file_name - .as_deref() - .and_then(|n| n.rsplit('.').next()) - .unwrap_or(if mime.starts_with("video/") { "mp4" } else { "jpg" }); - let upload_id = Uuid::new_v4(); let event_slug = &state.config.event_slug; - let relative_path = format!("originals/{event_slug}/{upload_id}.{ext}"); + let relative_path = format!("originals/{event_slug}/{upload_id}.{safe_ext}"); let absolute_path = state.config.media_path.join(&relative_path); - // Ensure directory exists and write file - if let Some(parent) = absolute_path.parent() { - tokio::fs::create_dir_all(parent).await.map_err(|e| AppError::Internal(e.into()))?; - } - tokio::fs::write(&absolute_path, &data).await.map_err(|e| AppError::Internal(e.into()))?; + // Per-user storage quota — dynamic formula based on available disk space and the + // number of active uploaders. Gated by master + per-area toggles so the admin can + // disable it on trusted instances. `None` ⇒ enforcement off. + let quota_on = config::get_bool(&state.pool, "quota_enabled", true).await; + let storage_quota_on = config::get_bool(&state.pool, "storage_quota_enabled", true).await; + let quota_limit: Option = if quota_on && storage_quota_on { + compute_storage_quota(&state).await.limit_bytes + } else { + None + }; - // Update user's total upload bytes - sqlx::query("UPDATE \"user\" SET total_upload_bytes = total_upload_bytes + $2 WHERE id = $1") + // Reserve quota and insert the row in a single transaction so the byte + // counter and the upload row are atomic (M6: no quota leak if we crash + // between them) and the reservation is a conditional UPDATE that row-locks + // (M5: concurrent uploads can no longer all pass a stale snapshot and + // overshoot the limit). The file is written before commit and unlinked on + // any rollback so a failed transaction leaves nothing behind. + let mut tx = state.pool.begin().await?; + + let reserved: Option<(Uuid,)> = if let Some(limit) = quota_limit { + sqlx::query_as( + "UPDATE \"user\" + SET total_upload_bytes = total_upload_bytes + $2 + WHERE id = $1 AND total_upload_bytes + $2 <= $3 + RETURNING id", + ) .bind(auth.user_id) .bind(size) - .execute(&state.pool) - .await?; + .bind(limit) + .fetch_optional(&mut *tx) + .await? + } else { + sqlx::query_as( + "UPDATE \"user\" + SET total_upload_bytes = total_upload_bytes + $2 + WHERE id = $1 + RETURNING id", + ) + .bind(auth.user_id) + .bind(size) + .fetch_optional(&mut *tx) + .await? + }; + if reserved.is_none() { + // The user exists (checked above), so a missing row here means the + // conditional limit guard rejected the reservation → over quota. + tx.rollback().await.ok(); + return Err(AppError::TooManyRequests( + "Du hast dein Upload-Limit für dieses Event erreicht.".into(), + None, + )); + } - // Insert upload record - let upload = Upload::create( - &state.pool, + // Write the file. On any failure, roll back (releasing the reservation) and + // remove a possibly-partial file. + if let Some(parent) = absolute_path.parent() { + if let Err(e) = tokio::fs::create_dir_all(parent).await { + tx.rollback().await.ok(); + return Err(AppError::Internal(e.into())); + } + } + if let Err(e) = tokio::fs::write(&absolute_path, &data).await { + tx.rollback().await.ok(); + return Err(AppError::Internal(e.into())); + } + + let upload = match Upload::create( + &mut *tx, auth.event_id, auth.user_id, &relative_path, - &mime, + &canonical_mime, size, caption.as_deref(), ) - .await?; + .await + { + Ok(u) => u, + Err(e) => { + tx.rollback().await.ok(); + let _ = tokio::fs::remove_file(&absolute_path).await; + return Err(e.into()); + } + }; + + if let Err(e) = tx.commit().await { + let _ = tokio::fs::remove_file(&absolute_path).await; + return Err(e.into()); + } // Process hashtags from caption and explicit CSV let mut tags: Vec = Vec::new(); @@ -219,7 +275,7 @@ pub async fn upload( // Spawn compression task state .compression - .process(upload.id, relative_path, mime.clone()); + .process(upload.id, relative_path, canonical_mime.clone()); // Broadcast SSE event let dto = UploadDto { @@ -228,7 +284,7 @@ pub async fn upload( uploader_name: user.display_name, preview_url: None, thumbnail_url: None, - mime_type: mime, + mime_type: canonical_mime, caption, hashtags: tags, like_count: 0, @@ -411,3 +467,41 @@ pub async fn get_original( .body(Body::from_stream(stream)) .map_err(|e| AppError::Internal(e.into())) } + +#[cfg(test)] +mod tests { + use super::classify; + + #[test] + fn classify_accepts_allowed_image_types() { + // PNG signature + let png = [0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0, 0, 0, 0]; + let (mime, ext, is_video) = classify(&png).expect("png allowed"); + assert_eq!(mime, "image/png"); + assert_eq!(ext, "png"); + assert!(!is_video); + + // JPEG signature + let jpeg = [0xFF, 0xD8, 0xFF, 0xE0, 0, 0, 0, 0, 0, 0, 0, 0]; + let (mime, ext, _) = classify(&jpeg).expect("jpeg allowed"); + assert_eq!(mime, "image/jpeg"); + assert_eq!(ext, "jpg"); + } + + #[test] + fn classify_rejects_unrecognized_and_html_svg() { + // Plain HTML / script payload — infer cannot fingerprint it, so it must + // be rejected (this is the stored-XSS vector the old code let through + // via a declared `application/*` Content-Type). + let html = b""; + assert!(classify(html).is_err()); + + // SVG (text-based) is likewise unrecognized → rejected. + let svg = b""; + assert!(classify(svg).is_err()); + + // Empty / garbage. + assert!(classify(&[]).is_err()); + assert!(classify(&[0x00, 0x01, 0x02, 0x03]).is_err()); + } +} diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index 1b085e1..9dc9d8c 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -36,15 +36,21 @@ pub struct UploadDto { } impl Upload { - pub async fn create( - pool: &PgPool, + /// Generic over the executor so it can run inside the upload transaction + /// (`&mut *tx`) alongside the quota-counter reservation, keeping the + /// byte-counter and the row atomic (no quota leak on a mid-write crash). + pub async fn create<'e, E>( + executor: E, event_id: Uuid, user_id: Uuid, original_path: &str, mime_type: &str, original_size_bytes: i64, caption: Option<&str>, - ) -> Result { + ) -> Result + where + E: sqlx::PgExecutor<'e>, + { sqlx::query_as::<_, Self>( "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption) VALUES ($1, $2, $3, $4, $5, $6) @@ -56,7 +62,7 @@ impl Upload { .bind(mime_type) .bind(original_size_bytes) .bind(caption) - .fetch_one(pool) + .fetch_one(executor) .await }