From 03278e4de4f540cbd0e346e9df878c7e6798d757 Mon Sep 17 00:00:00 2001 From: David Knaack Date: Mon, 31 Jul 2023 21:44:31 +0200 Subject: [PATCH] fix(git): prevent `core.fsmonitor` from executing external commands (#3981) --- src/context.rs | 52 +++++++++++++++++++++++ src/modules/git_metrics.rs | 18 ++------ src/modules/git_status.rs | 86 +++++++++++++++++++++++++++----------- 3 files changed, 116 insertions(+), 40 deletions(-) diff --git a/src/context.rs b/src/context.rs index 54a2310a4..124b5ced1 100644 --- a/src/context.rs +++ b/src/context.rs @@ -313,6 +313,12 @@ impl<'a> Context<'a> { let branch = get_current_branch(&repository); let remote = get_remote_repository_info(&repository, branch.as_deref()); let path = repository.path().to_path_buf(); + + let fs_monitor_value_is_true = repository + .config_snapshot() + .boolean("core.fs_monitor") + .unwrap_or(false); + Ok(Repo { repo: shared_repo, branch, @@ -320,6 +326,7 @@ impl<'a> Context<'a> { path, state: repository.state(), remote, + fs_monitor_value_is_true, }) }) } @@ -589,6 +596,10 @@ pub struct Repo { /// Remote repository pub remote: Option, + + /// Contains `true` if the value of `core.fsmonitor` is set to `true`. + /// If not `true`, `fsmonitor` is explicitly disabled in git commands. + fs_monitor_value_is_true: bool, } impl Repo { @@ -596,6 +607,47 @@ impl Repo { pub fn open(&self) -> Repository { self.repo.to_thread_local() } + + /// Wrapper to execute external git commands. + /// Handles adding the appropriate `--git-dir` and `--work-tree` flags to the command. + /// Also handles additional features required for security, such as disabling `fsmonitor`. + /// At this time, mocking is not supported. + pub fn exec_git + Debug>( + &self, + context: &Context, + git_args: &[T], + ) -> Option { + let mut command = create_command("git").ok()?; + + // A value of `true` should not execute external commands. + let fsm_config_value = if self.fs_monitor_value_is_true { + "core.fsmonitor=true" + } else { + "core.fsmonitor=" + }; + + command.env("GIT_OPTIONAL_LOCKS", "0").args([ + OsStr::new("-C"), + context.current_dir.as_os_str(), + OsStr::new("--git-dir"), + self.path.as_os_str(), + OsStr::new("-c"), + OsStr::new(fsm_config_value), + ]); + + // Bare repositories might not have a workdir, so we need to check for that. + if let Some(wt) = self.workdir.as_ref() { + command.args([OsStr::new("--work-tree"), wt.as_os_str()]); + } + + command.args(git_args); + log::trace!("Executing git command: {:?}", command); + + exec_timeout( + &mut command, + Duration::from_millis(context.root_config.command_timeout), + ) + } } /// Remote repository diff --git a/src/modules/git_metrics.rs b/src/modules/git_metrics.rs index b82784ad2..f5a653fb2 100644 --- a/src/modules/git_metrics.rs +++ b/src/modules/git_metrics.rs @@ -1,5 +1,4 @@ use regex::Regex; -use std::ffi::OsStr; use crate::{ config::ModuleConfig, configs::git_metrics::GitMetricsConfig, @@ -21,23 +20,12 @@ pub fn module<'a>(context: &'a Context) -> Option> { }; let repo = context.get_repo().ok()?; - let repo_root = repo.workdir.as_ref()?; - - let mut args = vec![ - OsStr::new("--git-dir"), - repo.path.as_os_str(), - OsStr::new("--work-tree"), - repo_root.as_os_str(), - OsStr::new("--no-optional-locks"), - OsStr::new("diff"), - OsStr::new("--shortstat"), - ]; - + let mut git_args = vec!["diff", "--shortstat"]; if config.ignore_submodules { - args.push(OsStr::new("--ignore-submodules")); + git_args.push("--ignore-submodules"); } - let diff = context.exec_cmd("git", &args)?.stdout; + let diff = repo.exec_git(context, &git_args)?.stdout; let stats = GitDiff::parse(&diff); diff --git a/src/modules/git_status.rs b/src/modules/git_status.rs index dad848b63..e54ba7000 100644 --- a/src/modules/git_status.rs +++ b/src/modules/git_status.rs @@ -4,9 +4,9 @@ use regex::Regex; use super::{Context, Module, ModuleConfig}; use crate::configs::git_status::GitStatusConfig; +use crate::context; use crate::formatter::StringFormatter; use crate::segment::Segment; -use std::ffi::OsStr; use std::sync::Arc; const ALL_STATUS_FORMAT: &str = @@ -31,10 +31,8 @@ pub fn module<'a>(context: &'a Context) -> Option> { let mut module = context.new_module("git_status"); let config: GitStatusConfig = GitStatusConfig::try_load(module.config); - let info = Arc::new(GitStatusInfo::load(context, config.clone())); - - //Return None if not in git repository - context.get_repo().ok()?; + // Return None if not in git repository + let repo = context.get_repo().ok()?; if let Some(git_status) = git_status_wsl(context, &config) { if git_status.is_empty() { @@ -44,6 +42,8 @@ pub fn module<'a>(context: &'a Context) -> Option> { return Some(module); } + let info = Arc::new(GitStatusInfo::load(context, repo, config.clone())); + let parsed = StringFormatter::new(config.format).and_then(|formatter| { formatter .map_meta(|variable, _| match variable { @@ -128,15 +128,21 @@ pub fn module<'a>(context: &'a Context) -> Option> { struct GitStatusInfo<'a> { context: &'a Context<'a>, + repo: &'a context::Repo, config: GitStatusConfig<'a>, repo_status: OnceCell>, stashed_count: OnceCell>, } impl<'a> GitStatusInfo<'a> { - pub fn load(context: &'a Context, config: GitStatusConfig<'a>) -> Self { + pub fn load( + context: &'a Context, + repo: &'a context::Repo, + config: GitStatusConfig<'a>, + ) -> Self { Self { context, + repo, config, repo_status: OnceCell::new(), stashed_count: OnceCell::new(), @@ -148,19 +154,20 @@ impl<'a> GitStatusInfo<'a> { } pub fn get_repo_status(&self) -> &Option { - self.repo_status - .get_or_init(|| match get_repo_status(self.context, &self.config) { + self.repo_status.get_or_init(|| { + match get_repo_status(self.context, self.repo, &self.config) { Some(repo_status) => Some(repo_status), None => { log::debug!("get_repo_status: git status execution failed"); None } - }) + } + }) } pub fn get_stashed(&self) -> &Option { self.stashed_count - .get_or_init(|| match get_stashed_count(self.context) { + .get_or_init(|| match get_stashed_count(self.repo) { Some(stashed_count) => Some(stashed_count), None => { log::debug!("get_stashed_count: git stash execution failed"); @@ -199,37 +206,35 @@ impl<'a> GitStatusInfo<'a> { } /// Gets the number of files in various git states (staged, modified, deleted, etc...) -fn get_repo_status(context: &Context, config: &GitStatusConfig) -> Option { +fn get_repo_status( + context: &Context, + repo: &context::Repo, + config: &GitStatusConfig, +) -> Option { log::debug!("New repo status created"); let mut repo_status = RepoStatus::default(); - let mut args = vec![ - OsStr::new("-C"), - context.current_dir.as_os_str(), - OsStr::new("--no-optional-locks"), - OsStr::new("status"), - OsStr::new("--porcelain=2"), - ]; + let mut args = vec!["status", "--porcelain=2"]; // for performance reasons, only pass flags if necessary... let has_ahead_behind = !config.ahead.is_empty() || !config.behind.is_empty(); let has_up_to_date_diverged = !config.up_to_date.is_empty() || !config.diverged.is_empty(); if has_ahead_behind || has_up_to_date_diverged { - args.push(OsStr::new("--branch")); + args.push("--branch"); } // ... and add flags that omit information the user doesn't want let has_untracked = !config.untracked.is_empty(); if !has_untracked { - args.push(OsStr::new("--untracked-files=no")); + args.push("--untracked-files=no"); } if config.ignore_submodules { - args.push(OsStr::new("--ignore-submodules=dirty")); + args.push("--ignore-submodules=dirty"); } else if !has_untracked { - args.push(OsStr::new("--ignore-submodules=untracked")); + args.push("--ignore-submodules=untracked"); } - let status_output = context.exec_cmd("git", &args)?; + let status_output = repo.exec_git(context, &args)?; let statuses = status_output.stdout.lines(); statuses.for_each(|status| { @@ -243,8 +248,8 @@ fn get_repo_status(context: &Context, config: &GitStatusConfig) -> Option Option { - let repo = context.get_repo().ok()?.open(); +fn get_stashed_count(repo: &context::Repo) -> Option { + let repo = repo.open(); let reference = match repo.try_find_reference("refs/stash") { Ok(Some(reference)) => reference, // No stash reference found @@ -742,6 +747,37 @@ mod tests { repo_dir.close() } + #[test] + #[cfg(unix)] + fn doesnt_run_fsmonitor() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + let repo_dir = fixture_repo(FixtureProvider::Git)?; + + let mut f = File::create(repo_dir.path().join("do_not_execute"))?; + write!(f, "#!/bin/sh\necho executed > executed\nsync executed")?; + let metadata = f.metadata()?; + let mut permissions = metadata.permissions(); + permissions.set_mode(0o700); + f.set_permissions(permissions)?; + f.sync_all()?; + + create_command("git")? + .args(["config", "core.fsmonitor"]) + .arg(repo_dir.path().join("do_not_execute")) + .current_dir(repo_dir.path()) + .output()?; + + ModuleRenderer::new("git_status") + .path(repo_dir.path()) + .collect(); + + let created_file = repo_dir.path().join("executed").exists(); + + assert!(!created_file); + + repo_dir.close() + } + #[test] fn shows_stashed() -> io::Result<()> { let repo_dir = fixture_repo(FixtureProvider::Git)?;