Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tower-http/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ tracing = { version = "0.1", default-features = false, optional = true }
httpdate = { version = "1.0", optional = true }
uuid = { version = "1.0", features = ["v4"], optional = true }

# linux only dependencies
[target.'cfg(target_os = "linux")'.dependencies]
rustix = { version = "1", features = ["fs"] }

[dev-dependencies]
brotli = "8"
bytes = "1"
Expand All @@ -55,6 +59,7 @@ tokio = { version = "1", features = ["full"] }
tower = { version = "0.5", features = ["buffer", "util", "retry", "make", "timeout"] }
tracing-subscriber = "0.3"
zstd = "0.13"
tempfile = "3"

[features]
default = []
Expand Down
18 changes: 18 additions & 0 deletions tower-http/src/services/fs/serve_dir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ const DEFAULT_CAPACITY: usize = 65536;
/// existing file (`/file.html/something`)
/// - We don't have necessary permissions to read the file
///
/// If the requested file is a symlink, it will be followed, even outside `base`.
/// On Linux this behaviour can be modified to only allow symlinks below `base`
/// with `follow_symlinks_outside_base`.
///
/// # Example
///
/// ```
Expand All @@ -59,6 +63,7 @@ pub struct ServeDir<F = DefaultServeDirFallback> {
variant: ServeVariant,
fallback: Option<F>,
call_fallback_on_method_not_allowed: bool,
follow_symlinks_outside_base: bool,
}

impl ServeDir<DefaultServeDirFallback> {
Expand All @@ -79,6 +84,7 @@ impl ServeDir<DefaultServeDirFallback> {
},
fallback: None,
call_fallback_on_method_not_allowed: false,
follow_symlinks_outside_base: true,
}
}

Expand All @@ -93,6 +99,7 @@ impl ServeDir<DefaultServeDirFallback> {
variant: ServeVariant::SingleFile { mime },
fallback: None,
call_fallback_on_method_not_allowed: false,
follow_symlinks_outside_base: true,
}
}
}
Expand Down Expand Up @@ -217,6 +224,7 @@ impl<F> ServeDir<F> {
variant: self.variant,
fallback: Some(new_fallback),
call_fallback_on_method_not_allowed: self.call_fallback_on_method_not_allowed,
follow_symlinks_outside_base: self.follow_symlinks_outside_base,
}
}

Expand Down Expand Up @@ -249,6 +257,14 @@ impl<F> ServeDir<F> {
self
}

/// Disallow following symlinks outside the base directory on Linux.
/// 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self
}

/// Call the service and get a future that contains any `std::io::Error` that might have
/// happened.
///
Expand Down Expand Up @@ -381,6 +397,8 @@ impl<F> ServeDir<F> {
negotiated_encodings,
range_header,
buf_chunk_size,
self.base.clone(),
self.follow_symlinks_outside_base,
));

ResponseFuture::open_file_future(open_file_future, fallback_and_request)
Expand Down
85 changes: 76 additions & 9 deletions tower-http/src/services/fs/serve_dir/open_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,16 @@ pub(super) enum FileRequestExtent {
Head(Metadata),
}

