Permalink
Browse files

Fix security checks in `PathBuf::FromSegments`.

In #134, @tunz discovered that Rocket does not properly prevent path traversal
or local file inclusion attacks. The issue is caused by a failure to check for
some dangerous characters after decoding. In this case, the path separator '/'
was left as-is after decoding. As such, an attacker could construct a path with
containing any number of `..%2f..` sequences to traverse the file system.

This commit resolves the issue by ensuring that the decoded segment does not
contains any `/` characters. It further hardens the `FromSegments`
implementation by checking for additional risky characters: ':', '>', '<' as the
last character, and '\' on Windows. This is in addition to the already present
checks for '.' and '*' as the first character.

The behavior for a failing check has also changed. Previously, Rocket would skip
segments that contained illegal characters. In this commit, the implementation
instead return an error.

The `Error` type of the `PathBuf::FromSegment` implementations was changed to a
new `SegmentError` type that indicates the condition that failed.

Closes #134.
  • Loading branch information...
SergioBenitez committed Jan 13, 2017
1 parent 41aecc3 commit 4bc5c20a45757be6b33e5fe9530e759d6c5bb032
Showing with 53 additions and 11 deletions.
  1. +2 −2 codegen/tests/run-pass/segments.rs
  2. +14 −0 lib/src/http/uri.rs
  3. +37 −9 lib/src/request/param.rs
@@ -4,15 +4,15 @@
extern crate rocket;
use std::path::PathBuf;
use std::str::Utf8Error;
use rocket::http::uri::SegmentError;
#[post("/<a>/<b..>")]
fn get(a: String, b: PathBuf) -> String {
format!("{}/{}", a, b.to_string_lossy())
}
#[post("/<a>/<b..>")]
fn get2(a: String, b: Result<PathBuf, Utf8Error>) -> String {
fn get2(a: String, b: Result<PathBuf, SegmentError>) -> String {
format!("{}/{}", a, b.unwrap().to_string_lossy())
}
View
@@ -373,6 +373,20 @@ impl<'a> Iterator for Segments<'a> {
// }
}
/// Errors which can occur when attempting to interpret a segment string as a
/// valid path segment.
#[derive(Debug, PartialEq, Eq, Clone)]
pub enum SegmentError {
/// The segment contained invalid UTF8 characters when percent decoded.
Utf8(Utf8Error),
/// The segment started with the wrapped invalid character.
BadStart(char),
/// The segment contained the wrapped invalid character.
BadChar(char),
/// The segment ended with the wrapped invalid character.
BadEnd(char),
}
#[cfg(test)]
mod tests {
use super::URI;
View
@@ -1,9 +1,9 @@
use std::str::{Utf8Error, FromStr};
use std::str::FromStr;
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV4, SocketAddrV6, SocketAddr};
use std::path::PathBuf;
use std::fmt::Debug;
use http::uri::{URI, Segments};
use http::uri::{URI, Segments, SegmentError};
/// Trait to convert a dynamic path segment string to a concrete value.
///
@@ -274,26 +274,54 @@ pub trait FromSegments<'a>: Sized {
impl<'a> FromSegments<'a> for Segments<'a> {
type Error = ();
fn from_segments(segments: Segments<'a>) -> Result<Segments<'a>, ()> {
Ok(segments)
}
}
/// Creates a `PathBuf` from a `Segments` iterator. The returned `PathBuf` is
/// percent-decoded. If a segment is equal to "..", the previous segment (if
/// any) is skipped. For security purposes, any other segments that begin with
/// "*" or "." are ignored. If a percent-decoded segment results in invalid
/// UTF8, an `Err` is returned.
/// any) is skipped.
///
/// For security purposes, if a segment meets any of the following conditions,
/// an `Err` is returned indicating the condition met:
///
/// * Decoded segment starts with any of: `.`, `*`
/// * Decoded segment ends with any of: `:`, `>`, `<`
/// * Decoded segment contains any of: `/`
/// * On Windows, decoded segment contains any of: '\'
/// * Percent-encoding results in invalid UTF8.
///
/// As a result of these conditions, a `PathBuf` derived via `FromSegments` is
/// safe to interpolate within, or use as a suffix of, a path without additional
/// checks.
impl<'a> FromSegments<'a> for PathBuf {
type Error = Utf8Error;
type Error = SegmentError;
fn from_segments(segments: Segments<'a>) -> Result<PathBuf, Utf8Error> {
fn from_segments(segments: Segments<'a>) -> Result<PathBuf, SegmentError> {
let mut buf = PathBuf::new();
for segment in segments {
let decoded = URI::percent_decode(segment.as_bytes())?;
let decoded = URI::percent_decode(segment.as_bytes())
.map_err(|e| SegmentError::Utf8(e))?;
if decoded == ".." {
buf.pop();
} else if !(decoded.starts_with('.') || decoded.starts_with('*')) {
} else if decoded.starts_with('.') {
return Err(SegmentError::BadStart('.'))
} else if decoded.starts_with('*') {
return Err(SegmentError::BadStart('*'))
} else if decoded.ends_with(':') {
return Err(SegmentError::BadEnd(':'))
} else if decoded.ends_with('>') {
return Err(SegmentError::BadEnd('>'))
} else if decoded.ends_with('<') {
return Err(SegmentError::BadEnd('<'))
} else if decoded.contains('/') {
return Err(SegmentError::BadChar('/'))
} else if cfg!(windows) && decoded.contains('\\') {
return Err(SegmentError::BadChar('\\'))
} else {
buf.push(&*decoded)
}
}

0 comments on commit 4bc5c20

Please sign in to comment.