diff --git a/Cargo.lock b/Cargo.lock index 5df1288f..642b14c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -113,9 +113,9 @@ dependencies = [ [[package]] name = "base64" -version = "0.22.1" +version = "0.21.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" +checksum = "9d297deb1925b89f2ccc13d7635fa0714f12c87adce1c75356b39ca9b7178567" [[package]] name = "bitflags" @@ -958,36 +958,23 @@ dependencies = [ [[package]] name = "rustls" -version = "0.23.37" +version = "0.21.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "758025cb5fccfd3bc2fd74708fd4682be41d99e5dff73c377c0646c6012c73a4" +checksum = "3f56a14d1f48b391359b22f731fd4bd7e43c97f3c50eee276f3aa09c94784d3e" dependencies = [ "log", - "once_cell", "ring", - "rustls-pki-types", "rustls-webpki", - "subtle", - "zeroize", -] - -[[package]] -name = "rustls-pki-types" -version = "1.14.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "be040f8b0a225e40375822a563fa9524378b9d63112f53e19ffff34df5d33fdd" -dependencies = [ - "zeroize", + "sct", ] [[package]] name = "rustls-webpki" -version = "0.103.9" +version = "0.101.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d7df23109aa6c1567d1c575b9952556388da57401e4ace1d15f79eedad0d8f53" +checksum = "8b6275d1ee7a1cd780b64aca7726599a1dbc893b1e64144529e55c3c2f745765" dependencies = [ "ring", - "rustls-pki-types", "untrusted", ] @@ -1006,6 +993,16 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "sct" +version = "0.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da046153aa2352493d6cb7da4b6e5c0c057d8a1d0a9aa8560baffdd945acd414" +dependencies = [ + "ring", + "untrusted", +] + [[package]] name = "semver" version = "1.0.27" @@ -1106,12 +1103,6 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" -[[package]] -name = "subtle" -version = "2.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" - [[package]] name = "syn" version = "2.0.117" @@ -1244,18 +1235,17 @@ checksum = "8ecb6da28b8a351d773b68d5825ac39017e680750f980f3a1a85cd8dd28a47c1" [[package]] name = "ureq" -version = "2.12.1" +version = "2.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "02d1a66277ed75f640d608235660df48c8e3c19f3b4edb6a263315626cc3c01d" +checksum = "7830e33f6e25723d41a63f77e434159dad02919f18f55a512b5f16f3b1d77138" dependencies = [ "base64", - "flate2", "log", "once_cell", "rustls", - "rustls-pki-types", + "rustls-webpki", "url", - "webpki-roots 0.26.11", + "webpki-roots", ] [[package]] @@ -1409,21 +1399,9 @@ dependencies = [ [[package]] name = "webpki-roots" -version = "0.26.11" +version = "0.25.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "521bc38abb08001b01866da9f51eb7c5d647a19260e00054a8c7fd5f9e57f7a9" -dependencies = [ - "webpki-roots 1.0.6", -] - -[[package]] -name = "webpki-roots" -version = "1.0.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22cfaf3c063993ff62e73cb4311efde4db1efb31ab78a3e5c457939ad5cc0bed" -dependencies = [ - "rustls-pki-types", -] +checksum = "5f20c57d8d7db6d3b86154206ae5d8fba62dd39573114de97c2cb0578251f8e1" [[package]] name = "which" @@ -1834,12 +1812,6 @@ dependencies = [ "synstructure", ] -[[package]] -name = "zeroize" -version = "1.8.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" - [[package]] name = "zerotrie" version = "0.2.3" diff --git a/Cargo.toml b/Cargo.toml index f83c3e39..e900e283 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,8 @@ toml = "0.8" chrono = "0.4" tempfile = "3" sha2 = "0.10" -ureq = "2" +# W-3: Pin to exact version for TLS reproducibility +ureq = { version = "=2.9.0", default-features = false, features = ["tls"] } hostname = "0.4" getrandom = "0.4" flate2 = "1.0" diff --git a/SECURITY.md b/SECURITY.md index 2d06b77c..11d09842 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,6 +12,8 @@ If you discover a security vulnerability in RTK, please report it to the maintai - Open public GitHub issues for security vulnerabilities - Disclose vulnerabilities on social media or forums before we've had a chance to address them +We follow a **coordinated disclosure** process: vulnerabilities are reported privately, we develop a fix, and then disclose publicly. We request a 90-day embargo period from initial report to public disclosure. If you discover a vulnerability, please give us reasonable time to address it before public disclosure. + --- ## Security Review Process for Pull Requests @@ -25,6 +27,37 @@ RTK is a CLI tool that executes shell commands and handles user input. PRs from --- +## C-4: Local File Disclosure — Downgraded to Medium (SE-Requested) + +**File:** `src/core/tracking.rs` +**Severity:** 🟡 Medium +**Category:** Data Privacy + +**CVSS v3.1 Justification (Medium):** `AV:L / AC:L / PR:N / Scope:U / C:L / I:L / A:N` +- **Attack Vector: Local** — requires file system access to `~/.local/share/rtk/tracking.db` +- **Privileges Required:** None — file may be world-readable if umask is permissive (0027 or broader) +- **Confidentiality Impact:** Low — project path disclosure reveals work patterns but no credentials or secrets +- **Integrity Impact:** Low — data at rest, no direct modification risk from exposure alone + +**Revised severity per SE-REVIEW_2:** Downgraded from Critical to Medium. Rationale: Local file disclosure requires file system access but only reveals project structure, not credentials or secrets. Standard CVSS language applied. + +**Fix:** Use `age` for at-rest encryption or store only SHA-256 hashes of project paths. + +--- + +## M-1 → M-2: Process Isolation Reclassification (SE-Requested Downgrade) + +**Original classification (M-1):** Medium vulnerability +**Revised classification (M-2):** Hardening recommendation + +**Rationale:** This was originally categorized as a Medium vulnerability. Upon SE review, it was reclassified as a hardening recommendation. This is not a current vulnerability that requires a fix — it's a defense-in-depth measure that would strengthen the system if implemented. The reclassification aligns with security best practices: hardening recommendations provide guidance for strengthening systems without blocking normal operation. + +**Context:** This finding is about `src/core/runner.rs` — process isolation recommendation (bubblewrap sandbox, Firecracker, or equivalent) — to prevent lateral movement if a malicious URL is passed to `rtk curl`. This is a "best practice" defense-in-depth measure, not a "fix this bug" task. + +**Note:** For the original vulnerability classification rationale (prior to SE-REQUESTED_CHANGES downgrade), see the audit history in the PR comments or contact the maintainers. + +--- + ## Automated Security Checks Every PR triggers our [`security-check.yml`](.github/workflows/security-check.yml) workflow: @@ -178,7 +211,7 @@ let api_key = std::env::var("API_KEY").context("API_KEY not set")?; - Validate all user input before passing to `Command` - Use allowlists for command flags (not denylists) - Canonicalize file paths to prevent traversal attacks -- Sanitize package names with strict regex patterns +- Sanitize package names with strict regex patterns (avoid ReDoS — Regular Expression Denial of Service) --- diff --git a/install.sh b/install.sh index 1654245d..71a2cfd9 100644 --- a/install.sh +++ b/install.sh @@ -74,18 +74,40 @@ install() { info "Target: $TARGET" info "Version: $VERSION" + # C-1: Document execution chain + # Note: Expands to: sh -c "rtk rewrite \"\$CMD\"" (C-1 finding) + DOWNLOAD_URL="https://github.com/${REPO}/releases/download/${VERSION}/${BINARY_NAME}-${TARGET}.tar.gz" TEMP_DIR=$(mktemp -d) ARCHIVE="${TEMP_DIR}/${BINARY_NAME}.tar.gz" + # C-1: GitHub TLS caveat (C-1 finding) + # WARNING: GitHub's TLS 1.3 implementation mitigates network-level MITM attacks, but artifact integrity verification remains critical for supply chain security. + # This download does not include pinned checksums or GPG signature verification. Users should verify releases independently. + info "Downloading from: $DOWNLOAD_URL" if ! curl -fsSL "$DOWNLOAD_URL" -o "$ARCHIVE"; then error "Failed to download binary" fi info "Extracting..." + # C-7: Tar extraction safeguards (symlink + path traversal detection) tar -xzf "$ARCHIVE" -C "$TEMP_DIR" + # Check for symlinks or suspicious path entries + if find "$TEMP_DIR" -type l -print 2>/dev/null | grep -q '.'; then + error "SECURITY: Archive contains symlinks. Aborting." + rm -rf "$TEMP_DIR" + exit 1 + fi + + # Check for path traversal attempts (../) + if find "$TEMP_DIR" -name '..*' -print 2>/dev/null | grep -q .; then + error "SECURITY: Archive contains path traversal attempts. Aborting." + rm -rf "$TEMP_DIR" + exit 1 + fi + mkdir -p "$INSTALL_DIR" mv "${TEMP_DIR}/${BINARY_NAME}" "${INSTALL_DIR}/" diff --git a/src/cmds/cloud/curl_cmd.rs b/src/cmds/cloud/curl_cmd.rs index d6930ef6..8ef1c9e1 100644 --- a/src/cmds/cloud/curl_cmd.rs +++ b/src/cmds/cloud/curl_cmd.rs @@ -5,12 +5,59 @@ use crate::core::utils::{exit_code_from_output, resolved_command, truncate}; use crate::json_cmd; use anyhow::{Context, Result}; +// C-2: SSRF Risk mitigation (SE-requested change) +// Block known internal/metadata endpoints +const BLOCKED_DOMAINS: &[&str] = &[ + "169.254.169.254", // Private IPv4 range + "169.254.1.0", // 169.254.0.0/8 (RFC 1918 private use) + "metadata.google.internal", + "metadata.googleapis.com", +]; + +const BLOCKED_PATTERNS: &[&str] = &[ + "/latest/meta-data", // AWS metadata endpoint + "/.internal/", + "/localhost:", // Local service + "/127.", // Loopback +]; + +fn is_blocked_url(url: &str) -> bool { + // Check against blocked domains + for domain in BLOCKED_DOMAINS { + if url.contains(domain) { + return true; + } + } + + // Check against blocked patterns + for pattern in BLOCKED_PATTERNS { + if url.contains(pattern) { + return true; + } + } + + false +} + /// Not using run_filtered: on failure, curl can return HTML error pages (404, 500) /// that the JSON schema filter would mangle. The early exit skips filtering entirely. pub fn run(args: &[String], verbose: u8) -> Result { let timer = tracking::TimedExecution::start(); let mut cmd = resolved_command("curl"); - cmd.arg("-s"); // Silent mode (no progress bar) + cmd.arg("-s "); // Silent mode (no progress bar) + + // C-2: SSRF Risk mitigation (SE-requested change) + // Validate URL before execution + let target_url = if !args.is_empty() { + args[0].clone() + } else { + "https://example.com".to_string() // Fallback for validation + }; + + if is_blocked_url(&target_url) { + eprintln!("Blocked: SSRF - internal/metadata URL not allowed: {}", target_url); + return Ok(1); // SSRF block exit code + } for arg in args { cmd.arg(arg); diff --git a/src/core/tracking.rs b/src/core/tracking.rs index d6a248af..e3c7fe8a 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -40,6 +40,22 @@ use std::time::Instant; // ── Project path helpers ── // added: project-scoped tracking support /// Get the canonical project path string for the current working directory. +/// +/// SECURITY NOTE (C-3/C-4 — SE-requested downgrade): Local file disclosure finding downgraded to Medium per +/// SE-REVIEW_2 rationale: "Local file disclosure requires file system access and only reveals project structure, +/// not credentials or secrets. Standard CVSS language applied." +/// +/// The database at `~/.local/share/rtk/tracking.db` may have overly permissive file +/// permissions (0644), exposing: +/// - All project paths worked on +/// - Command frequency per project +/// - Token savings patterns per project +/// +/// Fix (v2 — SE hardened): `chmod 600` is insufficient — `DB` contains project path +/// fingerprints that can be correlated over time. Use `age` for at-rest encryption: +/// See SECURITY.md for CVSS v3.1 Medium classification (AV:L / AC:L / PR:N / Scope:U / C:L / I:L / A:N) +/// data. See SECURITY.md for full analysis. +/// fn current_project_path_string() -> String { std::env::current_dir() .ok() @@ -996,7 +1012,7 @@ pub struct ParseFailureSummary { } /// Record a parse failure without ever crashing. -/// Silently ignores all errors — used in the fallback path. +/// Silently ignores all errors - used in the fallback path. pub fn record_parse_failure_silent(raw_command: &str, error_message: &str, succeeded: bool) { if let Ok(tracker) = Tracker::new() { let _ = tracker.record_parse_failure(raw_command, error_message, succeeded); @@ -1164,7 +1180,7 @@ pub fn args_display(args: &[OsString]) -> String { mod tests { use super::*; - // 1. estimate_tokens — verify ~4 chars/token ratio + // 1. estimate_tokens - verify ~4 chars/token ratio #[test] fn test_estimate_tokens() { assert_eq!(estimate_tokens(""), 0); @@ -1174,7 +1190,7 @@ mod tests { assert_eq!(estimate_tokens("12345678"), 2); // 8 chars = 2 tokens } - // 2. args_display — format OsString vec + // 2. args_display - format OsString vec #[test] fn test_args_display() { let args = vec![OsString::from("status"), OsString::from("--short")]; @@ -1185,7 +1201,7 @@ mod tests { assert_eq!(args_display(&single), "log"); } - // 3. Tracker::record + get_recent — round-trip DB + // 3. Tracker::record + get_recent - round-trip DB #[test] fn test_tracker_record_and_recent() { let tracker = Tracker::new().expect("Failed to create tracker");