Server allows path traversal #3
Comments
|
See this example code from Axum for one method of preventing path traversal attacks. |
|
Dang I didn't think of that. I'll work on a fix now. |
|
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. |
|
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. |
|
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. |
|
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 This is a bit more heavyweight, but it's code I've written elsewhere adapted to // 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. |
|
By the way, I had to make a few change to your code. At line 14, you tried to call the |
|
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 I'm glad you were able to make use of it. You can actually optimize further by eliminating the 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 |
|
If I'm right about the 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. |
|
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
The text was updated successfully, but these errors were encountered: