diff --git a/Cargo.lock b/Cargo.lock index 96c38234..1dd449b5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4705,6 +4705,7 @@ dependencies = [ "smol_str", "socket2 0.6.4", "sqlx", + "subtle", "tantivy", "tempfile", "testcontainers-modules", diff --git a/Cargo.toml b/Cargo.toml index 16cbb6c6..aeb0fa2d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -125,6 +125,13 @@ mp3-duration = "0.1" kamadak-exif = "0.6.1" md-5 = "0.11.0" sha2 = "0.11.0" +# Constant-time equality primitives for security-sensitive comparisons. +# Direct dep is free: `subtle` is already pulled in transitively via +# sqlx-postgres → sha2 → digest, so this doesn't add a compile unit or +# bytes — just makes the import explicit for our own callsites (WebDAV +# lock-token comparison in `evaluate_if_header`, and any future +# token/secret comparisons). +subtle = "2.6" unicode-normalization = "0.1.25" blake3 = { version = "1.8.5", features = ["rayon", "mmap"] } hex = "0.4.3" diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 00000000..ce98578b --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,118 @@ +# Security Policy + +## Reporting a Vulnerability + +Please report security issues through **GitHub Security Advisories** — the +private-disclosure channel integrated with this repository: + + + +If GitHub isn't a viable channel for you (organisational policy, no GH +account, etc.), email a maintainer directly at +. Please include the word `security` +in the subject line so it routes ahead of general project mail. + +**Please do NOT open public GitHub issues for security vulnerabilities.** +A public issue makes the finding available to attackers before the fix +ships, which is exactly what we're trying to avoid. + +## What to include + +A short, specific report is far more useful than a long generic one. If +you can share: + +- A concise description of the issue and its impact +- Steps to reproduce, or a proof-of-concept if one exists +- Affected component (REST API, WebDAV, NextCloud DAV, CalDAV, CardDAV, + WOPI, auth, frontend, …) +- Affected version or commit hash +- Any remediation you've already identified + +Both executed exploits AND code-review findings are welcome — mention +which one it is (e.g. "I found this pattern in the source but couldn't +build the service to verify runtime") so we know what to expect. + +If you're not sure whether something is a vulnerability, err on the +side of reporting; we'd rather triage a false positive than miss a +real issue. + +## Response expectations + +OxiCloud is maintained by a small volunteer team. Realistic timelines: + +- **Acknowledgement:** within 5 business days +- **Initial triage and severity assessment:** within 14 days +- **Fix on `main`:** timeline depends on complexity, communicated during + triage +- **Public disclosure:** coordinated with the reporter, typically after + the fix has been available on `main` long enough for downstream users + to update + +If you don't hear back within 5 business days, please ping the private +advisory or resend the email — reports occasionally get missed. + +## Scope + +**In scope:** + +- Server (`src/`) — anything reachable via the HTTP surface + (REST API, WebDAV, NextCloud DAV, CalDAV, CardDAV, WOPI) or via + authentication / authorization / session management +- Frontend (`frontend/`) — client-side issues (XSS, CSRF gaps, + insecure client-side storage, DOM sinks) +- Build artifacts and release tarballs — supply-chain and packaging + integrity +- The migration and background-job surfaces — anything an authenticated + user or admin can trigger + +**Out of scope** (please don't spend your time on these): + +- Missing security headers on non-authenticated public endpoints + (already tracked) +- Rate-limit tuning suggestions +- Denial-of-service via resource exhaustion at scales beyond the + documented deployment guidance +- Findings that require attacker-controlled physical or root access to + the server host +- Vulnerabilities in third-party dependencies that don't have a + reachable path from OxiCloud code (report those upstream) + +## Safe-harbor + +We won't pursue legal action against researchers acting in good faith +under this policy — that means: + +- Not accessing or modifying data belonging to other users beyond the + minimum needed to demonstrate the issue +- Not degrading service for others (no volumetric testing without + coordination) +- Reporting the issue privately before any public disclosure +- Giving us reasonable time to fix before publishing + +Testing against your own self-hosted instance is always fine. Testing +against a third-party OxiCloud deployment requires explicit permission +from that deployment's operator. + +## Credit + +We're happy to credit reporters in the fix commit, release notes, and +this file's history. Tell us your preferred name or handle when you +report, or say if you'd rather stay anonymous. + +## Prior reports + +Coordinated disclosures we've received and resolved: + +- **2026-09-05** — Timing side-channel in WebDAV lock-token comparison + (`evaluate_if_header` in `src/interfaces/api/handlers/webdav_handler.rs` + used plain `==` on state-tokens, byte-wise with early exit). Fixed by + routing lock-token comparisons through a `subtle::ConstantTimeEq` + helper. Practical exploitability was marginal (ns-scale signal buried + in ms-scale network jitter, ~5×10⁸ samples required within the lock's + default 60 s–1 h lifetime), but the fix is small and matches the + constant-time-compare hygiene applied elsewhere in the codebase. + Reported by **Abdurazzoqov Javohir** + ([@abdurazzoqovjavohir700-dev](https://github.com/abdurazzoqovjavohir700-dev)) + via responsible disclosure; fix landed in + [PR #712](https://github.com/AtalayaLabs/OxiCloud/pull/712). Thanks + for the clear report and the specific remediation suggestion. diff --git a/src/infrastructure/services/webdav_lock_service.rs b/src/infrastructure/services/webdav_lock_service.rs index 9bedaf99..fc15bdb5 100644 --- a/src/infrastructure/services/webdav_lock_service.rs +++ b/src/infrastructure/services/webdav_lock_service.rs @@ -19,8 +19,26 @@ use std::sync::Arc; use std::time::{Duration, Instant}; +use subtle::ConstantTimeEq; + use crate::application::adapters::webdav_adapter::{LockInfo, LockScope}; +/// Constant-time equality for lock tokens. Same rationale as +/// `webdav_handler::ct_str_eq` — see that helper's doc-comment. +/// +/// The two callsites in this file (`refresh` at :169, `release` +/// at :191) are already gated by `self.by_token.get(token)?`, so +/// the attacker CANNOT reach these checks without already having +/// presented a valid token — the practical timing-attack surface is +/// nil. Kept constant-time for defense-in-depth consistency across +/// every token comparison in the WebDAV surface, so a future +/// auditor doesn't have to re-derive "this one is safe because…" +/// for each individual callsite. +#[inline] +fn ct_str_eq(a: &str, b: &str) -> bool { + a.len() == b.len() && a.as_bytes().ct_eq(b.as_bytes()).into() +} + /// Default lock timeout when the client does not specify one (RFC 4918 §10.7). const DEFAULT_LOCK_TIMEOUT_SECS: u64 = 1800; // 30 minutes @@ -166,7 +184,7 @@ impl WebDavLockStore { let path = self.by_token.get(token)?; let mut entry = self.by_path.get(&path)?; - if entry.info.token != token { + if !ct_str_eq(&entry.info.token, token) { return None; // token mismatch — lock was replaced } @@ -188,7 +206,7 @@ impl WebDavLockStore { if let Some(path) = self.by_token.get(token) { // Only remove from by_path if the token still matches if let Some(entry) = self.by_path.get(&path) - && entry.info.token == token + && ct_str_eq(&entry.info.token, token) { self.by_path.invalidate(&path); } diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 22fd0374..780d9815 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -46,6 +46,7 @@ use crate::interfaces::upload_ingest::{IngestedBlob, RangeSegment, discard_inges use percent_encoding::{AsciiSet, NON_ALPHANUMERIC, percent_decode_str, utf8_percent_encode}; use std::collections::HashMap; use std::sync::Arc; +use subtle::ConstantTimeEq; /// Characters that MUST NOT be percent-encoded inside a URI path segment. /// RFC 3986 §3.3 pchar = unreserved / pct-encoded / sub-delims / ":" / "@" @@ -1581,6 +1582,34 @@ fn parse_if_header(header: &str) -> IfLists { lists } +/// Constant-time string equality for security-sensitive tokens +/// (WebDAV lock State-tokens today; extend for future session / +/// secret-adjacent comparisons if any). +/// +/// Rust's built-in `str::eq` compares byte-wise with early exit on +/// mismatch — the position of the differing byte is observable via +/// timing. For WebDAV lock tokens the practical exploit is not +/// realistic (ns-scale signal buried under ms-scale network jitter, +/// plus ~5×10⁸ samples needed to average through the noise before +/// the lock expires), but the fix is a 5-line change with zero +/// measurable perf cost and matches the "constant-time compare on +/// any token that gates access" hygiene rule the rest of the code +/// follows on session tokens. Reported responsibly on 2026-09-05. +/// +/// Length leaks are acceptable here — WebDAV lock tokens have a +/// fixed public format (`opaquelocktoken:`), so the length is +/// not secret and any timing distinguishability from a length +/// mismatch reveals nothing an attacker doesn't already know from +/// the URI grammar. +#[inline] +fn ct_str_eq(a: &str, b: &str) -> bool { + // `ct_eq` returns 1 on match, 0 on mismatch — same length always, + // no early exit within the byte compare. Different-length inputs + // still short-circuit at the length check (see doc note above), + // and equal-length inputs run the full constant-time compare. + a.len() == b.len() && a.as_bytes().ct_eq(b.as_bytes()).into() +} + /// Evaluate a parsed `If:` header against the current resource state. /// /// Returns `(header_true, submitted_active_lock)`: @@ -1614,7 +1643,7 @@ fn evaluate_if_header( negated: false, token, } = cond - && token == active + && ct_str_eq(token, active) { submitted_active_lock = true; } @@ -1627,7 +1656,14 @@ fn evaluate_if_header( list.iter().all(|cond| { let (negated, natural) = match cond { IfCondition::StateToken { negated, token } => { - let is_active = active_lock_token == Some(token.as_str()); + // Constant-time compare (see `ct_str_eq` above). + // `active_lock_token = None` short-circuits at the + // outer `Some(_)` match — that branch is only + // reachable when a lock actually exists, so the + // "no lock present" fast path stays public info. + let is_active = active_lock_token + .map(|a| ct_str_eq(token, a)) + .unwrap_or(false); (*negated, is_active) } IfCondition::EntityTag {