alphabitserial
commented
2 years ago See this example code from Axum for one method of preventing path traversal attacks.
Closed tibordp closed 2 years ago
alphabitserial
commented
2 years ago See this example code from Axum for one method of preventing path traversal attacks.
PeterPierinakos
commented
2 years ago Dang I didn't think of that. I'll work on a fix now.
PeterPierinakos
commented
2 years ago EDIT: Deprecated solution, look at the comment below.
@alphabitserial it seems like I have found a simpler fix although it seems a little junky.
tl;dr: potential fix
Why does this work?
HTML files end with a dot at the end. As a result, if the buffer's requested URI contains more than 1 dot, that means they are trying to use an incorrect path. BTW, I tried exploiting it by using the absolute path and it seems not to work.
If you guys see any problem or do not like this solution, I will try to implement @alphabitserial's solution. Otherwise, I will merge with main.
PeterPierinakos
commented
2 years ago Due to some file formats using more than one dot, I have come to the conclusion that @alphabitserial solution is the optimal one.
I have put both of you in the credits. If you see any problem with this final fix, please report it so that I can fix it before merging with the main branch.
PeterPierinakos
commented
2 years ago Seems like there are no flaws with this solution. The fix will be merged to the main branch soon. Thanks once again for reporting this vulnerability.
mcronce
commented
2 years ago I just found this from a Reddit link, and obviously might be misreading, but doesn't this solution still allow (e.g.) a path like a/../../b to traverse above the configured root?
This is a bit more heavyweight, but it's code I've written elsewhere adapted to std::path and should be fully robust, if suboptimal:
// Could return an Option<Cow<'_, Path>> if you want to avoid an allocation in cases where the path is already normalized
// Could also just return a bool if you only want this to be a check for validity, which seems good for your use case
fn normalize_path(path: &Path) -> Option<PathBuf> {
let mut result = PathBuf::new();
let mut components = path.components();
let first_component = components.next();
match first_component {
Some(Component::Prefix(p)) => result.push(Component::Prefix(p)),
Some(Component::RootDir) => result.push(Component::RootDir), // If you've already stripped the leading / from the requested path, this should also return None
_ => return None
};
for component in components {
match component {
Component::Prefix(_) => return None, // Should be unreachable
Component::RootDir => return None, // Should be unreachable
Component::CurDir => if result.as_str().is_empty() {
// If you've already stripped the leading / from the requested path, this should no-op
result.push(Component::RootDir);
},
Component::ParentDir => if !result.pop() {
return None;
},
Component::Normal(p) => result.push(p)
{;
}
Some(result)
}@mcronce you are right. It is still exploitable. I'll test your solution which seems to be improved and push it to main.
PeterPierinakos
commented
2 years ago By the way, I had to make a few change to your code. At line 14, you tried to call the .as_str() function for a PathBuf type. There is no such function for PathBuf, I am assuming you meant to call .as_os_str() before checking whether it's empty :)
PeterPierinakos
commented
2 years ago Here is the final solution by the way (optimized it a little bit, still works)
pub fn path_is_valid(path: &Path) -> bool {
let mut result = PathBuf::new();
let components = path.components();
for component in components {
match component {
Component::Prefix(_) => return false, // Should be unreachable
Component::RootDir => return false, // Should be unreachable
Component::CurDir => if result.as_os_str().is_empty() {
// If you've already stripped the leading / from the requested path, this should no-op
result.push(Component::RootDir);
},
Component::ParentDir => if !result.pop() {
return false;
},
Component::Normal(p) => result.push(p)
};
}
true
}Whoops, good call - yeah should have been .as_os_str(). I adapted this from code that uses camino and did not try to compile before posting it ;)
I'm glad you were able to make use of it. You can actually optimize further by eliminating the result PathBuf entirely in favor of just incrementing and decrementing an integer since you're only looking to get a bool out of it :)
Something like this:
pub fn path_is_valid(path: &Path) -> bool {
let mut stack = 0;
let components = path.components();
for component in components {
match component {
Component::Prefix(_) => return false, // Should be unreachable
Component::RootDir => return false, // Should be unreachable
Component::CurDir => if stack == 0 {
// If you've already stripped the leading / from the requested path, this should no-op
stack += 1;
},
Component::ParentDir if stack == 0 => return false,
Component::ParentDir => stack -= 1,
Component::Normal(p) => stack += 1
};
}
true
}I translated it directly from your code, but that Component::CurDir arm does smell a little sus; based on what you posted it looks like you've already made the path non-absolute, which suggests that should just no-op, but I haven't read the code so don't just take my word for it ;) this is complex enough logic that if it were me I'd probably throw together a few tests to validate.
mcronce
commented
2 years ago If I'm right about the CurDir arm (which is a big if), this would probably be correct:
pub fn path_is_valid(path: &Path) -> bool {
let mut stack = 0;
let components = path.components();
for component in components {
stack += match component {
Component::Prefix(_) => return false, // Should be unreachable
Component::RootDir => return false, // Should be unreachable
Component::CurDir => 0,
Component::ParentDir if stack == 0 => return false,
Component::ParentDir => -1,
Component::Normal(p) => 1
};
}
true
}I will probably create a future-improvements branch with minor changes like this to be merged with the main branch for the next release.
PeterPierinakos
commented
2 years ago Seems like there are no more complaints about path traversal exploits anymore, locking the conversation. Feel free to submit a new issue if a new path traversal vulnerability is found
Describe the bug vrs serves files that are outside
/var/www/staticby using..in path. This allows potentially sensistive files to be read from the server, for example/etc/passwdTo Reproduce
Expected behavior 404 or 400 error