From 47837926cb00518ae94897ec5fd72bce8a6a98fc Mon Sep 17 00:00:00 2001 From: TacShade <198919272+tacshade@users.noreply.github.com> Date: Wed, 25 Feb 2026 22:27:14 +0100 Subject: [PATCH] security: enforce safe network/file defaults in core pipelines --- Cargo.lock | 4 +- crates/trace-share-core/src/config.rs | 46 +++++++- crates/trace-share-core/src/publish.rs | 10 +- crates/trace-share-core/src/sanitize.rs | 45 ++++++++ crates/trace-share-core/src/snapshot.rs | 7 +- crates/trace-share-core/src/sources.rs | 10 +- crates/trace-share-core/src/worker.rs | 11 +- docs/SECURITY_AUDIT.md | 138 ++++++++++++++++++++++++ 8 files changed, 258 insertions(+), 13 deletions(-) create mode 100644 docs/SECURITY_AUDIT.md diff --git a/Cargo.lock b/Cargo.lock index a9b4678..28c7102 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1780,7 +1780,7 @@ checksum = "8df9b6e13f2d32c91b9bd719c00d1958837bc7dec474d94952798cc8e69eeec3" [[package]] name = "trace-share-cli" -version = "0.0.2" +version = "0.0.3" dependencies = [ "anyhow", "clap", @@ -1795,7 +1795,7 @@ dependencies = [ [[package]] name = "trace-share-core" -version = "0.0.2" +version = "0.0.3" dependencies = [ "anyhow", "blake3", diff --git a/crates/trace-share-core/src/config.rs b/crates/trace-share-core/src/config.rs index 45f9125..c1122a3 100644 --- a/crates/trace-share-core/src/config.rs +++ b/crates/trace-share-core/src/config.rs @@ -1,7 +1,10 @@ use anyhow::{Context, Result}; use directories::ProjectDirs; use serde::{Deserialize, Serialize}; -use std::{env, fs, path::PathBuf}; +use std::{ + env, fs, + path::{Path, PathBuf}, +}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct UpstashConfig { @@ -174,3 +177,44 @@ pub fn ensure_dirs() -> Result<()> { fs::create_dir_all(&data_dir)?; Ok(()) } + +pub fn validate_network_url(url: &str, label: &str) -> Result<()> { + let parsed = reqwest::Url::parse(url).with_context(|| format!("invalid {label} URL: {url}"))?; + + if parsed.scheme() == "https" { + return Ok(()); + } + + if parsed.scheme() == "http" && (allow_insecure_http() || is_loopback_http(&parsed)) { + return Ok(()); + } + + anyhow::bail!( + "insecure {label} URL scheme '{}' is not allowed (use https; http is only allowed for loopback or when TRACE_SHARE_ALLOW_INSECURE_HTTP=1)", + parsed.scheme() + ); +} + +fn allow_insecure_http() -> bool { + env::var("TRACE_SHARE_ALLOW_INSECURE_HTTP") + .ok() + .map(|v| matches!(v.as_str(), "1" | "true" | "TRUE" | "yes")) + .unwrap_or(false) +} + +fn is_loopback_http(url: &reqwest::Url) -> bool { + matches!( + url.host_str(), + Some("localhost") | Some("127.0.0.1") | Some("::1") + ) +} + +pub fn write_private_file(path: &Path, contents: &[u8]) -> Result<()> { + fs::write(path, contents)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(path, fs::Permissions::from_mode(0o600))?; + } + Ok(()) +} diff --git a/crates/trace-share-core/src/publish.rs b/crates/trace-share-core/src/publish.rs index 1024177..d3ac482 100644 --- a/crates/trace-share-core/src/publish.rs +++ b/crates/trace-share-core/src/publish.rs @@ -5,7 +5,7 @@ use std::fs; use tokio::time::{Duration, sleep}; use crate::{ - config::{AppConfig, data_dir}, + config::{AppConfig, data_dir, validate_network_url, write_private_file}, episode::EpisodeRecord, models::ChunkDocument, sanitize::contains_sensitive_patterns, @@ -42,6 +42,7 @@ pub async fn publish_upsert_data(config: &AppConfig, docs: &[ChunkDocument]) -> .rest_url .as_ref() .context("missing UPSTASH_VECTOR_REST_URL")?; + validate_network_url(rest_url, "Upstash REST")?; let token = config .upstash .rest_token @@ -49,7 +50,7 @@ pub async fn publish_upsert_data(config: &AppConfig, docs: &[ChunkDocument]) -> .context("missing UPSTASH_VECTOR_REST_TOKEN")?; let endpoint = format!("{}/upsert-data", rest_url.trim_end_matches('/')); - let client = reqwest::Client::new(); + let client = reqwest::Client::builder().no_proxy().build()?; let payload = json!({ "vectors": anonymized_docs.iter().map(|doc| { @@ -133,7 +134,7 @@ pub fn load_or_create_anonymization_salt() -> Result { } } let salt = uuid::Uuid::new_v4().to_string(); - fs::write(path, &salt)?; + write_private_file(&path, salt.as_bytes())?; Ok(salt) } @@ -148,6 +149,7 @@ pub async fn index_episode_pointer( .rest_url .as_ref() .context("missing UPSTASH_VECTOR_REST_URL")?; + validate_network_url(rest_url, "Upstash REST")?; let token = config .upstash .rest_token @@ -156,7 +158,7 @@ pub async fn index_episode_pointer( let salt = load_or_create_anonymization_salt()?; let endpoint = format!("{}/upsert-data", rest_url.trim_end_matches('/')); - let client = reqwest::Client::new(); + let client = reqwest::Client::builder().no_proxy().build()?; let metadata = serde_json::json!({ "kind": "episode_pointer", diff --git a/crates/trace-share-core/src/sanitize.rs b/crates/trace-share-core/src/sanitize.rs index 24fdbe8..b77a6fd 100644 --- a/crates/trace-share-core/src/sanitize.rs +++ b/crates/trace-share-core/src/sanitize.rs @@ -348,8 +348,19 @@ fn extract_event_index(path_text: &str) -> Option { } fn find_gitleaks_binary() -> Option { + if let Some(configured) = std::env::var_os("TRACE_SHARE_GITLEAKS_PATH") { + let configured_path = PathBuf::from(configured); + if configured_path.is_absolute() && configured_path.exists() { + return Some(configured_path); + } + return None; + } + let path = std::env::var_os("PATH")?; std::env::split_paths(&path).find_map(|dir| { + if !is_trusted_gitleaks_dir(&dir) { + return None; + } let candidate = dir.join("gitleaks"); if candidate.exists() { return Some(candidate); @@ -365,6 +376,40 @@ fn find_gitleaks_binary() -> Option { }) } +fn is_trusted_gitleaks_dir(dir: &std::path::Path) -> bool { + if let Ok(home) = std::env::var("HOME") { + let home_path = PathBuf::from(home); + if dir.starts_with(home_path) { + return false; + } + } + + #[cfg(unix)] + { + return dir.starts_with("/usr/bin") + || dir.starts_with("/usr/local/bin") + || dir.starts_with("/opt/homebrew/bin") + || dir.starts_with("/bin") + || dir.starts_with("/sbin") + || dir.starts_with("/usr/sbin"); + } + + #[cfg(windows)] + { + if let Ok(program_files) = std::env::var("ProgramFiles") { + if dir.starts_with(PathBuf::from(program_files)) { + return true; + } + } + if let Ok(program_files_x86) = std::env::var("ProgramFiles(x86)") { + if dir.starts_with(PathBuf::from(program_files_x86)) { + return true; + } + } + return false; + } +} + #[derive(Debug, Clone, Default, Deserialize)] struct GitleaksFinding { #[serde(rename = "File")] diff --git a/crates/trace-share-core/src/snapshot.rs b/crates/trace-share-core/src/snapshot.rs index 1f5a0ff..45f615f 100644 --- a/crates/trace-share-core/src/snapshot.rs +++ b/crates/trace-share-core/src/snapshot.rs @@ -12,7 +12,7 @@ use std::{ use tokio::time::{Duration, sleep}; use crate::{ - config::AppConfig, + config::{AppConfig, validate_network_url}, episode::{EpisodeRecord, derive_sft, derive_tooltrace}, }; @@ -349,11 +349,13 @@ async fn publish_snapshot_to_worker( .base_url .as_ref() .context("missing TRACE_SHARE_WORKER_BASE_URL")?; + validate_network_url(base_url, "worker base")?; let endpoint = format!("{}/v1/snapshots", base_url.trim_end_matches('/')); let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs( config.worker.timeout_seconds.max(5), )) + .no_proxy() .build()?; let payload = serde_json::json!({ @@ -414,13 +416,14 @@ async fn index_snapshot_pointer( .rest_url .as_ref() .context("missing UPSTASH_VECTOR_REST_URL")?; + validate_network_url(rest_url, "Upstash REST")?; let token = config .upstash .rest_token .as_ref() .context("missing UPSTASH_VECTOR_REST_TOKEN")?; let endpoint = format!("{}/upsert-data", rest_url.trim_end_matches('/')); - let client = reqwest::Client::new(); + let client = reqwest::Client::builder().no_proxy().build()?; let payload = serde_json::json!({ "vectors": [ diff --git a/crates/trace-share-core/src/sources.rs b/crates/trace-share-core/src/sources.rs index cfec9d2..01567a5 100644 --- a/crates/trace-share-core/src/sources.rs +++ b/crates/trace-share-core/src/sources.rs @@ -10,7 +10,9 @@ use std::{ }; use tracing::warn; -use crate::config::{AppConfig, data_dir, default_sources_path}; +use crate::config::{ + AppConfig, data_dir, default_sources_path, validate_network_url, write_private_file, +}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SourceDef { @@ -174,7 +176,7 @@ pub fn add_local_source(config: &AppConfig, source: SourceDef) -> Result Result .url .clone() .context("remote registry url missing")?; + validate_network_url(&url, "remote registry")?; let cache_path = data_dir()?.join("registry-cache.json"); let cached = read_cache(&cache_path).ok(); @@ -233,7 +236,8 @@ pub async fn load_remote_registry(config: &AppConfig) -> Result etag, manifest: manifest.clone(), }; - fs::write(cache_path, serde_json::to_vec_pretty(&snapshot)?)?; + let snapshot_bytes = serde_json::to_vec_pretty(&snapshot)?; + write_private_file(&cache_path, &snapshot_bytes)?; Ok(manifest) } diff --git a/crates/trace-share-core/src/worker.rs b/crates/trace-share-core/src/worker.rs index bdcced2..5a4a8c1 100644 --- a/crates/trace-share-core/src/worker.rs +++ b/crates/trace-share-core/src/worker.rs @@ -4,7 +4,10 @@ use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use tokio::time::{Duration, sleep}; -use crate::{config::AppConfig, episode::EpisodeRecord}; +use crate::{ + config::{AppConfig, validate_network_url}, + episode::EpisodeRecord, +}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct WorkerUploadResponse { @@ -51,12 +54,14 @@ async fn upload_episode_legacy( .base_url .as_ref() .context("missing TRACE_SHARE_WORKER_BASE_URL")?; + validate_network_url(base_url, "worker base")?; let endpoint = format!("{}/v1/episodes", base_url.trim_end_matches('/')); let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs( config.worker.timeout_seconds.max(5), )) + .no_proxy() .build()?; let mut attempt: u32 = 0; @@ -103,12 +108,14 @@ async fn upload_episode_presigned( .base_url .as_ref() .context("missing TRACE_SHARE_WORKER_BASE_URL")?; + validate_network_url(base_url, "worker base")?; let presign_endpoint = format!("{}/v1/episodes/presign", base_url.trim_end_matches('/')); let complete_endpoint = format!("{}/v1/episodes/complete", base_url.trim_end_matches('/')); let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs( config.worker.timeout_seconds.max(5), )) + .no_proxy() .build()?; let presign_payload = serde_json::json!({ @@ -163,12 +170,14 @@ pub async fn push_revocation( .base_url .as_ref() .context("missing TRACE_SHARE_WORKER_BASE_URL")?; + validate_network_url(base_url, "worker base")?; let endpoint = format!("{}/v1/revocations", base_url.trim_end_matches('/')); let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs( config.worker.timeout_seconds.max(5), )) + .no_proxy() .build()?; let payload = serde_json::json!({ diff --git a/docs/SECURITY_AUDIT.md b/docs/SECURITY_AUDIT.md new file mode 100644 index 0000000..9b238f7 --- /dev/null +++ b/docs/SECURITY_AUDIT.md @@ -0,0 +1,138 @@ +# Security Audit Report + +Date: 2026-02-25 +Scope: `trace-share` Rust CLI/core crates and security-relevant data flows. + +## Method + +- Manual code review of authentication, transport, local storage, external-process execution, and privacy-sensitive paths. +- Static spot checks via `rg` for network/file/process primitives and sensitive pattern handling. +- Test execution to detect runtime regressions in security-adjacent behavior. + +## Findings + +### 1) Missing transport security enforcement for authenticated outbound requests (High) + +**What I found** + +Multiple upload/index/revocation code paths accept user-configured URLs and send bearer tokens without enforcing `https://` schemes. If a user (or compromised config) sets an `http://` endpoint, API tokens and payloads can be exposed to interception or tampering. + +- Worker uploads build endpoint from `worker.base_url` and attach bearer auth. +- Revocation pushes do the same. +- Upstash publish/index calls build endpoint from `upstash.rest_url` and attach bearer auth. +- Remote registry fetch also accepts arbitrary URL without scheme restriction. + +**Evidence** + +- `worker.base_url` is used directly for authenticated POSTs. + `crates/trace-share-core/src/worker.rs` lines 49-67, 167-185. +- `upstash.rest_url` is used directly for authenticated POSTs. + `crates/trace-share-core/src/publish.rs` lines 40-52, 66-70, 146-159, 194-198. +- Remote registry URL is fetched directly via HTTP client. + `crates/trace-share-core/src/sources.rs` lines 181-200, 207. + +**Risk** + +- Credential disclosure (bearer tokens). +- In-transit tampering of uploaded episodes/metadata. +- Registry poisoning if insecure transport is used. + +**Recommendation** + +- Validate URL schemes at config load and before request execution; reject non-HTTPS URLs by default. +- Allow explicit insecure override only via dedicated opt-in env flag intended for local testing. +- Consider host allowlist/certificate pinning for high-assurance deployments. + +--- + +### 2) Sensitive local artifacts written without explicit restrictive permissions (Medium) + +**What I found** + +Security-sensitive local files are created with default process umask rather than explicitly setting owner-only permissions. + +- Anonymization salt (`anonymization_salt`) is written without `0600`/equivalent hardening. +- Registry cache is written similarly. +- Local sources manifest write path likewise uses default perms. + +**Evidence** + +- Salt creation write call: `fs::write(path, &salt)`. + `crates/trace-share-core/src/publish.rs` line 136. +- Registry cache write call: `fs::write(cache_path, ...)`. + `crates/trace-share-core/src/sources.rs` line 236. +- Sources manifest write call: `fs::write(&path, text)`. + `crates/trace-share-core/src/sources.rs` line 177. + +**Risk** + +- On permissive umask/shared systems, other local users may read sensitive operational metadata (or anonymization salt, which weakens privacy unlinkability assumptions). + +**Recommendation** + +- Use atomic file creation with explicit permissions (`OpenOptions` + `set_permissions`, or platform-specific secure defaults). +- Restrict security-sensitive state files to owner-read/write. + +--- + +### 3) External tool discovery trusts `PATH` for `gitleaks` binary (Low/Medium) + +**What I found** + +The sanitization flow auto-executes a `gitleaks` binary discovered by scanning `PATH` and only checks file existence. This can execute a malicious binary if `PATH` is poisoned in the runtime environment. + +**Evidence** + +- Binary search and selection from `PATH` via `candidate.exists()`. + `crates/trace-share-core/src/sanitize.rs` lines 350-366. +- Selected binary is executed with `Command::new(gitleaks_bin)`. + `crates/trace-share-core/src/sanitize.rs` lines 288-299. + +**Risk** + +- Arbitrary code execution in user context when sanitization runs. + +**Recommendation** + +- Require explicit configured absolute path for `gitleaks` in hardened mode. +- Optionally verify binary integrity/signature or constrain search to trusted directories. + +--- + +### 4) Automatic update check performs network call on every startup (Informational) + +**What I found** + +CLI startup performs a GitHub request prior to command dispatch unless disabled via env var. + +**Evidence** + +- Update check called in `main` before command handling. + `crates/trace-share-cli/src/main.rs` lines 235-239. +- Remote call to GitHub release API. + `crates/trace-share-cli/src/main.rs` lines 260-277. + +**Risk** + +- Metadata/privacy concern in restricted environments (command invocation leaks to external service by default). + +**Recommendation** + +- Consider opt-in update checks (or first-run prompt). +- Document behavior clearly in privacy/security docs. + +## Positive controls observed + +- Sanitization includes multiple regex-based secret/PII detectors and entropy/JWT/PEM handling. + `crates/trace-share-core/src/sanitize.rs` lines 34-74, 76-168, 184-261. +- Upload path blocks if sensitive patterns still detected post-sanitize in text/metadata. + `crates/trace-share-core/src/publish.rs` lines 25-37, 178-180. +- Source definitions are validated for traversal segments and constrained to allowlisted roots. + `crates/trace-share-core/src/sources.rs` lines 336-377. + +## Suggested remediation order + +1. Enforce HTTPS + explicit insecure override gate. +2. Harden local file permissions for salts/state/cache/config artifacts. +3. Harden external binary trust model for `gitleaks` integration. +4. Make update checks opt-in or more explicitly controllable.