Merge pull request #712 from EdouardVanbelle/fix/webdav-security

Cargo.lock

@@ -4705,6 +4705,7 @@ dependencies = [
  "smol_str",
  "socket2 0.6.4",
  "sqlx",
+ "subtle",
  "tantivy",
  "tempfile",
  "testcontainers-modules",

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"

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:
+
+  <https://github.com/EdouardVanbelle/OxiCloud/security/advisories/new>
+
+If GitHub isn't a viable channel for you (organisational policy, no GH
+account, etc.), email a maintainer directly at
+<opensource+security@edouard.vanbelle.fr>. 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.

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);
             }

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:<UUID>`), 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 {