Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

c5soft
Copy link
Contributor

@c5soft c5soft commented Jan 14, 2022

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
Copy link
Member

Thanks for finding this!

@davidpdrsn
Copy link
Member

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
Copy link
Contributor Author

c5soft commented Jan 15, 2022

I have tested the warp, unfortunately,warp 0.3.2 has exactly the same bug. I don't care about othor crates.
204bug
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
Copy link

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
Copy link

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
Copy link

@adumbidiot
Copy link

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
Copy link
Member

@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
Copy link
Contributor Author

c5soft commented Jan 15, 2022

@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
Copy link
Member

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
Copy link

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
Copy link
Member

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

@adumbidiot
Copy link

That commit seems to fix the issue

@davidpdrsn davidpdrsn merged commit ec394e7 into tower-rs:master Jan 21, 2022
@davidpdrsn davidpdrsn mentioned this pull request Jan 21, 2022
@davidpdrsn
Copy link
Member

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants