diff options
author | Jikstra <34889164+Jikstra@users.noreply.github.com> | 2021-09-10 13:56:31 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-09-10 13:56:31 +0000 |
commit | d0cd4f0f21210cae8677941f6ca4059365767879 (patch) | |
tree | 623b58e978ef1c31c11553a5b077f51432a34f84 | |
parent | Apply requested changes (diff) | |
parent | Merge pull request #598 from svenstaro/dependabot/cargo/sha2-0.9.8 (diff) | |
download | miniserve-d0cd4f0f21210cae8677941f6ca4059365767879.tar.gz miniserve-d0cd4f0f21210cae8677941f6ca4059365767879.zip |
Merge branch 'master' into feat_raw_mode
-rw-r--r-- | CHANGELOG.md | 8 | ||||
-rw-r--r-- | Cargo.lock | 59 | ||||
-rw-r--r-- | Cargo.toml | 3 | ||||
-rw-r--r-- | Cross.toml | 4 | ||||
-rw-r--r-- | README.md | 2 | ||||
-rw-r--r-- | data/style.scss | 2 | ||||
-rw-r--r-- | src/args.rs | 6 | ||||
-rw-r--r-- | src/file_upload.rs | 93 | ||||
-rw-r--r-- | src/listing.rs | 20 | ||||
-rw-r--r-- | src/main.rs | 15 | ||||
-rw-r--r-- | tests/upload_files.rs | 91 |
11 files changed, 221 insertions, 82 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 85bf11e..7e53fe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). <!-- next-header --> ## [Unreleased] - ReleaseDate + +## [0.17.0] - 2021-09-04 - Print QR codes on terminal [#524](https://github.com/svenstaro/miniserve/pull/524) (thanks @aliemjay) +- Fix mobile layout info pills taking whole width [#591](https://github.com/svenstaro/miniserve/issues/591) +- Fix security exploit when uploading is enabled [#590](https://github.com/svenstaro/miniserve/pull/590) [#518](https://github.com/svenstaro/miniserve/issues/518) (thanks @aliemjay) +- Fix uploading to symlink directories [#590](https://github.com/svenstaro/miniserve/pull/590) [#466](https://github.com/svenstaro/miniserve/issues/466) (thanks @aliemjay) ## [0.16.0] - 2021-08-31 - Fix serving files with backslashes in their names [#578](https://github.com/svenstaro/miniserve/pull/578) (thanks @Jikstra) @@ -112,7 +117,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Some theme related bug fixes (thanks @boastful-squirrel) <!-- next-url --> -[Unreleased]: https://github.com/svenstaro/miniserve/compare/v0.16.0...HEAD +[Unreleased]: https://github.com/svenstaro/miniserve/compare/v0.17.0...HEAD +[0.17.0]: https://github.com/svenstaro/miniserve/compare/v0.16.0...v0.17.0 [0.16.0]: https://github.com/svenstaro/miniserve/compare/v0.15.0...v0.16.0 [0.15.0]: https://github.com/svenstaro/miniserve/compare/v0.14.0...v0.15.0 [0.14.0]: https://github.com/svenstaro/miniserve/compare/v0.13.0...v0.14.0 @@ -329,9 +329,9 @@ checksum = "90c108c1a94380c89d2215d0ac54ce09796823cca0fd91b299cfff3b33e346fb" [[package]] name = "assert_cmd" -version = "2.0.0" +version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "54f002ce7d0c5e809ebb02be78fd503aeed4a511fd0fcaff6e6914cbdabbfa33" +checksum = "b800c4403e8105d959595e1f88119e78bc12bc874c4336973658b648a746ba93" dependencies = [ "bstr", "doc-comment", @@ -343,9 +343,9 @@ dependencies = [ [[package]] name = "assert_fs" -version = "1.0.4" +version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "58436a22e4ac01846f8e72e7dcd3f6aa96a4c9f8fa5e77f9234d287dfac0c354" +checksum = "7d7b349de9bde9383966c8d3be1103623620ca34d2c43a41b82360e552661007" dependencies = [ "doc-comment", "globwalk", @@ -513,9 +513,9 @@ checksum = "4964518bd3b4a8190e832886cdc0da9794f12e8e6c1613a9e90ff331c4c8724b" [[package]] name = "cc" -version = "1.0.69" +version = "1.0.70" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e70cc2f62c6ce1868963827bd677764c62d07c3d9a3e1fb1177ee1a9ab199eb2" +checksum = "d26a6ce4b6a484fa3edb70f7efa6fc430fd2b87285fe8b84304fd0936faa0dc0" dependencies = [ "jobserver", ] @@ -663,9 +663,9 @@ dependencies = [ [[package]] name = "ctor" -version = "0.1.20" +version = "0.1.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5e98e2ad1a782e33928b96fc3948e7c355e5af34ba4de7670fe8bac2a3b2006d" +checksum = "ccc0a48a9b826acdf4028595adc9db92caea352f7af011a3034acd172a52a0aa" dependencies = [ "quote", "syn", @@ -1393,7 +1393,7 @@ dependencies = [ [[package]] name = "miniserve" -version = "0.16.1-alpha.0" +version = "0.17.1-alpha.0" dependencies = [ "actix-files", "actix-multipart", @@ -1404,7 +1404,6 @@ dependencies = [ "assert_cmd", "assert_fs", "atty", - "bytes", "bytesize", "chrono", "chrono-humanize", @@ -1508,9 +1507,9 @@ dependencies = [ [[package]] name = "num-bigint" -version = "0.4.1" +version = "0.4.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "76e97c412795abf6c24ba30055a8f20642ea57ca12875220b854cfa501bf1e48" +checksum = "74e768dff5fb39a41b3bcd30bb25cf989706c90d028d1ad71971987aa309d535" dependencies = [ "autocfg", "num-integer", @@ -1849,9 +1848,9 @@ checksum = "bc881b2c22681370c6a780e47af9840ef841837bc98118431d4e1868bd0c1086" [[package]] name = "proc-macro2" -version = "1.0.28" +version = "1.0.29" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c7ed8b8c7b886ea3ed7dde405212185f423ab44682667c8c6dd14aa1d9f6612" +checksum = "b9f5105d4fdaab20335ca9565e106a5d9b82b6219b5ba735731124ac6711d23d" dependencies = [ "unicode-xid", ] @@ -2258,9 +2257,9 @@ checksum = "2579985fda508104f7587689507983eadd6a6e84dd35d6d115361f530916fa0d" [[package]] name = "sha2" -version = "0.9.6" +version = "0.9.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9204c41a1597a8c5af23c82d1c921cb01ec0a4c59e07a9c7306062829a3903f3" +checksum = "b69f9a4c9740d74c5baa3fd2e547f9525fa8088a8a958e0ca2409a514e33f5fa" dependencies = [ "block-buffer", "cfg-if", @@ -2291,9 +2290,9 @@ dependencies = [ [[package]] name = "siphasher" -version = "0.3.6" +version = "0.3.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "729a25c17d72b06c68cb47955d44fda88ad2d3e7d77e025663fdd69b93dd71a1" +checksum = "533494a8f9b724d33625ab53c6c4800f7cc445895924a8ef649222dcb76e938b" [[package]] name = "slab" @@ -2438,9 +2437,9 @@ dependencies = [ [[package]] name = "syn" -version = "1.0.75" +version = "1.0.76" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b7f58f7e8eaa0009c5fec437aabf511bd9933e4b2d7407bd05273c01a8906ea7" +checksum = "c6f107db402c2c2055242dbf4d2af0e69197202e9faacbef9571bbe47f5a1b84" dependencies = [ "proc-macro2", "quote", @@ -2523,18 +2522,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.28" +version = "1.0.29" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "283d5230e63df9608ac7d9691adc1dfb6e701225436eb64d0b9a7f0a5a04f6ec" +checksum = "602eca064b2d83369e2b2f34b09c70b605402801927c65c11071ac911d299b88" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.28" +version = "1.0.29" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fa3884228611f5cd3608e2d409bf7dce832e4eb3135e3f11addbd7e41bd68e71" +checksum = "bad553cc2c78e8de258400763a647e80e6d1b31ee237275d756f6836d204494c" dependencies = [ "proc-macro2", "quote", @@ -2615,9 +2614,9 @@ checksum = "cda74da7e1a664f795bb1f8a87ec406fb89a02522cf6e50620d016add6dbbf5c" [[package]] name = "tokio" -version = "1.10.1" +version = "1.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "92036be488bb6594459f2e03b60e42df6f937fe6ca5c5ffdcb539c6b84dc40f5" +checksum = "b4efe6fc2395938c8155973d7be49fe8d03a843726e285e100a8a383cc0154ce" dependencies = [ "autocfg", "bytes", @@ -2645,9 +2644,9 @@ dependencies = [ [[package]] name = "tokio-util" -version = "0.6.7" +version = "0.6.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592" +checksum = "08d3725d3efa29485e87311c5b699de63cde14b00ed4d256b8318aa30ca452cd" dependencies = [ "bytes", "futures-core", @@ -2707,9 +2706,9 @@ dependencies = [ [[package]] name = "typenum" -version = "1.13.0" +version = "1.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "879f6906492a7cd215bfa4cf595b600146ccfac0c79bcbd1f3000162af5e8b06" +checksum = "b63708a265f51345575b27fe43f9500ad611579e764c79edbc2037b1121959ec" [[package]] name = "ucd-trie" @@ -1,6 +1,6 @@ [package] name = "miniserve" -version = "0.16.1-alpha.0" +version = "0.17.1-alpha.0" description = "For when you really just want to serve some files over HTTP right now!" authors = ["Sven-Hendrik Haase <svenstaro@gmail.com>", "Boastful Squirrel <boastful.squirrel@gmail.com>"] repository = "https://github.com/svenstaro/miniserve" @@ -50,7 +50,6 @@ qrcodegen = "1" mime = "0.3" httparse = "1" http = "0.2" -bytes = "1" atty = "0.2" rustls = { version = "0.19", optional = true } socket2 = "0.4" @@ -1,5 +1,3 @@ # NOTE: Custom image specification for freebsd is required until new version of cross is released. -# Also we'll have to use a custom image until https://github.com/rust-embedded/cross/pull/582 is -# merged and released on Docker Hub. [target.x86_64-unknown-freebsd] -image = "svenstaro/cross-x86_64-unknown-freebsd" +image = "rustembedded/cross:x86_64-unknown-freebsd" @@ -79,7 +79,7 @@ Sometimes this is just a more practical and quick way than doing things properly ## Usage - miniserve 0.16.0 + miniserve 0.17.0 Sven-Hendrik Haase <svenstaro@gmail.com>, Boastful Squirrel <boastful.squirrel@gmail.com> diff --git a/data/style.scss b/data/style.scss index 8bdd5d7..9115c0e 100644 --- a/data/style.scss +++ b/data/style.scss @@ -483,6 +483,8 @@ th span.active span { .mobile-info { display: block; + float: right; + margin-top: 0.5rem; } table tbody tr td { diff --git a/src/args.rs b/src/args.rs index 5467573..ff8d92b 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,4 +1,3 @@ -use bytes::Bytes; use clap::{Clap, ValueHint}; use clap_generate::Shell; use http::header::{HeaderMap, HeaderName, HeaderValue}; @@ -212,15 +211,14 @@ fn parse_auth(src: &str) -> Result<auth::RequiredAuth, ContextualError> { /// Custom header parser (allow multiple headers input) pub fn parse_header(src: &str) -> Result<HeaderMap, httparse::Error> { let mut headers = [httparse::EMPTY_HEADER; 1]; - let mut header = src.to_string(); - header.push('\n'); + let header = format!("{}\n", src); httparse::parse_headers(header.as_bytes(), &mut headers)?; let mut header_map = HeaderMap::new(); if let Some(h) = headers.first() { if h.name != httparse::EMPTY_HEADER.name { header_map.insert( - HeaderName::from_bytes(&Bytes::copy_from_slice(h.name.as_bytes())).unwrap(), + HeaderName::from_bytes(h.name.as_bytes()).unwrap(), HeaderValue::from_bytes(h.value).unwrap(), ); } diff --git a/src/file_upload.rs b/src/file_upload.rs index 5f9738c..5009f36 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -2,13 +2,16 @@ use actix_web::{http::header, HttpRequest, HttpResponse}; use futures::TryStreamExt; use std::{ io::Write, - path::{Component, PathBuf}, + path::{Component, Path, PathBuf}, }; use crate::errors::ContextualError; use crate::listing::{self}; -/// Create future to save file. +/// Saves file data from a multipart form field (`field`) to `file_path`, optionally overwriting +/// existing file. +/// +/// Returns total bytes written to file. async fn save_file( field: actix_multipart::Field, file_path: PathBuf, @@ -34,10 +37,10 @@ async fn save_file( Ok(written_len) } -/// Create new future to handle file as multipart data. +/// Handles a single field in a multipart form async fn handle_multipart( field: actix_multipart::Field, - file_path: PathBuf, + path: PathBuf, overwrite_files: bool, ) -> Result<u64, ContextualError> { let filename = field @@ -50,21 +53,25 @@ async fn handle_multipart( ) })?; - match std::fs::metadata(&file_path) { + let filename = sanitize_path(Path::new(&filename), false).ok_or_else(|| { + ContextualError::InvalidPathError("Invalid file name to upload".to_string()) + })?; + + match std::fs::metadata(&path) { Err(_) => Err(ContextualError::InsufficientPermissionsError( - file_path.display().to_string(), + path.display().to_string(), )), Ok(metadata) if !metadata.is_dir() => Err(ContextualError::InvalidPathError(format!( "cannot upload file to {}, since it's not a directory", - &file_path.display() + &path.display() ))), Ok(metadata) if metadata.permissions().readonly() => Err( - ContextualError::InsufficientPermissionsError(file_path.display().to_string()), + ContextualError::InsufficientPermissionsError(path.display().to_string()), ), Ok(_) => Ok(()), }?; - save_file(field, file_path.join(filename), overwrite_files).await + save_file(field, path.join(filename), overwrite_files).await } /// Handle incoming request to upload file. @@ -87,9 +94,9 @@ pub async fn upload_file( let upload_path = query_params.path.as_ref().ok_or_else(|| { ContextualError::InvalidHttpRequestError("Missing query parameter 'path'".to_string()) })?; - let upload_path = upload_path - .strip_prefix(Component::RootDir) - .unwrap_or(upload_path); + let upload_path = sanitize_path(upload_path, conf.show_hidden).ok_or_else(|| { + ContextualError::InvalidPathError("Invalid value for 'path' parameter".to_string()) + })?; let app_root_dir = conf.path.canonicalize().map_err(|e| { ContextualError::IoError("Failed to resolve path served by miniserve".to_string(), e) @@ -97,6 +104,7 @@ pub async fn upload_file( // If the target path is under the app root directory, save the file. let target_dir = match app_root_dir.join(upload_path).canonicalize() { + Ok(path) if !conf.no_symlinks => Ok(path), Ok(path) if path.starts_with(&app_root_dir) => Ok(path), _ => Err(ContextualError::InvalidHttpRequestError( "Invalid value for 'path' parameter".to_string(), @@ -113,3 +121,64 @@ pub async fn upload_file( .append_header((header::LOCATION, return_path)) .finish()) } + +/// Guarantee that the path is relative and cannot traverse back to parent directories +/// and optionally prevent traversing hidden directories. +/// +/// See the unit tests tests::test_sanitize_path* for examples +fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option<PathBuf> { + let mut buf = PathBuf::new(); + + for comp in path.components() { + match comp { + Component::Normal(name) => buf.push(name), + Component::ParentDir => { + buf.pop(); + } + _ => (), + } + } + + // Double-check that all components are Normal and check for hidden dirs + for comp in buf.components() { + match comp { + Component::Normal(_) if traverse_hidden => (), + Component::Normal(name) if !name.to_str()?.starts_with('.') => (), + _ => return None, + } + } + + Some(buf) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use rstest::rstest; + + #[rstest] + #[case("/foo", "foo")] + #[case("////foo", "foo")] + #[case("C:/foo", if cfg!(windows) { "foo" } else { "C:/foo" })] + #[case("../foo", "foo")] + #[case("../foo/../bar/abc", "bar/abc")] + fn test_sanitize_path(#[case] input: &str, #[case] output: &str) { + assert_eq!( + sanitize_path(Path::new(input), true).unwrap(), + Path::new(output) + ); + assert_eq!( + sanitize_path(Path::new(input), false).unwrap(), + Path::new(output) + ); + } + + #[rstest] + #[case(".foo")] + #[case("/.foo")] + #[case("foo/.bar/foo")] + fn test_sanitize_path_no_hidden_files(#[case] input: &str) { + assert_eq!(sanitize_path(Path::new(input), false), None); + } +} diff --git a/src/listing.rs b/src/listing.rs index 20768f1..9273025 100644 --- a/src/listing.rs +++ b/src/listing.rs @@ -28,7 +28,7 @@ mod percent_encode_sets { } /// Query parameters -#[derive(Deserialize)] +#[derive(Deserialize, Default)] pub struct QueryParameters { pub path: Option<PathBuf>, pub sort: Option<SortingMethod>, @@ -389,25 +389,11 @@ pub fn directory_listing( pub fn extract_query_parameters(req: &HttpRequest) -> QueryParameters { match Query::<QueryParameters>::from_query(req.query_string()) { - Ok(query) => QueryParameters { - sort: query.sort, - order: query.order, - download: query.download, - raw: query.raw, - qrcode: query.qrcode.to_owned(), - path: query.path.clone(), - }, + Ok(Query(query_params)) => query_params, Err(e) => { let err = ContextualError::ParseError("query parameters".to_string(), e.to_string()); errors::log_error_chain(err.to_string()); - QueryParameters { - sort: None, - order: None, - download: None, - raw: None, - qrcode: None, - path: None, - } + QueryParameters::default() } } } diff --git a/src/main.rs b/src/main.rs index 5259bee..133f320 100644 --- a/src/main.rs +++ b/src/main.rs @@ -297,17 +297,10 @@ fn create_tcp_listener(addr: SocketAddr) -> io::Result<TcpListener> { } fn configure_header(conf: &MiniserveConfig) -> middleware::DefaultHeaders { - let headers = conf.clone().header; - - let mut default_headers = middleware::DefaultHeaders::new(); - for header in headers { - for (header_name, header_value) in header.into_iter() { - if let Some(header_name) = header_name { - default_headers = default_headers.header(&header_name, header_value); - } - } - } - default_headers + conf.header.iter().flatten().fold( + middleware::DefaultHeaders::new(), + |headers, (header_name, header_value)| headers.header(header_name, header_value), + ) } /// Configures the Actix application diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 698eb46..5e764ba 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -1,6 +1,7 @@ mod fixtures; -use fixtures::{server, Error, TestServer}; +use assert_fs::fixture::TempDir; +use fixtures::{server, server_no_stderr, tmpdir, Error, TestServer}; use reqwest::blocking::{multipart, Client}; use rstest::rstest; use select::document::Document; @@ -78,3 +79,91 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } + +/// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in +/// file name (Content-Disposition) +/// +/// see: https://github.com/svenstaro/miniserve/issues/518 +#[rstest] +#[case("foo", "bar", "foo/bar")] +#[case("/../foo", "bar", "foo/bar")] +#[case("/foo", "/../bar", "foo/bar")] +#[case("C:/foo", "C:/bar", if cfg!(windows) { "foo/bar" } else { "C:/foo/C:/bar" })] +#[case(r"C:\foo", r"C:\bar", if cfg!(windows) { "foo/bar" } else { r"C:\foo/C:\bar" })] +#[case(r"\foo", r"\..\bar", if cfg!(windows) { "foo/bar" } else { r"\foo/\..\bar" })] +fn prevent_path_traversal_attacks( + #[with(&["-u"])] server: TestServer, + #[case] path: &str, + #[case] filename: &'static str, + #[case] expected: &str, +) -> Result<(), Error> { + // Create test directories + use std::fs::create_dir_all; + create_dir_all(server.path().join("foo")).unwrap(); + if !cfg!(windows) { + for dir in &["C:/foo/C:", r"C:\foo", r"\foo"] { + create_dir_all(server.path().join(dir)).expect(&format!("failed to create: {:?}", dir)); + } + } + + let expected_path = server.path().join(expected); + assert!(!expected_path.exists()); + + // Perform the actual upload. + let part = multipart::Part::text("this should be uploaded") + .file_name(filename) + .mime_str("text/plain")?; + let form = multipart::Form::new().part("file_to_upload", part); + + Client::new() + .post(server.url().join(&format!("/upload?path={}", path))?) + .multipart(form) + .send()? + .error_for_status()?; + + // Make sure that the file was uploaded to the expected path + assert!(expected_path.exists()); + + Ok(()) +} + +/// Test uploading to symlink directories that point outside the server root. +/// See https://github.com/svenstaro/miniserve/issues/466 +#[rstest] +#[case(server(&["-u"]), true)] +#[case(server_no_stderr(&["-u", "--no-symlinks"]), false)] +fn upload_to_symlink_directory( + #[case] server: TestServer, + #[case] ok: bool, + tmpdir: TempDir, +) -> Result<(), Error> { + #[cfg(unix)] + use std::os::unix::fs::symlink as symlink_dir; + #[cfg(windows)] + use std::os::windows::fs::symlink_dir; + + // Create symlink directory "foo" to point outside the root + let (dir, filename) = ("foo", "bar"); + symlink_dir(tmpdir.path(), server.path().join(dir)).unwrap(); + + let full_path = server.path().join(dir).join(filename); + assert!(!full_path.exists()); + + // Try to upload + let part = multipart::Part::text("this should be uploaded") + .file_name(filename) + .mime_str("text/plain")?; + let form = multipart::Form::new().part("file_to_upload", part); + + let status = Client::new() + .post(server.url().join(&format!("/upload?path={}", dir))?) + .multipart(form) + .send()? + .error_for_status(); + + // Make sure upload behave as expected + assert_eq!(status.is_ok(), ok); + assert_eq!(full_path.exists(), ok); + + Ok(()) +} |