fix: implement 22 security pentest remediation fixes
Server-side session management with SHA-256 hashed tokens and HttpOnly cookies. Auth middleware gating all RPC/WS/proxy routes with method allowlist. Login rate limiting (5/60s per IP). CORS restricted to config origin. Docker registry allowlist. App ID and path validation. P2P message sanitization (HTML + log injection). Onion address and known-peer validation. Nginx security headers (CSP, X-Frame-Options, etc.) and AIUI proxy auth. Systemd hardening (non-root, NoNewPrivileges, ProtectSystem). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
6623dbc4ab
commit
6656d2f1d9
@@ -17,6 +17,22 @@ impl RpcHandler {
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| anyhow::anyhow!("Missing manifest_path"))?;
|
||||
|
||||
// Validate manifest path: reject path traversal and paths outside apps/
|
||||
if manifest_path.contains("..") {
|
||||
return Err(anyhow::anyhow!(
|
||||
"Invalid manifest_path: path traversal not allowed"
|
||||
));
|
||||
}
|
||||
let path = std::path::Path::new(manifest_path);
|
||||
if path.is_absolute() {
|
||||
let apps_dir = self.config.data_dir.join("apps");
|
||||
if !path.starts_with(&apps_dir) {
|
||||
return Err(anyhow::anyhow!(
|
||||
"Invalid manifest_path: must be under the apps directory"
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// Load manifest
|
||||
let manifest_content = tokio::fs::read_to_string(manifest_path)
|
||||
.await
|
||||
|
||||
@@ -10,10 +10,12 @@ use crate::auth::AuthManager;
|
||||
use crate::config::Config;
|
||||
use crate::container::DevContainerOrchestrator;
|
||||
use crate::port_allocator::PortAllocator;
|
||||
use crate::session::{self, LoginRateLimiter, SessionStore};
|
||||
use crate::state::StateManager;
|
||||
use anyhow::{Context, Result};
|
||||
use hyper::{Request, Response, StatusCode};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::net::IpAddr;
|
||||
use std::sync::{Arc, Mutex};
|
||||
use tracing::{debug, error};
|
||||
|
||||
@@ -39,16 +41,29 @@ struct RpcError {
|
||||
/// Default dev password when no user is set up (matches mock-backend).
|
||||
pub(crate) const DEV_DEFAULT_PASSWORD: &str = "password123";
|
||||
|
||||
/// Methods that do not require a valid session cookie.
|
||||
const UNAUTHENTICATED_METHODS: &[&str] = &[
|
||||
"auth.login",
|
||||
"auth.isOnboardingComplete",
|
||||
"health",
|
||||
];
|
||||
|
||||
pub struct RpcHandler {
|
||||
config: Config,
|
||||
auth_manager: AuthManager,
|
||||
orchestrator: Option<Arc<DevContainerOrchestrator>>,
|
||||
state_manager: Arc<StateManager>,
|
||||
port_allocator: Arc<Mutex<PortAllocator>>,
|
||||
pub session_store: SessionStore,
|
||||
login_rate_limiter: LoginRateLimiter,
|
||||
}
|
||||
|
||||
impl RpcHandler {
|
||||
pub async fn new(config: Config, state_manager: Arc<StateManager>) -> Result<Self> {
|
||||
pub async fn new(
|
||||
config: Config,
|
||||
state_manager: Arc<StateManager>,
|
||||
session_store: SessionStore,
|
||||
) -> Result<Self> {
|
||||
let auth_manager = AuthManager::new(config.data_dir.clone());
|
||||
let orchestrator = if config.dev_mode {
|
||||
Some(Arc::new(
|
||||
@@ -65,6 +80,8 @@ impl RpcHandler {
|
||||
orchestrator,
|
||||
state_manager,
|
||||
port_allocator,
|
||||
session_store,
|
||||
login_rate_limiter: LoginRateLimiter::new(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -72,8 +89,10 @@ impl RpcHandler {
|
||||
&self,
|
||||
req: Request<hyper::Body>,
|
||||
) -> Result<Response<hyper::Body>> {
|
||||
// Read request body
|
||||
let (_, body) = req.into_parts();
|
||||
// Extract session cookie before consuming the request
|
||||
let (parts, body) = req.into_parts();
|
||||
let session_token = session::extract_session_cookie(&parts.headers);
|
||||
|
||||
let body_bytes = hyper::body::to_bytes(body).await
|
||||
.context("Failed to read body")?;
|
||||
|
||||
@@ -82,6 +101,55 @@ impl RpcHandler {
|
||||
|
||||
debug!("RPC method: {}", rpc_req.method);
|
||||
|
||||
// Enforce authentication for non-allowlisted methods
|
||||
let is_unauthenticated = UNAUTHENTICATED_METHODS.contains(&rpc_req.method.as_str());
|
||||
if !is_unauthenticated {
|
||||
let authenticated = match &session_token {
|
||||
Some(token) => self.session_store.validate(token).await,
|
||||
None => false,
|
||||
};
|
||||
if !authenticated {
|
||||
let rpc_resp = RpcResponse {
|
||||
result: None,
|
||||
error: Some(RpcError {
|
||||
code: 401,
|
||||
message: "Unauthorized".to_string(),
|
||||
data: None,
|
||||
}),
|
||||
};
|
||||
let resp_body = serde_json::to_vec(&rpc_resp)
|
||||
.context("Failed to serialize response")?;
|
||||
return Ok(Response::builder()
|
||||
.status(StatusCode::UNAUTHORIZED)
|
||||
.header("Content-Type", "application/json")
|
||||
.body(hyper::Body::from(resp_body))
|
||||
.unwrap());
|
||||
}
|
||||
}
|
||||
|
||||
// Rate limit login attempts
|
||||
if rpc_req.method == "auth.login" {
|
||||
let client_ip = extract_client_ip(&parts.headers);
|
||||
if !self.login_rate_limiter.check(client_ip).await {
|
||||
let rpc_resp = RpcResponse {
|
||||
result: None,
|
||||
error: Some(RpcError {
|
||||
code: 429,
|
||||
message: "Too many login attempts. Try again later.".to_string(),
|
||||
data: None,
|
||||
}),
|
||||
};
|
||||
let resp_body = serde_json::to_vec(&rpc_resp)
|
||||
.context("Failed to serialize response")?;
|
||||
return Ok(Response::builder()
|
||||
.status(StatusCode::TOO_MANY_REQUESTS)
|
||||
.header("Content-Type", "application/json")
|
||||
.header("Retry-After", "60")
|
||||
.body(hyper::Body::from(resp_body))
|
||||
.unwrap());
|
||||
}
|
||||
}
|
||||
|
||||
// Route to handler
|
||||
let result = match rpc_req.method.as_str() {
|
||||
"echo" => self.handle_echo(rpc_req.params).await,
|
||||
@@ -158,14 +226,46 @@ impl RpcHandler {
|
||||
}
|
||||
};
|
||||
|
||||
let body = serde_json::to_vec(&rpc_resp)
|
||||
let resp_body = serde_json::to_vec(&rpc_resp)
|
||||
.context("Failed to serialize response")?;
|
||||
|
||||
Ok(Response::builder()
|
||||
let mut response = Response::builder()
|
||||
.status(StatusCode::OK)
|
||||
.header("Content-Type", "application/json")
|
||||
.body(hyper::Body::from(body))
|
||||
.unwrap())
|
||||
.body(hyper::Body::from(resp_body))
|
||||
.unwrap();
|
||||
|
||||
// Track failed login attempts for rate limiting
|
||||
if rpc_req.method == "auth.login" && rpc_resp.error.is_some() {
|
||||
let client_ip = extract_client_ip(&parts.headers);
|
||||
self.login_rate_limiter.record_failure(client_ip).await;
|
||||
}
|
||||
|
||||
// On successful login, create a session and set the cookie
|
||||
if rpc_req.method == "auth.login" && rpc_resp.error.is_none() {
|
||||
let token = self.session_store.create().await;
|
||||
response.headers_mut().insert(
|
||||
"Set-Cookie",
|
||||
format!("session={}; HttpOnly; SameSite=Strict; Path=/", token)
|
||||
.parse()
|
||||
.unwrap(),
|
||||
);
|
||||
}
|
||||
|
||||
// On logout, invalidate session and expire the cookie
|
||||
if rpc_req.method == "auth.logout" {
|
||||
if let Some(token) = &session_token {
|
||||
self.session_store.remove(token).await;
|
||||
}
|
||||
response.headers_mut().insert(
|
||||
"Set-Cookie",
|
||||
"session=; HttpOnly; SameSite=Strict; Path=/; Max-Age=0"
|
||||
.parse()
|
||||
.unwrap(),
|
||||
);
|
||||
}
|
||||
|
||||
Ok(response)
|
||||
}
|
||||
|
||||
async fn handle_echo(&self, params: Option<serde_json::Value>) -> Result<serde_json::Value> {
|
||||
@@ -177,3 +277,14 @@ impl RpcHandler {
|
||||
Ok(serde_json::json!({ "message": "Hello from Archipelago!" }))
|
||||
}
|
||||
}
|
||||
|
||||
/// Extract the client IP from request headers (X-Real-IP or X-Forwarded-For).
|
||||
fn extract_client_ip(headers: &hyper::HeaderMap) -> IpAddr {
|
||||
headers
|
||||
.get("x-real-ip")
|
||||
.or_else(|| headers.get("x-forwarded-for"))
|
||||
.and_then(|v| v.to_str().ok())
|
||||
.and_then(|s| s.split(',').next())
|
||||
.and_then(|s| s.trim().parse::<IpAddr>().ok())
|
||||
.unwrap_or(IpAddr::V4(std::net::Ipv4Addr::LOCALHOST))
|
||||
}
|
||||
|
||||
@@ -16,6 +16,7 @@ impl RpcHandler {
|
||||
.get("id")
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| anyhow::anyhow!("Missing package id"))?;
|
||||
validate_app_id(package_id)?;
|
||||
|
||||
let docker_image = params
|
||||
.get("dockerImage")
|
||||
@@ -565,6 +566,7 @@ impl RpcHandler {
|
||||
.get("id")
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| anyhow::anyhow!("Missing package id"))?;
|
||||
validate_app_id(package_id)?;
|
||||
let preserve_data = params
|
||||
.get("preserve_data")
|
||||
.and_then(|v| v.as_bool())
|
||||
@@ -711,6 +713,7 @@ impl RpcHandler {
|
||||
|
||||
/// Get all container names for an app (handles multi-container apps like mempool)
|
||||
async fn get_containers_for_app(package_id: &str) -> Result<Vec<String>> {
|
||||
validate_app_id(package_id)?;
|
||||
let output = tokio::process::Command::new("sudo")
|
||||
.args(["podman", "ps", "-a", "--format", "{{.Names}}"])
|
||||
.output()
|
||||
@@ -759,7 +762,8 @@ async fn get_containers_for_app(package_id: &str) -> Result<Vec<String>> {
|
||||
Ok(result)
|
||||
}
|
||||
|
||||
/// Get data directories to clean for an app
|
||||
/// Get data directories to clean for an app.
|
||||
/// Caller must validate package_id before calling.
|
||||
fn get_data_dirs_for_app(package_id: &str) -> Vec<String> {
|
||||
let base = "/var/lib/archipelago";
|
||||
match package_id {
|
||||
@@ -781,20 +785,39 @@ fn get_data_dirs_for_app(package_id: &str) -> Vec<String> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Validate Docker image name format
|
||||
/// Prevents command injection via malicious image names
|
||||
/// Trusted Docker registries. Only images from these sources are allowed.
|
||||
const TRUSTED_REGISTRIES: &[&str] = &[
|
||||
"docker.io/",
|
||||
"ghcr.io/",
|
||||
"localhost/",
|
||||
];
|
||||
|
||||
/// Validate Docker image against trusted registry allowlist.
|
||||
fn is_valid_docker_image(image: &str) -> bool {
|
||||
if image.is_empty() || image.len() > 256 {
|
||||
return false;
|
||||
}
|
||||
// Reject shell metacharacters
|
||||
let dangerous_chars = ['&', '|', ';', '`', '$', '(', ')', '<', '>', '\n', '\r'];
|
||||
if image.chars().any(|c| dangerous_chars.contains(&c)) {
|
||||
return false;
|
||||
}
|
||||
if !image.chars().any(|c| c.is_alphanumeric()) {
|
||||
return false;
|
||||
// Must come from a trusted registry
|
||||
TRUSTED_REGISTRIES.iter().any(|r| image.starts_with(r))
|
||||
}
|
||||
|
||||
/// Validate that a package/app ID is safe (lowercase alphanumeric + hyphens, 1-64 chars).
|
||||
fn validate_app_id(id: &str) -> Result<()> {
|
||||
if id.is_empty() || id.len() > 64 {
|
||||
anyhow::bail!("Invalid app id: must be 1-64 characters");
|
||||
}
|
||||
if image.len() > 256 {
|
||||
return false;
|
||||
if !id.bytes().all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-') {
|
||||
anyhow::bail!("Invalid app id: only lowercase letters, digits, and hyphens allowed");
|
||||
}
|
||||
true
|
||||
if id.starts_with('-') {
|
||||
anyhow::bail!("Invalid app id: must not start with a hyphen");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Per-app Linux capabilities needed beyond the default cap-drop=ALL.
|
||||
|
||||
@@ -60,6 +60,19 @@ impl RpcHandler {
|
||||
.get("message")
|
||||
.and_then(|v| v.as_str())
|
||||
.ok_or_else(|| anyhow::anyhow!("Missing message"))?;
|
||||
|
||||
// Validate onion is a known peer to prevent SSRF to arbitrary Tor destinations
|
||||
let known_peers = peers::load_peers(&self.config.data_dir).await?;
|
||||
let is_known = known_peers.iter().any(|p| {
|
||||
p.onion == onion || p.onion == format!("{}.onion", onion)
|
||||
|| format!("{}.onion", p.onion) == onion
|
||||
});
|
||||
if !is_known {
|
||||
return Err(anyhow::anyhow!(
|
||||
"Onion address not in known peers list. Add the peer first."
|
||||
));
|
||||
}
|
||||
|
||||
let (data, _) = self.state_manager.get_snapshot().await;
|
||||
let pubkey = data.server_info.pubkey.clone();
|
||||
node_message::send_to_peer(onion, &pubkey, message).await?;
|
||||
|
||||
Reference in New Issue
Block a user