From 89a31ff473470513e3855edaa1c73cdedef0643d Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" <75863576+jeffwashington@users.noreply.github.com> Date: Wed, 18 Aug 2021 15:49:02 -0500 Subject: [PATCH] change untar to use unpack instead of unpack_in (#19216) * change untar to use unpack instead of unpack_in * hacky, but maybe passes tests * chore: bump tar from 0.4.35 to 0.4.37 Bumps [tar](https://github.com/alexcrichton/tar-rs) from 0.4.35 to 0.4.37. - [Release notes](https://github.com/alexcrichton/tar-rs/releases) - [Commits](https://github.com/alexcrichton/tar-rs/compare/0.4.35...0.4.37) --- updated-dependencies: - dependency-name: tar dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] * [auto-commit] Update all Cargo lock files * cleanup * cleanup, add validate_inside_dst * collapse use Co-authored-by: Tyera Eulberg * delete comment line * add comments Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite Co-authored-by: Tyera Eulberg --- Cargo.lock | 4 +- download-utils/Cargo.toml | 2 +- install/Cargo.toml | 2 +- programs/bpf/Cargo.lock | 4 +- runtime/Cargo.toml | 2 +- runtime/src/hardened_unpack.rs | 87 ++++++++++++++++++++++++++++++++-- sdk/cargo-build-bpf/Cargo.toml | 2 +- 7 files changed, 91 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 721959945a..ec428c8a4a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6242,9 +6242,9 @@ checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369" [[package]] name = "tar" -version = "0.4.35" +version = "0.4.37" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7d779dc6aeff029314570f666ec83f19df7280bb36ef338442cfa8c604021b80" +checksum = "d6f5515d3add52e0bbdcad7b83c388bb36ba7b754dda3b5f5bc2d38640cdba5c" dependencies = [ "filetime", "libc", diff --git a/download-utils/Cargo.toml b/download-utils/Cargo.toml index 0f9d84b14e..995457566b 100644 --- a/download-utils/Cargo.toml +++ b/download-utils/Cargo.toml @@ -17,7 +17,7 @@ log = "0.4.14" reqwest = { version = "0.11.4", default-features = false, features = ["blocking", "rustls-tls", "json"] } solana-sdk = { path = "../sdk", version = "=1.8.0" } solana-runtime = { path = "../runtime", version = "=1.8.0" } -tar = "0.4.35" +tar = "0.4.37" [lib] crate-type = ["lib"] diff --git a/install/Cargo.toml b/install/Cargo.toml index 5198b8ea7e..234a38e964 100644 --- a/install/Cargo.toml +++ b/install/Cargo.toml @@ -32,7 +32,7 @@ solana-logger = { path = "../logger", version = "=1.8.0" } solana-sdk = { path = "../sdk", version = "=1.8.0" } solana-version = { path = "../version", version = "=1.8.0" } semver = "1.0.4" -tar = "0.4.35" +tar = "0.4.37" tempfile = "3.2.0" url = "2.2.2" diff --git a/programs/bpf/Cargo.lock b/programs/bpf/Cargo.lock index f1909b5c76..134203ce34 100644 --- a/programs/bpf/Cargo.lock +++ b/programs/bpf/Cargo.lock @@ -3487,9 +3487,9 @@ dependencies = [ [[package]] name = "tar" -version = "0.4.35" +version = "0.4.37" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7d779dc6aeff029314570f666ec83f19df7280bb36ef338442cfa8c604021b80" +checksum = "d6f5515d3add52e0bbdcad7b83c388bb36ba7b754dda3b5f5bc2d38640cdba5c" dependencies = [ "filetime", "libc", diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index b475d53c9f..c920bf0a61 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -49,7 +49,7 @@ solana-secp256k1-program = { path = "../programs/secp256k1", version = "=1.8.0" solana-stake-program = { path = "../programs/stake", version = "=1.8.0" } solana-vote-program = { path = "../programs/vote", version = "=1.8.0" } symlink = "0.1.0" -tar = "0.4.35" +tar = "0.4.37" tempfile = "3.2.0" thiserror = "1.0" zstd = "0.9.0" diff --git a/runtime/src/hardened_unpack.rs b/runtime/src/hardened_unpack.rs index fdaa424f6c..7580ab4b10 100644 --- a/runtime/src/hardened_unpack.rs +++ b/runtime/src/hardened_unpack.rs @@ -9,7 +9,7 @@ use { fs::{self, File}, io::{BufReader, Read}, path::{ - Component::{CurDir, Normal}, + Component::{self, CurDir, Normal}, Path, PathBuf, }, time::Instant, @@ -161,9 +161,14 @@ where )?; total_count = checked_total_count_increment(total_count, limit_count)?; - // unpack_in does its own sanitization - // ref: https://docs.rs/tar/*/tar/struct.Entry.html#method.unpack_in - check_unpack_result(entry.unpack_in(unpack_dir)?, path_str)?; + let target = sanitize_path(&entry.path()?, unpack_dir)?; // ? handles file system errors + if target.is_none() { + continue; // skip it + } + let target = target.unwrap(); + + let unpack = entry.unpack(target); + check_unpack_result(unpack.map(|_unpack| true)?, path_str)?; // Sanitize permissions. let mode = match entry.header().entry_type() { @@ -199,6 +204,80 @@ where } } +// return Err on file system error +// return Some(path) if path is good +// return None if we should skip this file +fn sanitize_path(entry_path: &Path, dst: &Path) -> Result> { + // We cannot call unpack_in because it errors if we try to use 2 account paths. + // So, this code is borrowed from unpack_in + // ref: https://docs.rs/tar/*/tar/struct.Entry.html#method.unpack_in + let mut file_dst = dst.to_path_buf(); + const SKIP: Result> = Ok(None); + { + let path = entry_path; + for part in path.components() { + match part { + // Leading '/' characters, root paths, and '.' + // components are just ignored and treated as "empty + // components" + Component::Prefix(..) | Component::RootDir | Component::CurDir => continue, + + // If any part of the filename is '..', then skip over + // unpacking the file to prevent directory traversal + // security issues. See, e.g.: CVE-2001-1267, + // CVE-2002-0399, CVE-2005-1918, CVE-2007-4131 + Component::ParentDir => return SKIP, + + Component::Normal(part) => file_dst.push(part), + } + } + } + + // Skip cases where only slashes or '.' parts were seen, because + // this is effectively an empty filename. + if *dst == *file_dst { + return SKIP; + } + + // Skip entries without a parent (i.e. outside of FS root) + let parent = match file_dst.parent() { + Some(p) => p, + None => return SKIP, + }; + + fs::create_dir_all(parent)?; + + // Here we are different than untar_in. The code for tar::unpack_in internally calling unpack is a little different. + // ignore return value here + validate_inside_dst(dst, parent)?; + let target = parent.join(entry_path.file_name().unwrap()); + + Ok(Some(target)) +} + +// copied from: +// https://github.com/alexcrichton/tar-rs/blob/d90a02f582c03dfa0fd11c78d608d0974625ae5d/src/entry.rs#L781 +fn validate_inside_dst(dst: &Path, file_dst: &Path) -> Result { + // Abort if target (canonical) parent is outside of `dst` + let canon_parent = file_dst.canonicalize().map_err(|err| { + UnpackError::Archive(format!( + "{} while canonicalizing {}", + err, + file_dst.display() + )) + })?; + let canon_target = dst.canonicalize().map_err(|err| { + UnpackError::Archive(format!("{} while canonicalizing {}", err, dst.display())) + })?; + if !canon_parent.starts_with(&canon_target) { + return Err(UnpackError::Archive(format!( + "trying to unpack outside of destination path: {}", + canon_target.display() + ))); + } + Ok(canon_target) +} + /// Map from AppendVec file name to unpacked file system location pub type UnpackedAppendVecMap = HashMap; diff --git a/sdk/cargo-build-bpf/Cargo.toml b/sdk/cargo-build-bpf/Cargo.toml index b789618fae..77ca163982 100644 --- a/sdk/cargo-build-bpf/Cargo.toml +++ b/sdk/cargo-build-bpf/Cargo.toml @@ -16,7 +16,7 @@ regex = "1.5.4" cargo_metadata = "0.14.0" solana-sdk = { path = "..", version = "=1.8.0" } solana-download-utils = { path = "../../download-utils", version = "=1.8.0" } -tar = "0.4.35" +tar = "0.4.37" [dev-dependencies] serial_test = "*"