Prevent bypassing safe guard in pathbuf segments

[tunz]

Problem

In the example static_file, there exist a LFI vulnerability. An attacker can read any file from the server.

For example,

$ curl 'http://localhost:8000/hidden%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd'

Invoking the curl command may show you the content of /etc/passwd that must not be disclosed.

It seems that Rocket is already filtering .. for PathBuf in params.rs. But, URI decode is invoked after segmenting, so the filtering can be bypassed.

Fix

I fixed it by creating another segments for each URI decoded segment. Since I'm new to Rust and Rocket, I'm not sure if it is the best solution or not. I think we can call URI decode before segmenting, but it concerns me to make a big change.

Feel free to give me any feedback.

[SergioBenitez]

Okay, let's get a fix in as soon as possible.

I think that if a '/' character is in the URL-decoded segment, than it's an obvious attempt to subvert, and we can immediately bail. So, I think Rocket should simply reject that kind of string. But I'm not sure what others might expect.

Is there ever a valid case to encode a '/' with %2f? If not, we can bail if a decoding ever results in a '/' appearing.

[tunz]

I also cannot expect any usual case for that. I think if any user want this special case, then they can use Segments type instead of PathBuf type. So, I think we can simply reject it.

Does the bail means 404 error?

One more, does we also need to reject \ for windows? Since I'm not a windows user, I cannot test it, but I heard that PathBuf supports \.

[SergioBenitez]

Bail in this case means that PathBuf::from_segments fails. The user can catch the error if they'd like, but if they don't, it results in a forward, which will ultimately result in a 404.

I think we should also reject '\'.

[SergioBenitez]

Okay, I've gone through OWASP guide and have come up with the following implementation:

/// 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, 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 with any of: `/`, `\`
///   * Percent-encoding results in invalid UTF8.
///
/// As a result of these conditions, a `PathBuf` derived `FromSegments` is safe
/// to use as the suffix of a relative path without additional checks.
impl<'a> FromSegments<'a> for PathBuf {
    type Error = SegmentError;

    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())
                .map_err(|e| SegmentError::Utf8(e))?;

            if decoded == ".." {
                buf.pop();
            } 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 decoded.contains('\\') {
                return Err(SegmentError::BadChar('\\'))
            } else {
                buf.push(&*decoded)
            }
        }

        Ok(buf)
    }
}

Does this look good to you?

[tunz]

I think checking only \ is too strong because \ is a valid character in macOS or Linux. maybe we can check ..\ instead.

[SergioBenitez]

We can make it so that the check happens only on Windows via cfg(target_os = "windows"). That sound okay?

[tunz]

Okey, that looks good.

[SergioBenitez]

Thank you so much for finding and reporting this. You are absolutely awesome.

[tunz]

Thank you for the fix!