fix(webdav): constant-time compare on lock-token equality checks
Replace plain `==` on lock tokens with `subtle::ConstantTimeEq` at every token-comparison site on the WebDAV surface. Closes a reported timing side-channel (2026-09-05) in `evaluate_if_header` where an authenticated attacker could theoretically recover another user's active lock token via response-latency measurements on the `If:` header state-token comparison. Practical exploitability is marginal — the signal is tens-of-ns buried under ms-scale network jitter, ~5×10⁸ samples needed per token to average through the noise vs a default lock lifetime of 60 s to 1 h — but the fix is a five-line change with zero measurable perf cost (`subtle` is already transitive via sqlx-postgres → digest, so no new binary weight), and adopting constant-time compare on any token that gates access matches the hygiene rule the rest of the codebase already follows on password and session paths. Sites fixed: * `evaluate_if_header` — first-pass state-token scan and second-pass condition eval in `webdav_handler.rs`. * `WebdavLockService::refresh` — `!= token` mismatch check. * `WebdavLockService::release` — `== token` guard on the by_path invalidation branch. The two `WebdavLockService` sites are already gated by `self.by_token.get(token)?` — the attacker cannot reach the comparison without already presenting a valid token, so their timing surface is nil in practice. Kept constant-time anyway for callsite consistency. Sweep confirmed no other secret-adjacent `==` in production code: password verification goes through Argon2's `verify_password`, session/CSRF/DPoP jti tokens are hashmap-gated, and blob-hash equality compares two server-side values with no attacker- controlled operand. Reported-by: Abdurazzoqov Javohir <abdurazzoqovjavohir700-dev@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Generated
+1
@@ -4705,6 +4705,7 @@ dependencies = [
|
|||||||
"smol_str",
|
"smol_str",
|
||||||
"socket2 0.6.4",
|
"socket2 0.6.4",
|
||||||
"sqlx",
|
"sqlx",
|
||||||
|
"subtle",
|
||||||
"tantivy",
|
"tantivy",
|
||||||
"tempfile",
|
"tempfile",
|
||||||
"testcontainers-modules",
|
"testcontainers-modules",
|
||||||
|
|||||||
@@ -125,6 +125,13 @@ mp3-duration = "0.1"
|
|||||||
kamadak-exif = "0.6.1"
|
kamadak-exif = "0.6.1"
|
||||||
md-5 = "0.11.0"
|
md-5 = "0.11.0"
|
||||||
sha2 = "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"
|
unicode-normalization = "0.1.25"
|
||||||
blake3 = { version = "1.8.5", features = ["rayon", "mmap"] }
|
blake3 = { version = "1.8.5", features = ["rayon", "mmap"] }
|
||||||
hex = "0.4.3"
|
hex = "0.4.3"
|
||||||
|
|||||||
@@ -19,8 +19,26 @@
|
|||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
use std::time::{Duration, Instant};
|
use std::time::{Duration, Instant};
|
||||||
|
|
||||||
|
use subtle::ConstantTimeEq;
|
||||||
|
|
||||||
use crate::application::adapters::webdav_adapter::{LockInfo, LockScope};
|
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).
|
/// Default lock timeout when the client does not specify one (RFC 4918 §10.7).
|
||||||
const DEFAULT_LOCK_TIMEOUT_SECS: u64 = 1800; // 30 minutes
|
const DEFAULT_LOCK_TIMEOUT_SECS: u64 = 1800; // 30 minutes
|
||||||
|
|
||||||
@@ -166,7 +184,7 @@ impl WebDavLockStore {
|
|||||||
let path = self.by_token.get(token)?;
|
let path = self.by_token.get(token)?;
|
||||||
let mut entry = self.by_path.get(&path)?;
|
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
|
return None; // token mismatch — lock was replaced
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -188,7 +206,7 @@ impl WebDavLockStore {
|
|||||||
if let Some(path) = self.by_token.get(token) {
|
if let Some(path) = self.by_token.get(token) {
|
||||||
// Only remove from by_path if the token still matches
|
// Only remove from by_path if the token still matches
|
||||||
if let Some(entry) = self.by_path.get(&path)
|
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);
|
self.by_path.invalidate(&path);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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 percent_encoding::{AsciiSet, NON_ALPHANUMERIC, percent_decode_str, utf8_percent_encode};
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
use subtle::ConstantTimeEq;
|
||||||
|
|
||||||
/// Characters that MUST NOT be percent-encoded inside a URI path segment.
|
/// Characters that MUST NOT be percent-encoded inside a URI path segment.
|
||||||
/// RFC 3986 §3.3 pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
|
/// RFC 3986 §3.3 pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
|
||||||
@@ -1581,6 +1582,34 @@ fn parse_if_header(header: &str) -> IfLists {
|
|||||||
lists
|
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.
|
/// Evaluate a parsed `If:` header against the current resource state.
|
||||||
///
|
///
|
||||||
/// Returns `(header_true, submitted_active_lock)`:
|
/// Returns `(header_true, submitted_active_lock)`:
|
||||||
@@ -1614,7 +1643,7 @@ fn evaluate_if_header(
|
|||||||
negated: false,
|
negated: false,
|
||||||
token,
|
token,
|
||||||
} = cond
|
} = cond
|
||||||
&& token == active
|
&& ct_str_eq(token, active)
|
||||||
{
|
{
|
||||||
submitted_active_lock = true;
|
submitted_active_lock = true;
|
||||||
}
|
}
|
||||||
@@ -1627,7 +1656,14 @@ fn evaluate_if_header(
|
|||||||
list.iter().all(|cond| {
|
list.iter().all(|cond| {
|
||||||
let (negated, natural) = match cond {
|
let (negated, natural) = match cond {
|
||||||
IfCondition::StateToken { negated, token } => {
|
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)
|
(*negated, is_active)
|
||||||
}
|
}
|
||||||
IfCondition::EntityTag {
|
IfCondition::EntityTag {
|
||||||
|
|||||||
Reference in New Issue
Block a user