diff --git a/CHANGELOG.md b/CHANGELOG.md index f7915eafbc..683b81a6f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,7 +98,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). includes multiple fixes. * Fixed path traversal by cloning/checking out crafted Git repository containing - `..` paths. + `..`, `.jj`, `.git` paths. ## [0.22.0] - 2024-10-02 diff --git a/cli/tests/test_git_clone.rs b/cli/tests/test_git_clone.rs index fc50ee4882..ab1282ea3a 100644 --- a/cli/tests/test_git_clone.rs +++ b/cli/tests/test_git_clone.rs @@ -21,12 +21,16 @@ use crate::common::get_stdout_string; use crate::common::TestEnvironment; fn set_up_non_empty_git_repo(git_repo: &git2::Repository) { + set_up_git_repo_with_file(git_repo, "file"); +} + +fn set_up_git_repo_with_file(git_repo: &git2::Repository, filename: &str) { let signature = git2::Signature::new("Some One", "some.one@example.com", &git2::Time::new(0, 0)).unwrap(); let mut tree_builder = git_repo.treebuilder(None).unwrap(); let file_oid = git_repo.blob(b"content").unwrap(); tree_builder - .insert("file", file_oid, git2::FileMode::Blob.into()) + .insert(filename, file_oid, git2::FileMode::Blob.into()) .unwrap(); let tree_oid = tree_builder.write().unwrap(); let tree = git_repo.find_tree(tree_oid).unwrap(); @@ -577,6 +581,53 @@ fn test_git_clone_with_depth() { "#); } +#[test] +fn test_git_clone_malformed() { + let test_env = TestEnvironment::default(); + let git_repo_path = test_env.env_root().join("source"); + let git_repo = git2::Repository::init(git_repo_path).unwrap(); + let clone_path = test_env.env_root().join("clone"); + // libgit2 doesn't allow to create a malformed repo containing ".git", etc., + // but we can insert ".jj" entry. + set_up_git_repo_with_file(&git_repo, ".jj"); + + // TODO: Perhaps, this should be a user error, not an internal error. + let stderr = + test_env.jj_cmd_internal_error(test_env.env_root(), &["git", "clone", "source", "clone"]); + insta::assert_snapshot!(stderr, @r#" + Fetching into new repo in "$TEST_ENV/clone" + bookmark: main@origin [new] untracked + Setting the revset alias "trunk()" to "main@origin" + Internal error: Failed to check out commit 039a1eae03465fd3be0fbad87c9ca97303742677 + Caused by: Reserved path component .jj in $TEST_ENV/clone/.jj + "#); + + // The cloned workspace isn't usable. + let stderr = test_env.jj_cmd_failure(&clone_path, &["status"]); + insta::assert_snapshot!(stderr, @r##" + Error: The working copy is stale (not updated since operation 4a8ddda0ff63). + Hint: Run `jj workspace update-stale` to update it. + See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy for more information. + "##); + + // The error can be somehow recovered. + // TODO: add an update-stale flag to reset the working-copy? + let stderr = test_env.jj_cmd_internal_error(&clone_path, &["workspace", "update-stale"]); + insta::assert_snapshot!(stderr, @r#" + Internal error: Failed to check out commit 039a1eae03465fd3be0fbad87c9ca97303742677 + Caused by: Reserved path component .jj in $TEST_ENV/clone/.jj + "#); + let (_stdout, stderr) = + test_env.jj_cmd_ok(&clone_path, &["new", "root()", "--ignore-working-copy"]); + insta::assert_snapshot!(stderr, @""); + let stdout = test_env.jj_cmd_success(&clone_path, &["status"]); + insta::assert_snapshot!(stdout, @r#" + The working copy is clean + Working copy : zsuskuln f652c321 (empty) (no description set) + Parent commit: zzzzzzzz 00000000 (empty) (no description set) + "#); +} + fn get_bookmark_output(test_env: &TestEnvironment, repo_path: &Path) -> String { test_env.jj_cmd_success(repo_path, &["bookmark", "list", "--all-remotes"]) } diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 018910119d..734dd47bbe 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -432,6 +432,9 @@ fn sparse_patterns_from_proto( /// function returns `Ok(None)` to signal that the path should be skipped. /// The `working_copy_path` directory may be a symlink. /// +/// If an existing or newly-created sub directory points to ".git" or ".jj", +/// this function returns an error. +/// /// Note that this does not prevent TOCTOU bugs caused by concurrent checkouts. /// Another process may remove the directory created by this function and put a /// symlink there. @@ -442,11 +445,15 @@ fn create_parent_dirs( let (parent_path, basename) = repo_path.split().expect("repo path shouldn't be root"); let mut dir_path = working_copy_path.to_owned(); for c in parent_path.components() { + // Ensure that the name is a normal entry of the current dir_path. dir_path.push(c.to_fs_name().map_err(|err| err.with_path(repo_path))?); - match fs::create_dir(&dir_path) { - Ok(()) => {} // New directory + // A directory named ".git" or ".jj" can be temporarily created. It + // might trick workspace path discovery, but is harmless so long as the + // directory is empty. + let new_dir_created = match fs::create_dir(&dir_path) { + Ok(()) => true, // New directory Err(err) => match dir_path.symlink_metadata() { - Ok(m) if m.is_dir() => {} // Existing directory + Ok(m) if m.is_dir() => false, // Existing directory Ok(_) => { return Ok(None); // Skip existing file or symlink } @@ -460,7 +467,14 @@ fn create_parent_dirs( }) } }, - } + }; + // Invalid component (e.g. "..") should have been rejected. + // The current dir_path should be an entry of dir_path.parent(). + reject_reserved_existing_path(&dir_path).inspect_err(|_| { + if new_dir_created { + fs::remove_dir(&dir_path).ok(); + } + })?; } let mut file_path = dir_path; @@ -472,11 +486,16 @@ fn create_parent_dirs( Ok(Some(file_path)) } -/// Removes existing file named `disk_path` if any. -fn remove_old_file(disk_path: &Path) -> Result<(), CheckoutError> { +/// Removes existing file named `disk_path` if any. Returns `Ok(true)` if the +/// file was there and got removed. +/// +/// If the existing file points to ".git" or ".jj", this function returns an +/// error. +fn remove_old_file(disk_path: &Path) -> Result { + reject_reserved_existing_path(disk_path)?; match fs::remove_file(disk_path) { - Ok(()) => Ok(()), - Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(()), + Ok(()) => Ok(true), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false), Err(err) => Err(CheckoutError::Other { message: format!("Failed to remove file {}", disk_path.display()), err: err.into(), @@ -489,16 +508,71 @@ fn remove_old_file(disk_path: &Path) -> Result<(), CheckoutError> { /// If the file already exists, this function return `Ok(false)` to signal /// that the path should be skipped. /// +/// If the path may point to ".git" or ".jj" entry, this function returns an +/// error. +/// /// This function can fail if `disk_path.parent()` isn't a directory. fn can_create_new_file(disk_path: &Path) -> Result { - match disk_path.symlink_metadata() { - Ok(_) => Ok(false), - Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(true), - Err(err) => Err(CheckoutError::Other { - message: format!("Failed to stat {}", disk_path.display()), + // New file or symlink will be created by caller. If it were pointed to by + // name ".git" or ".jj", git/jj CLI could be tricked to load configuration + // from an attacker-controlled location. So we first test the path by + // creating an empty file. + let new_file_created = match OpenOptions::new() + .write(true) + .create_new(true) // Don't overwrite, don't follow symlink + .open(disk_path) + { + Ok(_) => true, + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => false, + Err(err) => { + return Err(CheckoutError::Other { + message: format!("Failed to create temporary file {}", disk_path.display()), + err: err.into(), + }); + } + }; + reject_reserved_existing_path(disk_path).inspect_err(|_| { + if new_file_created { + fs::remove_file(disk_path).ok(); + } + })?; + if new_file_created { + fs::remove_file(disk_path).map_err(|err| CheckoutError::Other { + message: format!("Failed to remove temporary file {}", disk_path.display()), err: err.into(), - }), + })?; + } + Ok(new_file_created) +} + +const RESERVED_DIR_NAMES: &[&str] = &[".git", ".jj"]; + +/// Suppose the `disk_path` exists, checks if the last component points to +/// ".git" or ".jj" in the same parent directory. +fn reject_reserved_existing_path(disk_path: &Path) -> Result<(), CheckoutError> { + let parent_dir_path = disk_path.parent().expect("content path shouldn't be root"); + for name in RESERVED_DIR_NAMES { + let reserved_path = parent_dir_path.join(name); + match same_file::is_same_file(disk_path, &reserved_path) { + Ok(true) => { + return Err(CheckoutError::ReservedPathComponent { + path: disk_path.to_owned(), + name, + }); + } + Ok(false) => {} + // If the existing disk_path pointed to the reserved path, the + // reserved path would exist. + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(err) => { + return Err(CheckoutError::Other { + message: format!("Failed to validate path {}", disk_path.display()), + err: err.into(), + }); + } + } } + Ok(()) } fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { @@ -980,7 +1054,7 @@ impl TreeState { path: file_name.clone(), })?; - if name == ".jj" || name == ".git" { + if RESERVED_DIR_NAMES.contains(&name) { return Ok(()); } let path = dir.join(RepoPathComponent::new(name)); @@ -1455,9 +1529,10 @@ impl TreeState { stats.skipped_files += 1; continue; }; - if present_before { - remove_old_file(&disk_path)?; - } else if !can_create_new_file(&disk_path)? { + // If the path was present, check reserved path first and delete it. + let was_present = present_before && remove_old_file(&disk_path)?; + // If not, create temporary file to test the path validity. + if !was_present && !can_create_new_file(&disk_path)? { changed_file_states.push((path, FileState::placeholder())); stats.skipped_files += 1; continue; diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 9a96e44ed8..09f71e555b 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -264,6 +264,14 @@ pub enum CheckoutError { /// Path in the commit contained invalid component such as `..`. #[error(transparent)] InvalidRepoPath(#[from] InvalidRepoPathError), + /// Path contained reserved name which cannot be checked out to disk. + #[error("Reserved path component {name} in {path}")] + ReservedPathComponent { + /// The file or directory path. + path: PathBuf, + /// The reserved path component. + name: &'static str, + }, /// Reading or writing from the commit backend failed. #[error("Internal backend error")] InternalBackendError(#[from] BackendError), diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 19ed954e80..e00f4eea6d 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -68,6 +68,29 @@ fn check_icase_fs(dir: &Path) -> bool { dir.join(upper_name).try_exists().unwrap() } +/// Returns true if the directory appears to ignore some unicode zero-width +/// characters, as in HFS+. +fn check_hfs_plus(dir: &Path) -> bool { + let test_file = tempfile::Builder::new() + .prefix("hfs-plus-\u{200c}-") + .tempfile_in(dir) + .unwrap(); + let orig_name = test_file.path().file_name().unwrap().to_str().unwrap(); + let stripped_name = orig_name.replace('\u{200c}', ""); + assert_ne!(orig_name, stripped_name); + dir.join(stripped_name).try_exists().unwrap() +} + +/// Returns true if the directory appears to support Windows short file names. +fn check_vfat(dir: &Path) -> bool { + let _test_file = tempfile::Builder::new() + .prefix("vfattest-") + .tempfile_in(dir) + .unwrap(); + let short_name = "VFATTE~1"; + dir.join(short_name).try_exists().unwrap() +} + fn to_owned_path_vec(paths: &[&RepoPath]) -> Vec { paths.iter().map(|&path| path.to_owned()).collect() } @@ -1419,6 +1442,244 @@ fn test_check_out_malformed_file_path_windows(file_path_str: &str) { assert!(!workspace_root.parent().unwrap().join("pwned").exists()); } +#[test_case(".git"; "root .git file")] +#[test_case(".jj"; "root .jj file")] +#[test_case(".git/pwned"; "root .git dir")] +#[test_case(".jj/pwned"; "root .jj dir")] +#[test_case("sub/.git"; "sub .git file")] +#[test_case("sub/.jj"; "sub .jj file")] +#[test_case("sub/.git/pwned"; "sub .git dir")] +#[test_case("sub/.jj/pwned"; "sub .jj dir")] +fn test_check_out_reserved_file_path(file_path_str: &str) { + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + std::fs::create_dir(workspace_root.join(".git")).unwrap(); + + let file_path = RepoPath::from_internal_string(file_path_str); + let disk_path = file_path.to_fs_path_unchecked(&workspace_root); + let tree1 = create_tree(repo, &[(file_path, "contents")]); + let tree2 = create_tree(repo, &[]); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let commit2 = commit_with_tree(repo.store(), tree2.id()); + + // Checkout should fail. + let ws = &mut test_workspace.workspace; + let result = ws.check_out(repo.op_id().clone(), None, &commit1); + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + + // Therefore, "pwned" file shouldn't be created. + if ![".git", ".jj"].contains(&file_path_str) { + assert!(!disk_path.exists()); + } + assert!(!workspace_root.join(".git").join("pwned").exists()); + assert!(!workspace_root.join(".jj").join("pwned").exists()); + assert!(!workspace_root.join("sub").join(".git").exists()); + assert!(!workspace_root.join("sub").join(".jj").exists()); + + // Pretend that the checkout somehow succeeded. + let mut locked_ws = ws.start_working_copy_mutation().unwrap(); + locked_ws.locked_wc().reset(&commit1).unwrap(); + locked_ws.finish(repo.op_id().clone()).unwrap(); + if ![".git", ".jj"].contains(&file_path_str) { + std::fs::create_dir_all(disk_path.parent().unwrap()).unwrap(); + std::fs::write(&disk_path, "").unwrap(); + } + + // Check out empty tree, which tries to remove the file. + let result = ws.check_out(repo.op_id().clone(), None, &commit2); + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + + // The existing file shouldn't be removed. + assert!(disk_path.exists()); +} + +#[test_case(".Git/pwned"; "root .git dir")] +#[test_case(".jJ/pwned"; "root .jj dir")] +#[test_case("sub/.GIt"; "sub .git file")] +#[test_case("sub/.JJ"; "sub .jj file")] +#[test_case("sub/.gIT/pwned"; "sub .git dir")] +#[test_case("sub/.Jj/pwned"; "sub .jj dir")] +fn test_check_out_reserved_file_path_icase_fs(file_path_str: &str) { + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + std::fs::create_dir(workspace_root.join(".git")).unwrap(); + let is_icase_fs = check_icase_fs(&workspace_root); + + let file_path = RepoPath::from_internal_string(file_path_str); + let disk_path = file_path.to_fs_path_unchecked(&workspace_root); + let tree1 = create_tree(repo, &[(file_path, "contents")]); + let tree2 = create_tree(repo, &[]); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let commit2 = commit_with_tree(repo.store(), tree2.id()); + + // Checkout should fail on icase fs. + let ws = &mut test_workspace.workspace; + let result = ws.check_out(repo.op_id().clone(), None, &commit1); + if is_icase_fs { + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + } else { + assert_matches!(result, Ok(_)); + } + + // Therefore, "pwned" file shouldn't be created. + if is_icase_fs { + assert!(!disk_path.exists()); + } + assert!(!workspace_root.join(".git").join("pwned").exists()); + assert!(!workspace_root.join(".jj").join("pwned").exists()); + assert!(!workspace_root.join("sub").join(".git").exists()); + assert!(!workspace_root.join("sub").join(".jj").exists()); + + // Pretend that the checkout somehow succeeded. + let mut locked_ws = ws.start_working_copy_mutation().unwrap(); + locked_ws.locked_wc().reset(&commit1).unwrap(); + locked_ws.finish(repo.op_id().clone()).unwrap(); + std::fs::create_dir_all(disk_path.parent().unwrap()).unwrap(); + std::fs::write(&disk_path, "").unwrap(); + + // Check out empty tree, which tries to remove the file. + let result = ws.check_out(repo.op_id().clone(), None, &commit2); + if is_icase_fs { + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + } else { + assert_matches!(result, Ok(_)); + } + + // The existing file shouldn't be removed on icase fs. + if is_icase_fs { + assert!(disk_path.exists()); + } +} + +// Here we don't test ignored characters exhaustively because our implementation +// isn't using deny list. +#[test_case("\u{200c}.git/pwned"; "root .git dir")] +#[test_case(".\u{200d}jj/pwned"; "root .jj dir")] +#[test_case("sub/.g\u{200c}it"; "sub .git file")] +#[test_case("sub/.jj\u{200d}"; "sub .jj file")] +#[test_case("sub/.gi\u{200e}t/pwned"; "sub .git dir")] +#[test_case("sub/.jj\u{200f}/pwned"; "sub .jj dir")] +fn test_check_out_reserved_file_path_hfs_plus(file_path_str: &str) { + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + std::fs::create_dir(workspace_root.join(".git")).unwrap(); + let is_hfs_plus = check_hfs_plus(&workspace_root); + + let file_path = RepoPath::from_internal_string(file_path_str); + let disk_path = file_path.to_fs_path_unchecked(&workspace_root); + let tree1 = create_tree(repo, &[(file_path, "contents")]); + let tree2 = create_tree(repo, &[]); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let commit2 = commit_with_tree(repo.store(), tree2.id()); + + // Checkout should fail on HFS+-like fs. + let ws = &mut test_workspace.workspace; + let result = ws.check_out(repo.op_id().clone(), None, &commit1); + if is_hfs_plus { + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + } else { + assert_matches!(result, Ok(_)); + } + + // Therefore, "pwned" file shouldn't be created. + if is_hfs_plus { + assert!(!disk_path.exists()); + } + assert!(!workspace_root.join(".git").join("pwned").exists()); + assert!(!workspace_root.join(".jj").join("pwned").exists()); + assert!(!workspace_root.join("sub").join(".git").exists()); + assert!(!workspace_root.join("sub").join(".jj").exists()); + + // Pretend that the checkout somehow succeeded. + let mut locked_ws = ws.start_working_copy_mutation().unwrap(); + locked_ws.locked_wc().reset(&commit1).unwrap(); + locked_ws.finish(repo.op_id().clone()).unwrap(); + std::fs::create_dir_all(disk_path.parent().unwrap()).unwrap(); + std::fs::write(&disk_path, "").unwrap(); + + // Check out empty tree, which tries to remove the file. + let result = ws.check_out(repo.op_id().clone(), None, &commit2); + if is_hfs_plus { + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + } else { + assert_matches!(result, Ok(_)); + } + + // The existing file shouldn't be removed on HFS+-like fs. + if is_hfs_plus { + assert!(disk_path.exists()); + } +} + +#[test_case("GIT~1/pwned"; "root .git dir short name")] +#[test_case("JJ~1/pwned"; "root .jj dir short name")] +#[test_case(".GIT./pwned"; "root .git dir trailing dots")] +#[test_case(".JJ../pwned"; "root .jj dir trailing dots")] +#[test_case("sub/.GIT.."; "sub .git file trailing dots")] +#[test_case("sub/.JJ."; "sub .jj file trailing dots")] +// TODO: Add more weird patterns? +// - https://en.wikipedia.org/wiki/8.3_filename ".GIxxxx~2" +// - See is_ntfs_dotgit() of Git and pathauditor of Mercurial +fn test_check_out_reserved_file_path_vfat(file_path_str: &str) { + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + std::fs::create_dir(workspace_root.join(".git")).unwrap(); + let is_vfat = check_vfat(&workspace_root); + + let file_path = RepoPath::from_internal_string(file_path_str); + let disk_path = file_path.to_fs_path_unchecked(&workspace_root); + let tree1 = create_tree(repo, &[(file_path, "contents")]); + let tree2 = create_tree(repo, &[]); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let commit2 = commit_with_tree(repo.store(), tree2.id()); + + // Checkout should fail on VFAT-like fs. + let ws = &mut test_workspace.workspace; + let result = ws.check_out(repo.op_id().clone(), None, &commit1); + if is_vfat { + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + } else { + assert_matches!(result, Ok(_)); + } + + // Therefore, "pwned" file shouldn't be created. + if is_vfat { + assert!(!disk_path.exists()); + } + assert!(!workspace_root.join(".git").join("pwned").exists()); + assert!(!workspace_root.join(".jj").join("pwned").exists()); + assert!(!workspace_root.join("sub").join(".git").exists()); + assert!(!workspace_root.join("sub").join(".jj").exists()); + + // Pretend that the checkout somehow succeeded. + let mut locked_ws = ws.start_working_copy_mutation().unwrap(); + locked_ws.locked_wc().reset(&commit1).unwrap(); + locked_ws.finish(repo.op_id().clone()).unwrap(); + std::fs::create_dir_all(disk_path.parent().unwrap()).unwrap(); + std::fs::write(&disk_path, "").unwrap(); + + // Check out empty tree, which tries to remove the file. + let result = ws.check_out(repo.op_id().clone(), None, &commit2); + if is_vfat { + assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. })); + } else { + assert_matches!(result, Ok(_)); + } + + // The existing file shouldn't be removed on VFAT-like fs. + if is_vfat { + assert!(disk_path.exists()); + } +} + #[test] fn test_fsmonitor() { let settings = testutils::user_settings();