Skip to content

Commit a89b23e

Browse files
committed
fix(security): Add path normalization defense-in-depth to FilterPolicy
Priority: MEDIUM Issue: Path traversal sequences (e.g., /var/../etc/passwd) were not automatically normalized in FilterPolicy::check(), potentially allowing filter bypass if callers did not explicitly normalize paths. Changes: - Add normalize_path() call in FilterPolicy::check() to automatically normalize paths before matching against rules - Add test verifying path traversal protection at policy level - Fix clippy lint: NoOpFilter::default() -> NoOpFilter Review-Iteration: 1
1 parent e58b127 commit a89b23e

2 files changed

Lines changed: 47 additions & 5 deletions

File tree

src/server/filter/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@ mod tests {
515515

516516
#[test]
517517
fn test_noop_filter_default() {
518-
let filter = NoOpFilter::default();
518+
let filter = NoOpFilter;
519519
assert!(!filter.is_enabled());
520520
}
521521

src/server/filter/policy.rs

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ use super::{FilterResult, Operation, TransferFilter};
2727
use crate::server::config::{
2828
CompositeLogicType, FilterAction, FilterConfig, FilterRule as FilterRuleConfig, MatcherConfig,
2929
};
30-
use crate::server::filter::path::{ComponentMatcher, MultiExtensionMatcher, PrefixMatcher};
30+
use crate::server::filter::path::{normalize_path, ComponentMatcher, MultiExtensionMatcher, PrefixMatcher};
3131
use crate::server::filter::pattern::{AllMatcher, CombinedMatcher, GlobMatcher, NotMatcher};
3232

3333
/// Trait for path matchers.
@@ -353,11 +353,18 @@ impl TransferFilter for FilterPolicy {
353353
return FilterResult::Allow;
354354
}
355355

356+
// Normalize path to prevent path traversal attacks (e.g., /var/../etc/passwd -> /etc/passwd)
357+
// This is a defense-in-depth measure - callers should also validate paths,
358+
// but we normalize here to ensure consistent security behavior.
359+
let normalized = normalize_path(path);
360+
let check_path = normalized.as_path();
361+
356362
for rule in &self.rules {
357-
if rule.matches(path, operation, user) {
363+
if rule.matches(check_path, operation, user) {
358364
tracing::debug!(
359365
rule_name = ?rule.name,
360-
path = %path.display(),
366+
path = %check_path.display(),
367+
original_path = %path.display(),
361368
operation = %operation,
362369
user = %user,
363370
action = %rule.action,
@@ -369,7 +376,7 @@ impl TransferFilter for FilterPolicy {
369376
}
370377

371378
tracing::trace!(
372-
path = %path.display(),
379+
path = %check_path.display(),
373380
operation = %operation,
374381
user = %user,
375382
action = %self.default_action,
@@ -1144,4 +1151,39 @@ mod tests {
11441151
FilterResult::Allow
11451152
);
11461153
}
1154+
1155+
#[test]
1156+
fn test_policy_path_traversal_protection() {
1157+
// Test that path traversal attempts are properly normalized and matched
1158+
let policy = FilterPolicy::new()
1159+
.add_rule(FilterRule::new(
1160+
Box::new(PrefixMatcher::new("/etc")),
1161+
FilterResult::Deny,
1162+
))
1163+
.with_default(FilterResult::Allow);
1164+
1165+
// Direct path should be denied
1166+
assert_eq!(
1167+
policy.check(Path::new("/etc/passwd"), Operation::Download, "user"),
1168+
FilterResult::Deny
1169+
);
1170+
1171+
// Path traversal attempt should also be denied (normalized to /etc/passwd)
1172+
assert_eq!(
1173+
policy.check(Path::new("/var/../etc/passwd"), Operation::Download, "user"),
1174+
FilterResult::Deny
1175+
);
1176+
1177+
// Another traversal pattern
1178+
assert_eq!(
1179+
policy.check(Path::new("/home/user/../../etc/shadow"), Operation::Download, "user"),
1180+
FilterResult::Deny
1181+
);
1182+
1183+
// Path outside /etc should be allowed
1184+
assert_eq!(
1185+
policy.check(Path::new("/home/user/file.txt"), Operation::Download, "user"),
1186+
FilterResult::Allow
1187+
);
1188+
}
11471189
}

0 commit comments

Comments
 (0)