In ServeDir, change the behaviour for handling symlinks#658
In ServeDir, change the behaviour for handling symlinks#658alexanderkjall wants to merge 2 commits into
Conversation
…tside the base_dir to return errors, and introduce a new setting that will allow it (on linux)
| }, | ||
| fallback: None, | ||
| call_fallback_on_method_not_allowed: false, | ||
| follow_symlinks_outside_base: false, |
There was a problem hiding this comment.
This should default to true. And relevant docs should mention the difference only if enabled.
There was a problem hiding this comment.
thanks for the quick review, I guess I was a bit too paranoid. I have flipped this and tried to improve the docs.
There was a problem hiding this comment.
Hi @alexanderkjall , thanks for those changes, looks good overall and strongly agree with the intent here. I'd be interested in discussing flipping this to a default behavior in a future breaking release, but opt-in is good for now.
I did have some concerns though, see comments. (Also needs some merge conflict resolution against main)
As a side note, I'm wondering, is there a good place to more clearly documenting the "no escape base" semantics for ServeFile? For ServeDir, its fairly intuitive ("this directory is the boundary"). But for ServeFile, it mostly implies, the selected file can't symlink outside its parent, yes?
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| fn canonicalize_and_openat2(base_path: &Path, path: &Path) -> io::Result<File> { |
There was a problem hiding this comment.
Currently ServeDir uses tokio::fs::File::open to avoid blocking the tokio runtime.
canonicalize_and_openat2 instead uses sync APIs which would block the runtime under load.
Can we shift to using the tokio::spawn_blocking API or something else that will avoid problems with blocking?
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| fn canonicalize_and_openat2(base_path: &Path, path: &Path) -> io::Result<File> { |
There was a problem hiding this comment.
This will return an EXDEV on failure, which then passes through as a 500 error inside our error handling in future.rs. We should instead map it to a 404 to avoid leaking information about our system context.
| /// Setting this to false on Linux introduces a dependency on at least | ||
| /// kernel version 5.6, as the openat2 syscall was introduced in that version. | ||
| pub fn follow_symlinks_outside_base(mut self, follow_symlinks_outside_base: bool) -> Self { | ||
| self.follow_symlinks_outside_base = follow_symlinks_outside_base; |
There was a problem hiding this comment.
Is there a reason not to gate this method behind conditional configuration targeting linux? I'm fine to punt on support for other platforms, but I'd rather not have this be a silent non-op.
|
|
||
| if req.method() == Method::HEAD { | ||
| let (meta, maybe_encoding) = | ||
| file_metadata_with_fallback(path_to_file, negotiated_encodings).await?; |
There was a problem hiding this comment.
Can we also keep HEAD requests from escaping base with symlinks?
|
|
||
| #[cfg(target_os = "linux")] | ||
| fn canonicalize_and_openat2(base_path: &Path, path: &Path) -> io::Result<File> { | ||
| let (path, base_path2) = if base_path.is_file() || !base_path.exists() { |
There was a problem hiding this comment.
We already know whether we are ServeVariant::SingleFile vs ServeVariant::Directory, can we just thread that down to skip the fs probe?
3 participants
that point outside the base_dir to return errors, and introduce a new setting that will allow it (on linux)
Motivation
Let me preface this with that I consider this a potential security problem, but I emailed security@tokio.rs and they didn't agree and encouraged me to open a public issue.
Now on to the issue:
ServeDir in tower-http doesn't have any configuration to control symlink behaviour. ( https://docs.rs/tower-http/latest/tower_http/services/struct.ServeDir.html ).
This means that it always follows symlinks.
Attack scenario:
A local user have write permissions to the directory that files are served from, and the webserver process have access to secrets that the user shouldn't be able to read, for example the https certificate with the private key.
Then the user can create a symlink in the directory that is served and exfiltrate those secrets.
Solution
This solution is not really ready to be merged, as it's implemented as a linux only solution for now, but maybe this can act as a starting point for a discussion on how it should be solved?
I have implemented this using the openat2 syscall, as that is as far as I know the only/best way to avoid a TOCTOU (https://cwe.mitre.org/data/definitions/367.html) race condition. This makes the solution linux only, and would need to be expanded to other platforms, but I don't own to any computers running macos/windows/freebsd.
The new open function I introduced
canonicalize_and_openat2is also not async, maybe a good way forward would be for tokio::fs::File to gain an asyncopen_at()function?I'm also a bit unsure on what to do if
ServeFilepoints to a symlink, happy to change that behavior.