From 4d81bcbd7b1eaffc964154bed946f8d834b817b5 Mon Sep 17 00:00:00 2001 From: Dionisio Date: Thu, 5 Mar 2026 16:57:44 +0100 Subject: [PATCH] fix(security): V-01,V-02,V-04,V-05,V-06 - multiple vulnerability fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - V-01: CalDAV unbounded body limit → MAX_CALDAV_BODY = 1MB - V-02: CardDAV unbounded body limit → MAX_CARDDAV_BODY = 1MB - V-04: Batch operations without max size → MAX_BATCH_SIZE = 1000 - V-05: WOPI get_editor_url IDOR → authorize_wopi_access with ownership check - V-06: JWT secret without entropy validation → panic <16, warn 16-31, accept >=32 --- src/common/config.rs | 22 ++++- src/interfaces/api/handlers/batch_handler.rs | 93 +++++++++++++++++++ src/interfaces/api/handlers/caldav_handler.rs | 14 ++- .../api/handlers/carddav_handler.rs | 14 ++- src/interfaces/api/handlers/wopi_handler.rs | 70 ++++++++------ 5 files changed, 176 insertions(+), 37 deletions(-) diff --git a/src/common/config.rs b/src/common/config.rs index 83d33501..57d422b1 100644 --- a/src/common/config.rs +++ b/src/common/config.rs @@ -569,10 +569,30 @@ impl AppConfig { // Auth configuration if let Ok(jwt_secret) = env::var("OXICLOUD_JWT_SECRET") { + // SECURITY: Validate JWT secret minimum entropy (RFC 7518 §3.2 + // recommends ≥256 bits for HS256). Panic on dangerously short + // secrets, warn on sub-optimal ones. + let len = jwt_secret.len(); + if config.features.enable_auth && len < 16 { + panic!( + "FATAL: OXICLOUD_JWT_SECRET is dangerously short ({} bytes). \ + Minimum: 32 bytes (256 bits) for HS256. \ + Generate a secure secret with: openssl rand -hex 32", + len + ); + } else if config.features.enable_auth && len < 32 { + tracing::warn!("=========================================================="); + tracing::warn!( + "OXICLOUD_JWT_SECRET is only {} bytes — recommended minimum is 32 (256 bits).", + len + ); + tracing::warn!("Generate a stronger secret with: openssl rand -hex 32"); + tracing::warn!("=========================================================="); + } config.auth.jwt_secret = jwt_secret; } - // SECURITY: Validate JWT secret when auth is enabled + // SECURITY: Generate ephemeral secret when none is provided if config.features.enable_auth && config.auth.jwt_secret.is_empty() { // Generate a random secret for this session and warn loudly use rand_core::{OsRng, RngCore}; diff --git a/src/interfaces/api/handlers/batch_handler.rs b/src/interfaces/api/handlers/batch_handler.rs index fb384f7d..36a3826b 100644 --- a/src/interfaces/api/handlers/batch_handler.rs +++ b/src/interfaces/api/handlers/batch_handler.rs @@ -14,6 +14,10 @@ use crate::application::services::batch_operations::{ use crate::interfaces::api::handlers::ApiResult; use crate::interfaces::middleware::auth::AuthUser; +/// Maximum number of items allowed in a single batch request. +/// Prevents fan-out amplification attacks and database connection exhaustion. +const MAX_BATCH_SIZE: usize = 1_000; + /// Shared state for the batch handler #[derive(Clone)] pub struct BatchHandlerState { @@ -143,6 +147,15 @@ pub async fn move_files_batch( ) .into_response()); } + if request.file_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.file_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Execute batch operation let result = state @@ -187,6 +200,15 @@ pub async fn copy_files_batch( ) .into_response()); } + if request.file_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.file_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Execute batch operation let result = state @@ -231,6 +253,15 @@ pub async fn delete_files_batch( ) .into_response()); } + if request.file_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.file_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Execute batch operation let result = state @@ -283,6 +314,15 @@ pub async fn delete_folders_batch( ) .into_response()); } + if request.folder_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.folder_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Execute batch operation let result = state @@ -335,6 +375,15 @@ pub async fn create_folders_batch( ) .into_response()); } + if request.folders.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.folders.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Transform the format for the service let folders = request @@ -386,6 +435,15 @@ pub async fn get_files_batch( ) .into_response()); } + if request.file_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.file_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Execute batch operation let result = state @@ -430,6 +488,15 @@ pub async fn get_folders_batch( ) .into_response()); } + if request.folder_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.folder_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } // Execute batch operation let result = state @@ -495,6 +562,16 @@ pub async fn trash_batch( ) .into_response()); } + let combined_size = request.file_ids.len() + request.folder_ids.len(); + if combined_size > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", combined_size, MAX_BATCH_SIZE) + })), + ) + .into_response()); + } let mut all_successful: Vec = Vec::new(); let mut all_failed: Vec = Vec::new(); @@ -597,6 +674,15 @@ pub async fn move_folders_batch( ) .into_response()); } + if request.folder_ids.len() > MAX_BATCH_SIZE { + return Ok(( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "error": format!("Batch size {} exceeds maximum of {}", request.folder_ids.len(), MAX_BATCH_SIZE) + })), + ) + .into_response()); + } let result = state .batch_service @@ -637,6 +723,13 @@ pub async fn download_batch( "No file or folder IDs provided".to_string(), )); } + let combined_size = request.file_ids.len() + request.folder_ids.len(); + if combined_size > MAX_BATCH_SIZE { + return Err(( + StatusCode::BAD_REQUEST, + format!("Batch size {} exceeds maximum of {}", combined_size, MAX_BATCH_SIZE), + )); + } let temp_file = state .batch_service diff --git a/src/interfaces/api/handlers/caldav_handler.rs b/src/interfaces/api/handlers/caldav_handler.rs index 172abbc1..20beff93 100644 --- a/src/interfaces/api/handlers/caldav_handler.rs +++ b/src/interfaces/api/handlers/caldav_handler.rs @@ -39,6 +39,10 @@ use crate::interfaces::middleware::auth::CurrentUser; const HEADER_DAV: HeaderName = HeaderName::from_static("dav"); +/// Maximum allowed request body size for CalDAV XML/iCal endpoints (1 MB). +/// Prevents OOM/DoS via unbounded body buffering. +const MAX_CALDAV_BODY: usize = 1_048_576; + /// Creates CalDAV routes with full path prefixes. /// /// Uses `merge()` instead of `nest()` to avoid Axum's trailing-slash routing gap. @@ -182,7 +186,7 @@ async fn handle_propfind( let user = extract_user(&req)?; let calendar_service = get_calendar_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CALDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -423,7 +427,7 @@ async fn handle_report( let user = extract_user(&req)?; let calendar_service = get_calendar_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CALDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -498,7 +502,7 @@ async fn handle_mkcalendar( let user = extract_user(&req)?; let calendar_service = get_calendar_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CALDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -551,7 +555,7 @@ async fn handle_put( let calendar_id = parts[0]; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CALDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -795,7 +799,7 @@ async fn handle_proppatch( let user = extract_user(&req)?; let calendar_service = get_calendar_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CALDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; diff --git a/src/interfaces/api/handlers/carddav_handler.rs b/src/interfaces/api/handlers/carddav_handler.rs index 8298a19e..09aa0b0d 100644 --- a/src/interfaces/api/handlers/carddav_handler.rs +++ b/src/interfaces/api/handlers/carddav_handler.rs @@ -39,6 +39,10 @@ use crate::interfaces::middleware::auth::CurrentUser; const HEADER_DAV: HeaderName = HeaderName::from_static("dav"); +/// Maximum allowed request body size for CardDAV XML/vCard endpoints (1 MB). +/// Prevents OOM/DoS via unbounded body buffering. +const MAX_CARDDAV_BODY: usize = 1_048_576; + /// Creates CardDAV routes with full path prefixes. /// /// Uses `merge()` instead of `nest()` to avoid Axum's trailing-slash routing gap. @@ -181,7 +185,7 @@ async fn handle_propfind( let addressbook_service = get_addressbook_service(&state)?; let contact_svc = get_contact_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CARDDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -312,7 +316,7 @@ async fn handle_report( let user = extract_user(&req)?; let contact_svc = get_contact_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CARDDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -381,7 +385,7 @@ async fn handle_mkcol( let user = extract_user(&req)?; let addressbook_service = get_addressbook_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CARDDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -435,7 +439,7 @@ async fn handle_put( let address_book_id = parts[0]; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CARDDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; @@ -628,7 +632,7 @@ async fn handle_proppatch( let user = extract_user(&req)?; let addressbook_service = get_addressbook_service(&state)?; - let body_bytes = body::to_bytes(req.into_body(), usize::MAX) + let body_bytes = body::to_bytes(req.into_body(), MAX_CARDDAV_BODY) .await .map_err(|e| AppError::bad_request(format!("Failed to read request body: {}", e)))?; diff --git a/src/interfaces/api/handlers/wopi_handler.rs b/src/interfaces/api/handlers/wopi_handler.rs index 9aa545a5..435c43f4 100644 --- a/src/interfaces/api/handlers/wopi_handler.rs +++ b/src/interfaces/api/handlers/wopi_handler.rs @@ -386,6 +386,28 @@ pub struct EditorUrlResponse { pub access_token_ttl: i64, } +/// Determines if `caller_id` can access `file_id` and with what permissions. +/// +/// Uses the SQL-level ownership check (`get_file_owned`) so that files +/// belonging to other users — or non-existent files — both return `NOT_FOUND`, +/// avoiding existence-leak oracles. +/// +/// Returns `(FileDto, can_write)` on success. +async fn authorize_wopi_access( + file_retrieval: &S, + file_id: &str, + caller_id: &str, + requested_action: &str, +) -> Result<(crate::application::dtos::file_dto::FileDto, bool), StatusCode> { + let file = file_retrieval + .get_file_owned(file_id, caller_id) + .await + .map_err(|_| StatusCode::NOT_FOUND)?; + // Owner verified — grant write unless explicitly requesting view-only. + let can_write = requested_action != "view"; + Ok((file, can_write)) +} + /// GET /api/wopi/editor-url — Returns the editor iframe URL + WOPI token. /// /// This endpoint is behind normal auth middleware. The authenticated user @@ -399,16 +421,17 @@ pub async fn get_editor_url( Query(params): Query, State(state): State, ) -> Response { - // Get file info to determine extension - let file = match state - .app_state - .applications - .file_retrieval_service - .get_file(¶ms.file_id) - .await + // Verify the caller owns the file (SQL-level check, no existence leak). + let (file, can_write) = match authorize_wopi_access( + state.app_state.applications.file_retrieval_service.as_ref(), + ¶ms.file_id, + &user_id, + ¶ms.action, + ) + .await { - Ok(f) => f, - Err(_) => return StatusCode::NOT_FOUND.into_response(), + Ok(result) => result, + Err(status) => return status.into_response(), }; // Extract extension from filename @@ -437,13 +460,6 @@ pub async fn get_editor_url( } }; - // Determine write permission: owner can write, others read-only. - // If no owner_id on the file, default to allowing write. - let can_write = match &file.owner_id { - Some(owner) => owner == &user_id, - None => true, - }; - // Generate WOPI access token let (access_token, access_token_ttl) = match state @@ -485,20 +501,22 @@ async fn host_page( return StatusCode::UNAUTHORIZED.into_response(); } - // Get file info for extension - let file = match state - .app_state - .applications - .file_retrieval_service - .get_file(&file_id) - .await + // Re-verify ownership even though the token was valid — defence in depth. + let requested_action = if claims.can_write { "edit" } else { "view" }; + let file = match authorize_wopi_access( + state.app_state.applications.file_retrieval_service.as_ref(), + &file_id, + &claims.sub, + requested_action, + ) + .await { - Ok(f) => f, - Err(_) => return StatusCode::NOT_FOUND.into_response(), + Ok((f, _)) => f, + Err(status) => return status.into_response(), }; let extension = file.name.rsplit('.').next().unwrap_or("").to_lowercase(); - let action = if claims.can_write { "edit" } else { "view" }; + let action = requested_action; let wopi_src = format!("{}/wopi/files/{}", state.wopi_base_url, file_id); let editor_url = match state