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.

codegen/tests/run-pass/segments.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())
 }
 

lib/src/http/uri.rs

@@ -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;

lib/src/request/param.rs

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