local_working_copy: do not create file or write in directory named .j…

…j or .git

I originally considered adding deny-list-based implementation, but the Windows
compatibility rules are super confusing and I don't have a machine to find out
possible aliases. This patch instead adds directory equivalence tests.

In order to test file entity equivalence, we first need to create a file or
directory of the requested name. It's harmless to create an empty .jj or .git
directory, but materializing .git file or symlink can temporarily set up RCE
situation. That's why new empty file is created to test the path validity. We
might want to add some optimization for safe names (e.g. ASCII, not contain
"git" or "jj", not contain "~", etc.)

That being said, I'm not pretty sure if .git/.jj in sub directory must be
checked. It's not safe to cd into the directory and run "jj", but the same
thing can be said to other tools such as "cargo". Perhaps, our minimum
requirement is to protect our metadata (= the root .jj and .git) directories.

Despite the crate name (and internal use of std::fs::File),
same_file::is_same_file() can test equivalence of directories. This is
documented and tested, so I've removed my custom implementation, which was
slightly simpler but lacks Windows support.

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
 

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", "[email protected]", &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"])
 }

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<bool, CheckoutError> {
+    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<bool, CheckoutError> {
-    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;

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),