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.
This commit is contained in:
Sergio Benitez 2017-01-13 13:25:33 -08:00
parent 855d9b7b00
commit d58c704d23
3 changed files with 53 additions and 11 deletions

View File

@ -4,7 +4,7 @@
extern crate rocket; extern crate rocket;
use std::path::PathBuf; use std::path::PathBuf;
use std::str::Utf8Error; use rocket::http::uri::SegmentError;
#[post("/<a>/<b..>")] #[post("/<a>/<b..>")]
fn get(a: String, b: PathBuf) -> String { fn get(a: String, b: PathBuf) -> String {
@ -12,7 +12,7 @@ fn get(a: String, b: PathBuf) -> String {
} }
#[post("/<a>/<b..>")] #[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()) format!("{}/{}", a, b.unwrap().to_string_lossy())
} }

View File

@ -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)] #[cfg(test)]
mod tests { mod tests {
use super::URI; use super::URI;

View File

@ -1,9 +1,9 @@
use std::str::{Utf8Error, FromStr}; use std::str::FromStr;
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV4, SocketAddrV6, SocketAddr}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV4, SocketAddrV6, SocketAddr};
use std::path::PathBuf; use std::path::PathBuf;
use std::fmt::Debug; 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. /// Trait to convert a dynamic path segment string to a concrete value.
/// ///
@ -274,6 +274,7 @@ pub trait FromSegments<'a>: Sized {
impl<'a> FromSegments<'a> for Segments<'a> { impl<'a> FromSegments<'a> for Segments<'a> {
type Error = (); type Error = ();
fn from_segments(segments: Segments<'a>) -> Result<Segments<'a>, ()> { fn from_segments(segments: Segments<'a>) -> Result<Segments<'a>, ()> {
Ok(segments) Ok(segments)
} }
@ -281,19 +282,46 @@ impl<'a> FromSegments<'a> for Segments<'a> {
/// Creates a `PathBuf` from a `Segments` iterator. The returned `PathBuf` is /// Creates a `PathBuf` from a `Segments` iterator. The returned `PathBuf` is
/// percent-decoded. If a segment is equal to "..", the previous segment (if /// percent-decoded. If a segment is equal to "..", the previous segment (if
/// any) is skipped. For security purposes, any other segments that begin with /// any) is skipped.
/// "*" or "." are ignored. If a percent-decoded segment results in invalid ///
/// UTF8, an `Err` is returned. /// 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 { 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(); let mut buf = PathBuf::new();
for segment in segments { 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 == ".." { if decoded == ".." {
buf.pop(); 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) buf.push(&*decoded)
} }
} }