ServeDir/File: Fix build_and_validate_path to prevent hacker from accessing arbitrary files

[c5soft]

Motivation

Current version 0.2.0 release has a high risk hidden danger that allowed a hacker to access arbitrary files in server.
When axum+tower-http-ServeDir static web server started with Windows 10 at driver D:\any-folder, a hacker can access any files at driver C:, for example:
http://127.0.0.1/anypath/c:/windows/win.ini will get some lines of text and
http://127.0.0.1/anypath/c:/windows/web/screen/img101.png will get a image

Solution

Fix build_and_validate_path function to exclude colon charactor #204

[davidpdrsn]

Thanks for finding this!

[davidpdrsn]

Have to do some investigation to figure out if not allowing : in paths is okay.

I don't have access to a windows machine but are you able test if other frameworks do the same as tower-http? Warp, tide, and actix-web would be cool to look at.

This validation code was originally copied from warp.

[c5soft]

I have tested the warp, unfortunately，warp 0.3.2 has exactly the same bug. I don't care about othor crates.

I was wondering if the bug originally from fs:::open, I have tested both std::fs and tokio::fs, both have no su
ch hidden danger, instead, both paniced with fs::open("d:/js/onlyone/dist/c:/windows/win.ini").

[adumbidiot]

I can confirm this issue on Warp and Tower. This is seems only exploitable on Windows and the server must have a current working directory that is different from the target disk's, though it still allows the user to access arbitrary paths from the working directory. The core of this issue boils down to PathBuf's push method replacing itself if it is given an absolute path or path with a prefix.

I enabled logging for the warp example to see what files the program tried to access, and found that it tried to access c:windows\\win.ini. This seems to be because the PathBuf impl does not push a separator for drive letters. This path is interpreted as a "relative path from the current directory of the... drive". From testing, this seems to mean if the user has a current working directory in that drive, it it used. If not, it seems to use the root of the disk. This means that an attack can potentially view all files below the current working directory and all files of another disk, assuming the program has the permissions to do so.

Instead of just rejecting : in paths outright (seems like it's valid on paths for Linux), I think the validation program should be edited to only accept relative path components while pushing to the final PathBuf. I think that there should be the normal check for \. Then, the function should convert the string to a Path, then check if it is relative and that is does not contain a prefix before pushing. I believe this validation can be performed with:

// Only accept current dir `.` and relative paths `a`, `a/b`.
let is_valid = path.components().all(|comp| matches!(comp, Component::Normal(_) | Component::CurDir));

I also believe that this check ensures that .. cannot exist in the path, making the prior check for this redundant.

[adumbidiot]

I've opened seanmonstar/warp#937 in warp to let them know about it since this seems important and I saw no other issues like it.

[adumbidiot]

I think Tide is ok. They have a check on the requested file paths to ensure that the requested file's path is prefixed by the served dir's path, which I think prevents this issue.

[adumbidiot]

I believe actix is also not affected by this. They seem to disallow path segments to end with :, which are needed to specify the drive prefix. I tried a couple other prefixes, but they seem to have guards for them too.

[davidpdrsn]

@adumbidiot Thank you that detailed explanation! I've pushed some changes to this branch that uses Path::components and checks that the final path we build is relative (which required fixing some tests). Do you want to take a look?

@c5soft I do return 404 if the path contains a "prefix component" but according to the docs that only appears on windows. Do you want to verify that my changes actually fixes the problem?

[c5soft]

@davidpdrsn I am test "prefix component" with following code:

fn build_and_validate_path(base_path: &Path, requested_path: &str) -> Option<PathBuf> {
    let mut full_path = base_path.to_path_buf();

    // build and validate the path
    let path = requested_path.trim_start_matches('/');
    let path_decoded = percent_decode(path.as_ref()).decode_utf8().ok()?;
    // for seg in path_decoded.split('/') {
    //     if seg.starts_with("..") || seg.contains('\\') || seg.contains(':') {
    //         return None;
    //     }
    //     full_path.push(seg);
    // }

    let path_decoded = Path::new(&*path_decoded);
    for component in path_decoded.components() {
        match component {
            std::path::Component::Normal(comp) =>{
                 println!("204 debug: {:?}",comp);
                 full_path.push(comp)
                },
            std::path::Component::CurDir => {}
            std::path::Component::Prefix(_)
            | std::path::Component::RootDir
            | std::path::Component::ParentDir => return None,
        }
    }
    Some(full_path)
}

and i access file with http://127.0.0.1/aaa/bbb/c:/windows/win.ini
unfortunately, i got the bad news, it didn't work as expected and the browser return win.ini text contents.
and here were debug outputs:

204 debug: "aaa"
204 debug: "bbb"
204 debug: "c:"
204 debug: "windows"
204 debug: "win.ini"

it means "c:" treats as Component::Normal not Component::Prefix in Windows 10 64bit OS.

[davidpdrsn]

Hm that's weird. Then I don't understand the docs in std 🤔

I guess we need to guard on the component ending in : then. I can do that later this weekend or you're free to add it.

[adumbidiot]

Looking at the path doc's source code, it seems the prefix component is only returned if it is the first element, which is why C: is returned as a Normal. So we still need to validate that comp is not a prefix. I don't think it's possible to squeeze an absolute path there since in order for a path to be absolute, it needs to have a \, which should already be gone at this point. So, I think we just need to ensure that comp is not a prefix. I changed that section to instead read the following:

let comp = Path::new(comp);
if !comp
    .components()
    .all(|c| matches!(c, std::path::Component::Normal(_))) 
{
    return None;
}
full_path.push(comp);

I can't seem to reproduce the issue after that change.

Also, why is full_path required to be relative now? Doesn't that force the provided directory to always be relative for the entire service?

[davidpdrsn]

@adumbidiot That makes sense! I've pushed a change that also checks each component individually.

[adumbidiot]

That commit seems to fix the issue

[davidpdrsn]

@c5soft @adumbidiot Thanks for your help in reporting and fixing this! The patch is shipped in version 0.2.1, I've filed a RUSTSEC about it (rustsec/advisory-db#1159), and yanked the affected versions from crates.io.

[Roguelazer]

It looks like the RUSTSEC advisory accidentally got marked as 2021 instead of 2022. :-(