#[allow(clippy::too_many_arguments)]
pub(super) async fn open_file(
variant: ServeVariant,
mut path_to_file: PathBuf,
req: Request<Empty<Bytes>>,
negotiated_encodings: Vec<(Encoding, QValue)>,
range_header: Option<String>,
buf_chunk_size: usize,
base_path: PathBuf,
follow_symlinks_outside_base: bool,
) -> io::Result<OpenFileOutput> {
let if_unmodified_since = req
.headers()
Expand Down Expand Up @@ -110,15 +113,21 @@ pub(super) async fn open_file(
last_modified,
})))
} else {
let (mut file, maybe_encoding) =
match open_file_with_fallback(path_to_file, negotiated_encodings).await {
Ok(result) => result,
let (mut file, maybe_encoding) = match open_file_with_fallback(
path_to_file,
negotiated_encodings,
&base_path,
follow_symlinks_outside_base,
)
.await
{
Ok(result) => result,

Err(err) if is_invalid_filename_error(&err) => {
return Ok(OpenFileOutput::InvalidFilename)
}
Err(err) => return Err(err),
};
Err(err) if is_invalid_filename_error(&err) => {
return Ok(OpenFileOutput::InvalidFilename)
}
Err(err) => return Err(err),
};

let meta = file.metadata().await?;
let last_modified = meta.modified().ok().map(LastModified::from);
Expand Down Expand Up @@ -226,17 +235,75 @@ fn preferred_encoding(
preferred_encoding
}

#[cfg(target_os = "linux")]
fn canonicalize_and_openat2(base_path: &Path, path: &Path) -> io::Result<File> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let (path, base_path2) = if base_path.is_file() || !base_path.exists() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already know whether we are ServeVariant::SingleFile vs ServeVariant::Directory, can we just thread that down to skip the fs probe?

let base_path = base_path.parent().unwrap().canonicalize()?;
let path = path
.canonicalize()?
.strip_prefix(&base_path)
.map_err(|err| io::Error::new(io::ErrorKind::Other, err))?
.to_owned();
(
path,
rustix::fs::open(
base_path,
rustix::fs::OFlags::RDONLY,
rustix::fs::Mode::empty(),
)?,
)
} else {
let base_path = base_path.canonicalize()?;
let path = path
.canonicalize()?
.strip_prefix(&base_path)
.map_err(|err| io::Error::new(io::ErrorKind::Other, err))?
.to_owned();
(
path,
rustix::fs::open(
base_path,
rustix::fs::OFlags::RDONLY,
rustix::fs::Mode::empty(),
)?,
)
};

rustix::fs::openat2(
base_path2,
&path,
rustix::fs::OFlags::RDONLY,
rustix::fs::Mode::empty(),
rustix::fs::ResolveFlags::BENEATH,
)
.map(std::fs::File::from)
.map(File::from)
.map_err(|err| io::Error::new(io::ErrorKind::Other, err))
}

