From cbfd214993e9e3ba7466b962045350bbc0caf4d0 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 7 Sep 2023 14:36:16 -0400 Subject: [PATCH] set safe.directory when getting config for git-annex-shell or git remotes Fix more breakage caused by git's fix for CVE-2022-24765, this time involving a remote (either local or ssh) that is a repository not owned by the current user. Sponsored-by: Dartmouth College's DANDI project --- CHANGELOG | 3 +++ CmdLine/GitAnnexShell.hs | 10 ++++++-- Git/Config.hs | 9 ++++++- Git/Construct.hs | 1 + Git/Types.hs | 2 ++ Remote/Git.hs | 7 +++++- ..._with___34__wrong_reason__34___stated.mdwn | 2 ++ ..._c23de4c4ff536a214c795a805957c9f1._comment | 24 +++++++++++++++++++ ..._c75d3a033b1b8699e512df4641a38548._comment | 13 ++++++++++ ..._39e13618336181485175dddc7185160a._comment | 8 +++++++ 10 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_1_c23de4c4ff536a214c795a805957c9f1._comment create mode 100644 doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_2_c75d3a033b1b8699e512df4641a38548._comment create mode 100644 doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_3_39e13618336181485175dddc7185160a._comment diff --git a/CHANGELOG b/CHANGELOG index b6e2757436..94e94f0404 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,8 @@ git-annex (10.20230829) UNRELEASED; urgency=medium + * Fix more breakage caused by git's fix for CVE-2022-24765, this time + involving a remote (either local or ssh) that is a repository not owned + by the current user. * Removed the vendored git-lfs and the GitLfs build flag. * Fix linker optimisation in linux standalone tarballs. diff --git a/CmdLine/GitAnnexShell.hs b/CmdLine/GitAnnexShell.hs index f7e65374b8..2306c2460c 100644 --- a/CmdLine/GitAnnexShell.hs +++ b/CmdLine/GitAnnexShell.hs @@ -1,6 +1,6 @@ {- git-annex-shell main program - - - Copyright 2010-2021 Joey Hess + - Copyright 2010-2023 Joey Hess - - Licensed under the GNU AGPL version 3 or higher. -} @@ -18,6 +18,7 @@ import CmdLine.GitAnnexShell.Checks import CmdLine.GitAnnexShell.Fields import Remote.GCrypt (getGCryptUUID) import P2P.Protocol (ServerMode(..)) +import Git.Types import qualified Command.ConfigList import qualified Command.NotifyChanges @@ -123,7 +124,12 @@ builtin cmd dir params = do mkrepo = do r <- Git.Construct.repoAbsPath (toRawFilePath dir) >>= Git.Construct.fromAbsPath - Git.Config.read r + {- Since the path to the repository was specified + - explicitly, CVE-2022-24765 is not a concern, + - so tell git to treat the repository directory as safe. + -} + let r' = r { safeDirectory = True } + Git.Config.read r' `catchIO` \_ -> do hn <- fromMaybe "unknown" <$> getHostname giveup $ "failed to read git config of git repository in " ++ hn ++ " on " ++ dir ++ "; perhaps this repository is not set up correctly or has moved" diff --git a/Git/Config.hs b/Git/Config.hs index 4ff3454d54..d7a782799c 100644 --- a/Git/Config.hs +++ b/Git/Config.hs @@ -72,12 +72,19 @@ read' repo = go repo go _ = assertLocal repo $ error "internal" git_config addparams d = withCreateProcess p (git_config' p) where - params = addparams ++ ["config", "--null", "--list"] + params = addparams ++ safedirparam + ++ ["config", "--null", "--list"] p = (proc "git" params) { cwd = Just (fromRawFilePath d) , env = gitEnv repo , std_out = CreatePipe } + safedirparam = if safeDirectory repo + -- Use * rather than d, because git treats + -- "dir/" differently than "dir" when comparing for + -- safe.directory purposes. + then ["-c", "safe.directory=*"] + else [] git_config' p _ (Just hout) _ pid = forceSuccessProcess p pid `after` diff --git a/Git/Construct.hs b/Git/Construct.hs index bdab8edba1..06c3b040ad 100644 --- a/Git/Construct.hs +++ b/Git/Construct.hs @@ -287,5 +287,6 @@ newFrom l = Repo , gitEnvOverridesGitDir = False , gitGlobalOpts = [] , gitDirSpecifiedExplicitly = False + , safeDirectory = False } diff --git a/Git/Types.hs b/Git/Types.hs index ce1818e71c..61cc96dab8 100644 --- a/Git/Types.hs +++ b/Git/Types.hs @@ -53,6 +53,8 @@ data Repo = Repo , gitGlobalOpts :: [CommandParam] -- True only when --git-dir or GIT_DIR was used , gitDirSpecifiedExplicitly :: Bool + -- Set -c safe.directory when using this repository. + , safeDirectory :: Bool } deriving (Show, Eq, Ord) newtype ConfigKey = ConfigKey S.ByteString diff --git a/Remote/Git.hs b/Remote/Git.hs index 56d85a01c3..2886b06eb8 100644 --- a/Remote/Git.hs +++ b/Remote/Git.hs @@ -339,7 +339,12 @@ tryGitConfigRead autoinit r hasuuid warning $ UnquotedString $ "Remote " ++ Git.repoDescribe r ++ ": " ++ show e Annex.getState Annex.repo - s <- newLocal r + {- Since the path to the repository was specified + - explicitly, CVE-2022-24765 is not a concern, + - so tell git to treat the repository directory as safe. + -} + let r' = r { Git.safeDirectory = True } + s <- newLocal r' liftIO $ Annex.eval s $ check `finally` quiesce True diff --git a/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated.mdwn b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated.mdwn index fee437f035..07c8eae8cb 100644 --- a/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated.mdwn +++ b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated.mdwn @@ -42,3 +42,5 @@ so, ideally `git annex enableremote` should provide a similar diagnostic output [[!tag projects/dandi]] ``` + +> [[fixed|done]] --[[Joey]] diff --git a/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_1_c23de4c4ff536a214c795a805957c9f1._comment b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_1_c23de4c4ff536a214c795a805957c9f1._comment new file mode 100644 index 0000000000..a19d556fee --- /dev/null +++ b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_1_c23de4c4ff536a214c795a805957c9f1._comment @@ -0,0 +1,24 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 1""" + date="2023-09-07T17:01:41Z" + content=""" +I wonder if it even makes sense for git-annex-shell to replicate this git +security check, or would it be better for it to instruct git to trust the +repository, so it can be used on it? + +git's CVE-2022-24765 involves a malicious creation of a .git repository +above the victim's cwd, with a .git/config that causes things like eg shell +prompts that run git to execute attacker-controlled commands. + +git-annex-shell commands all take the directory that the repository is +in, and uses that repository. So it doesn't traverse above looking for +other .git directories. + +And, `git clone` will happily clone a remote repsository that's owned +by another user, including over ssh. And pull and push etc work with such a +remote. So git-annex-shell should too. + +(For that matter, other git-annex-shell commands do work, it's only the +command that reads the git config that fails to work.) +"""]] diff --git a/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_2_c75d3a033b1b8699e512df4641a38548._comment b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_2_c75d3a033b1b8699e512df4641a38548._comment new file mode 100644 index 0000000000..2bb5be7ff9 --- /dev/null +++ b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_2_c75d3a033b1b8699e512df4641a38548._comment @@ -0,0 +1,13 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 2""" + date="2023-09-07T18:21:30Z" + content=""" +Closely related, when a local repo is owned by someone else, cloning it and +using it as a git-annex remote also fails, at the same config listing +stage. + +I think the same reasoning applies to that, the path to the repo is +explicitly specified in the remote url, so it should treat it as a safe +repo for the purposes of listing its config. +"""]] diff --git a/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_3_39e13618336181485175dddc7185160a._comment b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_3_39e13618336181485175dddc7185160a._comment new file mode 100644 index 0000000000..ce89951483 --- /dev/null +++ b/doc/bugs/enableremote_fails_with___34__wrong_reason__34___stated/comment_3_39e13618336181485175dddc7185160a._comment @@ -0,0 +1,8 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 3""" + date="2023-09-07T18:32:57Z" + content=""" +Basically the same fix works for both the ssh remote and the local +remote cases. +"""]]