From 4aca118aaee846d4270fa9ae01c3a6c255f1c727 Mon Sep 17 00:00:00 2001 From: Nicolas Dreno Date: Thu, 2 Jul 2026 13:50:05 +0200 Subject: [PATCH] security(control-plane): error leakage, upload limits, WS cap, filename sanitization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the control-plane hardening items (area 3, issue #8). - CP-3: DB errors no longer interpolate raw sqlx::Error into responses (schema disclosure). All DB call sites route through the existing generic `From` mapper, which logs the detail server-side and returns a generic 500. Input-parse errors (YAML/spec) keep their user-facing detail. - CP-4: explicit request-body limits via DefaultBodyLimit — 1 MiB default for JSON endpoints, 32 MiB for spec/plugin upload routes — bounding in-memory buffering. - CP-6: cap concurrent WebSocket sessions (semaphore in ConnectionManager) so unauthenticated sockets can't pile up during the 30s registration window; sanitize upload filenames to a safe basename, defeating path traversal into the compile temp dir (worker.rs join) and CRLF/quote injection in the spec download Content-Disposition header. Sanitized at ingestion and at each use. CP-2 residual (per-project ownership / IDOR) is deferred: it needs the multi-tenant authz model and is not exploitable in the current single-admin deployment. Left tracked in #8. --- CHANGELOG.md | 1 + crates/barbacane-control/src/api/api_keys.rs | 19 ++---- .../barbacane-control/src/api/data_planes.rs | 40 +++-------- crates/barbacane-control/src/api/mod.rs | 2 +- crates/barbacane-control/src/api/multipart.rs | 67 ++++++++++++++++++- crates/barbacane-control/src/api/plugins.rs | 11 +-- crates/barbacane-control/src/api/router.rs | 24 ++++++- crates/barbacane-control/src/api/specs.rs | 7 +- .../barbacane-control/src/api/ws/handler.rs | 11 +++ .../barbacane-control/src/api/ws/manager.rs | 28 +++++++- .../barbacane-control/src/compiler/worker.rs | 12 +++- 11 files changed, 156 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 873052c..b2d98c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **security**: chunked request bodies with no `Content-Length` are now size-capped while streaming (`http_body_util::Limited`) instead of being fully buffered before the limit check. - **plugin**: `jwt-auth` and `oidc-auth` now declare the `log` capability they use, so they load under WASM capability enforcement (they emit a one-time warning when no `audience` is configured). - **ci**: the adversarial security suite (`crates/barbacane-test/tests/security/`) now runs in CI. +- **security (control plane)**: request-body size limits — 1 MiB default for JSON endpoints, 32 MiB for spec/plugin uploads — bound in-memory buffering; database errors are routed through the generic error mapper (no schema disclosure); upload filenames are sanitized to a safe basename (defeating path traversal into the compile temp dir and CRLF/quote injection in `Content-Disposition`); and concurrent WebSocket sessions are capped so unauthenticated sockets can't pile up during the registration window. - **deps**: bump `anyhow` to 1.0.103 (RUSTSEC-2026-0190). ## [0.7.0] - 2026-05-05 diff --git a/crates/barbacane-control/src/api/api_keys.rs b/crates/barbacane-control/src/api/api_keys.rs index bdef2ce..85cd5c7 100644 --- a/crates/barbacane-control/src/api/api_keys.rs +++ b/crates/barbacane-control/src/api/api_keys.rs @@ -19,9 +19,9 @@ pub async fn create_api_key( ) -> Result<(StatusCode, Json), ProblemDetails> { let repo = ApiKeysRepository::new(state.pool.clone()); - let created = repo.create(project_id, input).await.map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to create API key: {}", e)) - })?; + // DB errors route through the generic `From` mapper, which logs + // the detail server-side and returns a generic 500 (no schema disclosure). + let created = repo.create(project_id, input).await?; Ok((StatusCode::CREATED, Json(created))) } @@ -33,9 +33,7 @@ pub async fn list_api_keys( ) -> Result>, ProblemDetails> { let repo = ApiKeysRepository::new(state.pool.clone()); - let keys = repo.list_for_project(project_id).await.map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to list API keys: {}", e)) - })?; + let keys = repo.list_for_project(project_id).await?; Ok(Json(keys)) } @@ -50,10 +48,7 @@ pub async fn revoke_api_key( // Verify the key belongs to the project let key = repo .get(key_id) - .await - .map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to get API key: {}", e)) - })? + .await? .ok_or_else(|| ProblemDetails::not_found(format!("API key {} not found", key_id)))?; if key.project_id != project_id { @@ -63,9 +58,7 @@ pub async fn revoke_api_key( ))); } - repo.revoke(key_id).await.map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to revoke API key: {}", e)) - })?; + repo.revoke(key_id).await?; Ok(StatusCode::NO_CONTENT) } diff --git a/crates/barbacane-control/src/api/data_planes.rs b/crates/barbacane-control/src/api/data_planes.rs index bce432d..5dc2eb6 100644 --- a/crates/barbacane-control/src/api/data_planes.rs +++ b/crates/barbacane-control/src/api/data_planes.rs @@ -26,9 +26,7 @@ pub async fn list_data_planes( let repo = DataPlanesRepository::new(state.pool.clone()); - let data_planes = repo.list_for_project(project_id).await.map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to list data planes: {}", e)) - })?; + let data_planes = repo.list_for_project(project_id).await?; Ok(Json(data_planes)) } @@ -42,10 +40,7 @@ pub async fn get_data_plane( let data_plane = repo .get(dp_id) - .await - .map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to get data plane: {}", e)) - })? + .await? .ok_or_else(|| ProblemDetails::not_found(format!("Data plane {} not found", dp_id)))?; if data_plane.project_id != project_id { @@ -68,10 +63,7 @@ pub async fn disconnect_data_plane( // Verify the data plane belongs to the project let data_plane = repo .get(dp_id) - .await - .map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to get data plane: {}", e)) - })? + .await? .ok_or_else(|| ProblemDetails::not_found(format!("Data plane {} not found", dp_id)))?; if data_plane.project_id != project_id { @@ -85,9 +77,7 @@ pub async fn disconnect_data_plane( state.connection_manager.remove(dp_id); // Delete the record - repo.delete(dp_id).await.map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to delete data plane: {}", e)) - })?; + repo.delete(dp_id).await?; Ok(StatusCode::NO_CONTENT) } @@ -118,26 +108,12 @@ pub async fn deploy_to_data_planes( // Get the artifact to deploy let artifact = if let Some(artifact_id) = request.artifact_id { - artifacts_repo - .get(artifact_id) - .await - .map_err(|e| { - ProblemDetails::internal_error_with_detail(format!("Failed to get artifact: {}", e)) - })? - .ok_or_else(|| { - ProblemDetails::not_found(format!("Artifact {} not found", artifact_id)) - })? + artifacts_repo.get(artifact_id).await?.ok_or_else(|| { + ProblemDetails::not_found(format!("Artifact {} not found", artifact_id)) + })? } else { // Get the latest artifact for this project - let artifacts = artifacts_repo - .list_for_project(project_id) - .await - .map_err(|e| { - ProblemDetails::internal_error_with_detail(format!( - "Failed to list artifacts: {}", - e - )) - })?; + let artifacts = artifacts_repo.list_for_project(project_id).await?; artifacts.into_iter().next().ok_or_else(|| { ProblemDetails::not_found(format!("No artifacts found for project {}", project_id)) diff --git a/crates/barbacane-control/src/api/mod.rs b/crates/barbacane-control/src/api/mod.rs index a98971f..bc479db 100644 --- a/crates/barbacane-control/src/api/mod.rs +++ b/crates/barbacane-control/src/api/mod.rs @@ -7,7 +7,7 @@ mod compilations; mod data_planes; mod health; mod init; -mod multipart; +pub(crate) mod multipart; mod operations; mod plugins; mod project_plugins; diff --git a/crates/barbacane-control/src/api/multipart.rs b/crates/barbacane-control/src/api/multipart.rs index 4e9ef68..d7083e2 100644 --- a/crates/barbacane-control/src/api/multipart.rs +++ b/crates/barbacane-control/src/api/multipart.rs @@ -4,6 +4,36 @@ use axum::extract::Multipart; use crate::error::ProblemDetails; +/// Reduce an untrusted upload filename to a safe basename. +/// +/// Strips any directory components (defeating `../` path traversal when the name +/// is later joined onto a filesystem path) and replaces any character outside a +/// conservative allowlist (defeating CRLF/quote injection when the name is +/// reflected into a `Content-Disposition` header). Falls back to `upload` when +/// nothing usable remains. +pub fn safe_filename(raw: &str) -> String { + let base = std::path::Path::new(raw) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(""); + let cleaned: String = base + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_') { + c + } else { + '_' + } + }) + .collect(); + let trimmed = cleaned.trim_matches('.'); + if trimmed.is_empty() { + "upload".to_string() + } else { + trimmed.to_string() + } +} + /// Extract a single `file` field from a multipart upload. /// /// Returns `(file_bytes, filename)`. Fails with 400 if the field is missing @@ -36,5 +66,40 @@ pub async fn extract_file_field( let content = file_data.ok_or_else(|| ProblemDetails::bad_request("Missing 'file' field"))?; let filename = filename.ok_or_else(|| ProblemDetails::bad_request("Missing filename"))?; - Ok((content, filename)) + // Sanitize before the name is stored and later used as a filesystem path + // and reflected into response headers. + Ok((content, safe_filename(&filename))) +} + +#[cfg(test)] +mod tests { + use super::safe_filename; + + #[test] + fn keeps_ordinary_names() { + assert_eq!(safe_filename("api.yaml"), "api.yaml"); + assert_eq!(safe_filename("my-api.v2.json"), "my-api.v2.json"); + } + + #[test] + fn strips_directory_traversal() { + assert_eq!(safe_filename("../../etc/passwd"), "passwd"); + assert_eq!(safe_filename("/abs/path/spec.yaml"), "spec.yaml"); + // A bare traversal segment leaves no basename. + assert_eq!(safe_filename(".."), "upload"); + assert_eq!(safe_filename(""), "upload"); + // Backslashes are not path separators on Unix, but are still neutralized + // to a safe character so the result can never form a traversal or inject. + let win = safe_filename("..\\..\\win.yaml"); + assert!(!win.contains('\\') && !win.contains('/')); + } + + #[test] + fn strips_header_injection_chars() { + // CRLF / quotes that would break a Content-Disposition header. + assert_eq!( + safe_filename("a\"; drop\r\nSet-Cookie: x.yaml"), + "a___drop__Set-Cookie__x.yaml" + ); + } } diff --git a/crates/barbacane-control/src/api/plugins.rs b/crates/barbacane-control/src/api/plugins.rs index 18f1e5b..87ba96f 100644 --- a/crates/barbacane-control/src/api/plugins.rs +++ b/crates/barbacane-control/src/api/plugins.rs @@ -222,16 +222,7 @@ pub async fn delete_plugin( ) -> Result { // Refuse to delete if any artifact still bundles this plugin version. let artifacts_repo = ArtifactsRepository::new(state.pool.clone()); - if artifacts_repo - .plugin_is_referenced(&name, &version) - .await - .map_err(|e| { - ProblemDetails::internal_error_with_detail(format!( - "Failed to check artifact references: {}", - e - )) - })? - { + if artifacts_repo.plugin_is_referenced(&name, &version).await? { return Err(ProblemDetails::conflict(format!( "Plugin {}:{} is referenced by existing artifacts and cannot be deleted", name, version diff --git a/crates/barbacane-control/src/api/router.rs b/crates/barbacane-control/src/api/router.rs index 296140b..d832368 100644 --- a/crates/barbacane-control/src/api/router.rs +++ b/crates/barbacane-control/src/api/router.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use axum::{ + extract::DefaultBodyLimit, http::{header, HeaderValue, Method, StatusCode}, response::{Html, IntoResponse}, routing::{delete, get, patch, post, put}, @@ -32,6 +33,14 @@ const OPENAPI_SPEC: &str = include_str!("../../openapi.yaml"); /// API version header value. const API_VERSION: &str = "application/vnd.barbacane.v1+json"; +/// Body-size ceiling for ordinary JSON API requests. Bounds how much a caller +/// can make the control plane buffer in memory per request. +const MAX_JSON_BODY: usize = 1024 * 1024; // 1 MiB + +/// Body-size ceiling for file uploads (spec documents, WASM plugins). Larger +/// than the JSON limit to accommodate real plugin binaries, but still bounded. +const MAX_UPLOAD_BODY: usize = 32 * 1024 * 1024; // 32 MiB + /// Shared application state. #[derive(Clone)] pub struct AppState { @@ -123,7 +132,10 @@ pub fn create_router( // Init .route("/init", post(init::init_project)) // Specs - .route("/specs", post(specs::upload_spec)) + .route( + "/specs", + post(specs::upload_spec).layer(DefaultBodyLimit::max(MAX_UPLOAD_BODY)), + ) .route("/specs", get(specs::list_specs)) .route("/specs/{id}", get(specs::get_spec)) .route("/specs/{id}", delete(specs::delete_spec)) @@ -136,7 +148,10 @@ pub fn create_router( get(compilations::list_spec_compilations), ) // Plugins - .route("/plugins", post(plugins::register_plugin)) + .route( + "/plugins", + post(plugins::register_plugin).layer(DefaultBodyLimit::max(MAX_UPLOAD_BODY)), + ) .route("/plugins", get(plugins::list_plugins)) .route("/plugins/{name}", get(plugins::list_plugin_versions)) .route("/plugins/{name}/{version}", get(plugins::get_plugin)) @@ -174,7 +189,7 @@ pub fn create_router( .route("/projects/{id}/specs", get(projects::list_project_specs)) .route( "/projects/{id}/specs", - post(projects::upload_spec_to_project), + post(projects::upload_spec_to_project).layer(DefaultBodyLimit::max(MAX_UPLOAD_BODY)), ) // Project plugins .route( @@ -240,6 +255,9 @@ pub fn create_router( Router::new() .merge(public) .merge(protected) + // Default request-body ceiling for all routes (upload routes opt into a + // larger limit via their own inner DefaultBodyLimit layer). + .layer(DefaultBodyLimit::max(MAX_JSON_BODY)) // Middleware applied to all routes .layer(TraceLayer::new_for_http()) .layer(cors_layer()) diff --git a/crates/barbacane-control/src/api/specs.rs b/crates/barbacane-control/src/api/specs.rs index 65620b5..0361257 100644 --- a/crates/barbacane-control/src/api/specs.rs +++ b/crates/barbacane-control/src/api/specs.rs @@ -226,7 +226,12 @@ pub async fn download_spec_content( ), ( axum::http::header::CONTENT_DISPOSITION, - format!("attachment; filename=\"{}\"", revision.filename), + // Sanitize before reflecting into the header (defeats CRLF/quote + // injection); older rows may predate ingestion-time sanitization. + format!( + "attachment; filename=\"{}\"", + super::multipart::safe_filename(&revision.filename) + ), ), ], revision.content, diff --git a/crates/barbacane-control/src/api/ws/handler.rs b/crates/barbacane-control/src/api/ws/handler.rs index e2e3573..1247823 100644 --- a/crates/barbacane-control/src/api/ws/handler.rs +++ b/crates/barbacane-control/src/api/ws/handler.rs @@ -31,6 +31,17 @@ pub async fn ws_handler(ws: WebSocketUpgrade, State(state): State) -> /// Handle an individual WebSocket connection. async fn handle_socket(socket: WebSocket, state: AppState) { + // Reserve a session slot up front so unauthenticated sockets can't pile up + // during the registration window. Held for the whole session (released on + // drop when this function returns). + let _session_permit = match state.connection_manager.try_acquire_session() { + Some(permit) => permit, + None => { + tracing::warn!("WebSocket session cap reached; rejecting connection"); + return; + } + }; + let (mut sender, mut receiver) = socket.split(); // Wait for registration message diff --git a/crates/barbacane-control/src/api/ws/manager.rs b/crates/barbacane-control/src/api/ws/manager.rs index 2e17014..e8562c5 100644 --- a/crates/barbacane-control/src/api/ws/manager.rs +++ b/crates/barbacane-control/src/api/ws/manager.rs @@ -1,11 +1,18 @@ //! Connection manager for tracking active WebSocket connections. +use std::sync::Arc; + use dashmap::DashMap; -use tokio::sync::mpsc; +use tokio::sync::{mpsc, OwnedSemaphorePermit, Semaphore}; use uuid::Uuid; use super::protocol::ControlPlaneMessage; +/// Ceiling on concurrently handled WebSocket sessions (registered *and* those +/// still pending registration). Bounds how many unauthenticated sockets an +/// attacker can pin during the pre-registration window. +const MAX_CONCURRENT_WS_SESSIONS: usize = 1024; + /// Information about a connected data plane. #[derive(Debug, Clone)] struct DataPlaneConnection { @@ -14,12 +21,20 @@ struct DataPlaneConnection { } /// Manages active WebSocket connections to data planes. -#[derive(Debug, Default)] +#[derive(Debug)] pub struct ConnectionManager { /// Active connections: data_plane_id -> connection info connections: DashMap, /// Index: project_id -> Vec project_connections: DashMap>, + /// Bounds concurrent WebSocket session handlers (pre-auth + registered). + session_slots: Arc, +} + +impl Default for ConnectionManager { + fn default() -> Self { + Self::new() + } } impl ConnectionManager { @@ -28,9 +43,18 @@ impl ConnectionManager { Self { connections: DashMap::new(), project_connections: DashMap::new(), + session_slots: Arc::new(Semaphore::new(MAX_CONCURRENT_WS_SESSIONS)), } } + /// Try to reserve a slot for a new (as-yet-unauthenticated) WebSocket + /// session. Returns `None` when the concurrent-session cap is reached, so the + /// handler can shed load instead of letting sockets pile up. The permit is + /// held for the session's lifetime and released on drop. + pub fn try_acquire_session(&self) -> Option { + Arc::clone(&self.session_slots).try_acquire_owned().ok() + } + /// Register a new connection. pub fn register( &self, diff --git a/crates/barbacane-control/src/compiler/worker.rs b/crates/barbacane-control/src/compiler/worker.rs index ad336e1..447611d 100644 --- a/crates/barbacane-control/src/compiler/worker.rs +++ b/crates/barbacane-control/src/compiler/worker.rs @@ -64,9 +64,13 @@ async fn process_compilation(pool: &PgPool, compilation_id: Uuid) -> anyhow::Res .await? .ok_or_else(|| anyhow::anyhow!("Spec revision not found"))?; - // Write spec to temp file + // Write spec to temp file. Sanitize the stored filename to a safe basename + // so it cannot escape the temp dir via `../` (defense-in-depth: uploads are + // sanitized on ingestion, but older rows may predate that). let temp_dir = tempfile::tempdir()?; - let spec_path = temp_dir.path().join(&spec_revision.filename); + let spec_path = temp_dir.path().join(crate::api::multipart::safe_filename( + &spec_revision.filename, + )); tokio::fs::write(&spec_path, &spec_revision.content).await?; // Collect all spec paths @@ -78,7 +82,9 @@ async fn process_compilation(pool: &PgPool, compilation_id: Uuid) -> anyhow::Res for additional_id in additional_spec_ids { if let Some(additional_revision) = specs_repo.get_latest_revision(additional_id).await? { - let additional_path = temp_dir.path().join(&additional_revision.filename); + let additional_path = temp_dir.path().join(crate::api::multipart::safe_filename( + &additional_revision.filename, + )); tokio::fs::write(&additional_path, &additional_revision.content).await?; spec_paths.push(additional_path); }