// Attempts to open the file with any of the possible negotiated_encodings in the
// preferred order. If none of the negotiated_encodings have a corresponding precompressed
// file the uncompressed file is used as a fallback.
async fn open_file_with_fallback(
mut path: PathBuf,
mut negotiated_encoding: Vec<(Encoding, QValue)>,
base_path: &Path,
follow_symlinks_outside_base: bool,
) -> io::Result<(File, Option<Encoding>)> {
let (file, encoding) = loop {
// Get the preferred encoding among the negotiated ones.
let encoding = preferred_encoding(&mut path, &negotiated_encoding);
match (File::open(&path).await, encoding) {
let file = {
#[cfg(target_os = "linux")]
if follow_symlinks_outside_base {
File::open(&path).await
} else {
canonicalize_and_openat2(base_path, &path)
}
#[cfg(not(target_os = "linux"))]
File::open(&path).await
};
match (file, encoding) {
(Ok(file), maybe_encoding) => break (file, maybe_encoding),
(Err(err), Some(encoding)) if err.kind() == io::ErrorKind::NotFound => {
// Remove the extension corresponding to a precompressed file (.gz, .br, .zz)
Expand Down
74 changes: 73 additions & 1 deletion tower-http/src/services/fs/serve_dir/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use http_body::Body as HttpBody;
use http_body_util::BodyExt;
use std::convert::Infallible;
use std::fs;
use std::io::Read;
use std::fs::{create_dir, File};
use std::io::{Read, Write};
use std::os::unix::fs::symlink;
use tower::{service_fn, ServiceExt};

#[tokio::test]
Expand Down Expand Up @@ -627,6 +629,23 @@ async fn read_partial_errs_on_bad_range() {
)
}

#[tokio::test]
async fn read_partial_errs_on_bad_range_follow_symlinks() {
let svc = ServeDir::new("..").follow_symlinks_outside_base(true);
let req = Request::builder()
.uri("/README.md")
.header("Range", "bytes=-1-15")
.body(Body::empty())
.unwrap();
let res = svc.oneshot(req).await.unwrap();
assert_eq!(res.status(), StatusCode::RANGE_NOT_SATISFIABLE);
let file_contents = std::fs::read("../README.md").unwrap();
assert_eq!(
res.headers()["content-range"],
&format!("bytes */{}", file_contents.len())
)
}

#[tokio::test]
async fn accept_encoding_identity() {
let svc = ServeDir::new("..");
Expand Down Expand Up @@ -877,3 +896,56 @@ async fn calls_fallback_on_null() {

assert_eq!(res.headers()["from-fallback"], "1");
}

#[tokio::test]
#[cfg(target_os = "linux")]
async fn returns_500_if_symlink_outside_base() {
let tmp = tempfile::tempdir().unwrap();
let base = tmp.path().join("base");
create_dir(&base).unwrap();
let file_outside = tmp.path().join("file_outside.txt");
let mut outside = File::create(&file_outside).unwrap();
write!(outside, "{}", "outside").unwrap();
let file_inside = base.join("inside.txt");
symlink(file_outside, file_inside).unwrap();

let svc = ServeDir::new(base).follow_symlinks_outside_base(false);

let request = Request::builder()
.header("Accept-Encoding", "deflate")
.method(Method::GET)
.uri("/inside.txt")
.body(Body::empty())
.unwrap();
let res = svc.oneshot(request).await.unwrap();

assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR);
assert!(res.headers().get(header::CONTENT_TYPE).is_none());
}

#[tokio::test]
async fn allow_symlink_outside_base() {
let tmp = tempfile::tempdir().unwrap();
let base = tmp.path().join("base");
create_dir(&base).unwrap();
let file_outside = tmp.path().join("file_outside.txt");
let mut outside = File::create(&file_outside).unwrap();
write!(outside, "{}", "outside").unwrap();
let file_inside = base.join("inside.txt");
symlink(file_outside, file_inside).unwrap();

let svc = ServeDir::new(base);

let request = Request::builder()
.header("Accept-Encoding", "deflate")
.method(Method::GET)
.uri("/inside.txt")
.body(Body::empty())
.unwrap();
let res = svc.oneshot(request).await.unwrap();

assert_eq!(res.status(), StatusCode::OK);
assert!(res.headers().get(header::CONTENT_TYPE).is_some());
let body = res.into_body().collect().await.unwrap().to_bytes();
assert_eq!("outside", String::from_utf8_lossy(&body));
}
38 changes: 38 additions & 0 deletions tower-http/src/services/fs/serve_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,16 @@ impl ServeFile {
Self(self.0.with_buf_chunk_size(chunk_size))
}

/// Disallow following symlinks outside the base directory on Linux.
/// 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(self, follow_symlinks_outside_base: bool) -> Self {
Self(
self.0
.follow_symlinks_outside_base(follow_symlinks_outside_base),
)
}

/// Call the service and get a future that contains any `std::io::Error` that might have
/// happened.
///
Expand Down Expand Up @@ -152,7 +162,10 @@ mod tests {
use http::{Request, StatusCode};
use http_body_util::BodyExt;
use mime::Mime;
use std::fs::{create_dir, File};
use std::io::Read;
use std::io::Write;
use std::os::unix::fs::symlink;
use std::str::FromStr;
use tokio::io::AsyncReadExt;
use tower::ServiceExt;
Expand Down Expand Up @@ -496,6 +509,31 @@ mod tests {
assert!(res.headers().get(header::CONTENT_TYPE).is_none());
}

#[tokio::test]
async fn returns_500_if_symlink_outside_base() {
let tmp = tempfile::tempdir().unwrap();
let base = tmp.path().join("base");
create_dir(&base).unwrap();
let file_outside = tmp.path().join("file_outside.txt");
let mut outside = File::create(&file_outside).unwrap();
write!(outside, "{}", "outside").unwrap();
let file_inside = base.join("inside.txt");
symlink(file_outside, file_inside).unwrap();

let svc = ServeFile::new(base).follow_symlinks_outside_base(false);

let request = Request::builder()
.header("Accept-Encoding", "deflate")
.method(Method::GET)
.uri("/inside.txt")
.body(Body::empty())
.unwrap();
let res = svc.oneshot(request).await.unwrap();

assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR);
assert!(res.headers().get(header::CONTENT_TYPE).is_none());
}

#[tokio::test]
async fn last_modified() {
let svc = ServeFile::new("../README.md");
Expand Down
Loading