Hi again,
I reached out to joeyh to see how we could backport git-annex security
patches to wheezy. He responded by sharing the attached patch he sent to
the git-annex maintainer that backports the fixes to stretch. I figured
it would be useful for the core secteam to have visibilty on this...
He also validated the approach i suggested of "grep for ssh and backport
the SshHost construct" to fix the issue in earlier version.
I may look at this again tomorrow, otherwise next week.
A.
--
Celui qui sait jouir du peu qu'il a est toujours assez riche.
- Démocrite
--- Begin Message ---
- To: Antoine Beaupré <anarcat@debian.org>
- Subject: FWD: heads up: git-annex security hole
- From: Joey Hess <id@joeyh.name>
- Date: Thu, 28 Sep 2017 12:31:34 -0400
- Message-id: <20170928163134.zdryh35vih3vvxqb@kitenet.net>
----- Forwarded message from Joey Hess <id@joeyh.name> ----- Date: Thu, 17 Aug 2017 22:42:27 -0400 From: Joey Hess <id@joeyh.name> To: Richard Hartmann <richih@debian.org> Subject: heads up: git-annex security hole User-Agent: NeoMutt/20170609 (1.8.3) I'll be releasing a new version of git-annex tomorrow fixing a remotely exploitable security hole, the same class of vulnerability that recently afflicted git. Patch is attached. This affects all versions of git-annex, so will need backporting. I've also attached a version of the patch that will apply cleanly to 6.20170101 in stable. -- see shy jo ----- End forwarded message ----- -- see shy joFrom cb521ac529f7072ed94d5cece78a098eac1aa715 Mon Sep 17 00:00:00 2001 From: Joey Hess <joeyh@joeyh.name> Date: Thu, 17 Aug 2017 22:39:23 -0400 Subject: [PATCH] (stable) avoid the dashed ssh hostname class of security holes Security fix: Disallow hostname starting with a dash, which would get passed to ssh and be treated an option. This could be used by an attacker who provides a crafted ssh url (for eg a git remote) to execute arbitrary code via ssh -oProxyCommand. No CVE has yet been assigned for this hole. The same class of security hole recently affected git itself, CVE-2017-1000117. Method: Identified all places where ssh is run, by git grep '"ssh"' Converted them all to use a SshHost, if they did not already, for specifying the hostname. SshHost was made a data type with a smart constructor, which rejects hostnames starting with '-'. Note that git-annex already contains extensive use of Utility.SafeCommand, which fixes a similar class of problem where a filename starting with a dash gets passed to a program which treats it as an option. --- Annex/Ssh.hs | 15 +++++++----- Assistant/Pairing/MakeRemote.hs | 4 ++-- Assistant/Ssh.hs | 11 +++++---- Assistant/WebApp/Configurators/Ssh.hs | 44 +++++++++++++++++------------------ CHANGELOG | 10 ++++++++ Remote/Ddar.hs | 10 ++++---- Remote/GCrypt.hs | 6 +++-- Remote/Helper/Ssh.hs | 8 +++++-- Remote/Rsync.hs | 4 +++- Utility/SshHost.hs | 29 +++++++++++++++++++++++ git-annex.cabal | 1 + 11 files changed, 98 insertions(+), 44 deletions(-) create mode 100644 Utility/SshHost.hs diff --git a/Annex/Ssh.hs b/Annex/Ssh.hs index 512f0375c..6bd1eeb32 100644 --- a/Annex/Ssh.hs +++ b/Annex/Ssh.hs @@ -34,6 +34,7 @@ import Config import Annex.Path import Utility.Env import Utility.FileSystemEncoding +import Utility.SshHost import Types.CleanupActions import Git.Env #ifndef mingw32_HOST_OS @@ -43,7 +44,7 @@ import Annex.LockPool {- Generates parameters to ssh to a given host (or user@host) on a given - port. This includes connection caching parameters, and any ssh-options. -} -sshOptions :: (String, Maybe Integer) -> RemoteGitConfig -> [CommandParam] -> Annex [CommandParam] +sshOptions :: (SshHost, Maybe Integer) -> RemoteGitConfig -> [CommandParam] -> Annex [CommandParam] sshOptions (host, port) gc opts = go =<< sshCachingInfo (host, port) where go (Nothing, params) = ret params @@ -60,7 +61,7 @@ sshOptions (host, port) gc opts = go =<< sshCachingInfo (host, port) {- Returns a filename to use for a ssh connection caching socket, and - parameters to enable ssh connection caching. -} -sshCachingInfo :: (String, Maybe Integer) -> Annex (Maybe FilePath, [CommandParam]) +sshCachingInfo :: (SshHost, Maybe Integer) -> Annex (Maybe FilePath, [CommandParam]) sshCachingInfo (host, port) = go =<< sshCacheDir where go Nothing = return (Nothing, []) @@ -201,9 +202,10 @@ forceStopSsh socketfile = do - of the path to a socket file. At the same time, it needs to be unique - for each host. -} -hostport2socket :: String -> Maybe Integer -> FilePath -hostport2socket host Nothing = hostport2socket' host -hostport2socket host (Just port) = hostport2socket' $ host ++ "!" ++ show port +hostport2socket :: SshHost -> Maybe Integer -> FilePath +hostport2socket host Nothing = hostport2socket' $ fromSshHost host +hostport2socket host (Just port) = hostport2socket' $ + fromSshHost host ++ "!" ++ show port hostport2socket' :: String -> FilePath hostport2socket' s | length s > lengthofmd5s = md5s (Str s) @@ -282,7 +284,8 @@ sshOptionsTo remote gc localr | otherwise = case Git.Url.hostuser remote of Nothing -> unchanged Just host -> do - (msockfile, _) <- sshCachingInfo (host, Git.Url.port remote) + let sshhost = either error id (mkSshHost host) + (msockfile, _) <- sshCachingInfo (sshhost, Git.Url.port remote) case msockfile of Nothing -> use [] Just sockfile -> do diff --git a/Assistant/Pairing/MakeRemote.hs b/Assistant/Pairing/MakeRemote.hs index e847edd39..a97bb31f0 100644 --- a/Assistant/Pairing/MakeRemote.hs +++ b/Assistant/Pairing/MakeRemote.hs @@ -42,9 +42,9 @@ finishedLocalPairing msg keypair = do [ sshOpt "StrictHostKeyChecking" "no" , sshOpt "NumberOfPasswordPrompts" "0" , "-n" - , genSshHost (sshHostName sshdata) (sshUserName sshdata) - , "git-annex-shell -c configlist " ++ T.unpack (sshDirectory sshdata) ] + (genSshHost (sshHostName sshdata) (sshUserName sshdata)) + ("git-annex-shell -c configlist " ++ T.unpack (sshDirectory sshdata)) Nothing r <- liftAnnex $ addRemote $ makeSshRemote sshdata liftAnnex $ setRemoteCost (Remote.repo r) semiExpensiveRemoteCost diff --git a/Assistant/Ssh.hs b/Assistant/Ssh.hs index 66ed54257..6712959c4 100644 --- a/Assistant/Ssh.hs +++ b/Assistant/Ssh.hs @@ -14,6 +14,7 @@ import Utility.Rsync import Utility.FileMode import Utility.SshConfig import Git.Remote +import Utility.SshHost import Data.Text (Text) import qualified Data.Text as T @@ -64,8 +65,9 @@ sshOpt :: String -> String -> String sshOpt k v = concat ["-o", k, "=", v] {- user@host or host -} -genSshHost :: Text -> Maybe Text -> String -genSshHost host user = maybe "" (\v -> T.unpack v ++ "@") user ++ T.unpack host +genSshHost :: Text -> Maybe Text -> SshHost +genSshHost host user = either error id $ mkSshHost $ + maybe "" (\v -> T.unpack v ++ "@") user ++ T.unpack host {- Generates a ssh or rsync url from a SshData. -} genSshUrl :: SshData -> String @@ -119,8 +121,9 @@ genSshRepoName host dir | otherwise = makeLegalName $ host ++ "_" ++ dir {- The output of ssh, including both stdout and stderr. -} -sshTranscript :: [String] -> (Maybe String) -> IO (String, Bool) -sshTranscript opts input = processTranscript "ssh" opts input +sshTranscript :: [String] -> SshHost -> String -> (Maybe String) -> IO (String, Bool) +sshTranscript opts sshhost cmd input = processTranscript "ssh" + (opts ++ [fromSshHost sshhost, cmd]) input {- Ensure that the ssh public key doesn't include any ssh options, like - command=foo, or other weirdness. diff --git a/Assistant/WebApp/Configurators/Ssh.hs b/Assistant/WebApp/Configurators/Ssh.hs index 950290249..e836af406 100644 --- a/Assistant/WebApp/Configurators/Ssh.hs +++ b/Assistant/WebApp/Configurators/Ssh.hs @@ -39,6 +39,7 @@ import Utility.Tmp import Utility.FileMode import Utility.ThreadScheduler import Utility.Env +import Utility.SshHost import qualified Data.Text as T import qualified Data.Map as M @@ -299,12 +300,11 @@ testServer sshinput@(SshInput { inputHostname = Just hn }) = do if knownhost then "yes" else "no" , "-n" -- don't read from stdin , "-p", show (inputPort sshinput) - , genSshHost - (fromJust $ inputHostname sshinput) - (inputUsername sshinput) - , remotecommand ] - parsetranscript . fst <$> sshAuthTranscript sshinput sshopts Nothing + let sshhost = genSshHost + (fromJust $ inputHostname sshinput) + (inputUsername sshinput) + parsetranscript . fst <$> sshAuthTranscript sshinput sshopts sshhost remotecommand Nothing parsetranscript s = let cs = map snd $ filter (reported . fst) [ ("git-annex-shell", GitAnnexShellCapable) @@ -339,9 +339,9 @@ testServer sshinput@(SshInput { inputHostname = Just hn }) = do {- Runs a ssh command to set up the repository; if it fails shows - the user the transcript, and if it succeeds, runs an action. -} -sshSetup :: SshInput -> [String] -> Maybe String -> Handler Html -> Handler Html -sshSetup sshinput opts input a = do - (transcript, ok) <- liftAssistant $ sshAuthTranscript sshinput opts input +sshSetup :: SshInput -> [String] -> SshHost -> String -> Maybe String -> Handler Html -> Handler Html +sshSetup sshinput opts sshhost cmd input a = do + (transcript, ok) <- liftAssistant $ sshAuthTranscript sshinput opts sshhost cmd input if ok then do liftAssistant $ expireCachedCred $ getLogin sshinput @@ -367,8 +367,8 @@ sshErr sshinput msg - cached password. ssh is coaxed to use git-annex as SSH_ASKPASS - to get the password. -} -sshAuthTranscript :: SshInput -> [String] -> (Maybe String) -> Assistant (String, Bool) -sshAuthTranscript sshinput opts input = case inputAuthMethod sshinput of +sshAuthTranscript :: SshInput -> [String] -> SshHost -> String -> (Maybe String) -> Assistant (String, Bool) +sshAuthTranscript sshinput opts sshhost cmd input = case inputAuthMethod sshinput of ExistingSshKey -> liftIO $ go [passwordprompts 0] Nothing CachedPassword -> setupAskPass Password -> do @@ -379,7 +379,7 @@ sshAuthTranscript sshinput opts input = case inputAuthMethod sshinput of geti f = maybe "" T.unpack (f sshinput) go extraopts environ = processTranscript' - (askPass environ) "ssh" (extraopts ++ opts) + (askPass environ) "ssh" (extraopts ++ opts ++ [fromSshHost sshhost, cmd]) -- Always provide stdin, even when empty. (Just (fromMaybe "" input)) @@ -521,10 +521,11 @@ prepSsh' needsinit origsshdata sshdata keypair a ] a sshdata | otherwise = sshSetup (mkSshInput origsshdata) - [ "-p", show (sshPort origsshdata) - , genSshHost (sshHostName origsshdata) (sshUserName origsshdata) - , remoteCommand - ] Nothing (a sshdata) + [ "-p", show (sshPort origsshdata) + ] + (genSshHost (sshHostName origsshdata) (sshUserName origsshdata)) + remoteCommand + Nothing (a sshdata) where remotedir = T.unpack $ sshDirectory sshdata remoteCommand = shellWrap $ intercalate "&&" $ catMaybes @@ -625,7 +626,7 @@ getMakeRsyncNetGCryptR :: SshData -> RepoKey -> Handler Html getMakeRsyncNetGCryptR sshdata NoRepoKey = whenGcryptInstalled $ withNewSecretKey $ getMakeRsyncNetGCryptR sshdata . RepoKey getMakeRsyncNetGCryptR sshdata (RepoKey keyid) = whenGcryptInstalled $ - sshSetup (mkSshInput sshdata) [sshhost, gitinit] Nothing $ + sshSetup (mkSshInput sshdata) [] sshhost gitinit Nothing $ makeGCryptRepo NewRepo keyid sshdata where sshhost = genSshHost (sshHostName sshdata) (sshUserName sshdata) @@ -661,11 +662,9 @@ prepRsyncNet sshinput reponame a = do , sshCapabilities = [RsyncCapable] } let sshhost = genSshHost (sshHostName sshdata) (sshUserName sshdata) - let torsyncnet cmd = filter (not . null) - [ if knownhost then "" else sshOpt "StrictHostKeyChecking" "no" - , sshhost - , cmd - ] + let torsyncnet + | knownhost = [] + | otherwise = [sshOpt "StrictHostKeyChecking" "no"] {- I'd prefer to separate commands with && , but - rsync.net's shell does not support that. -} let remotecommand = intercalate ";" @@ -674,7 +673,8 @@ prepRsyncNet sshinput reponame a = do , "dd of=.ssh/authorized_keys oflag=append conv=notrunc" , "mkdir -p " ++ T.unpack (sshDirectory sshdata) ] - sshSetup sshinput (torsyncnet remotecommand) (Just $ sshPubKey keypair) (a sshdata) + sshSetup sshinput torsyncnet sshhost remotecommand + (Just $ sshPubKey keypair) (a sshdata) isRsyncNet :: Maybe Text -> Bool isRsyncNet Nothing = False diff --git a/CHANGELOG b/CHANGELOG index 07dd3d84e..aa80d70dd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,13 @@ +git-annex (6.20170101.1) unstable; urgency=medium + + * Security fix: Disallow hostname starting with a dash, which + would get passed to ssh and be treated an option. This could + be used by an attacker who provides a crafted ssh url to execute + arbitrary code via -oProxyCommand. + (The same class of security hole recently affected git itself.) + + -- Joey Hess <id@joeyh.name> Thu, 17 Aug 2017 22:18:24 -0400 + git-annex (6.20170101) unstable; urgency=medium * XMPP support has been removed from the assistant in this release. diff --git a/Remote/Ddar.hs b/Remote/Ddar.hs index dcb16f5dd..e03858416 100644 --- a/Remote/Ddar.hs +++ b/Remote/Ddar.hs @@ -21,6 +21,7 @@ import Config.Cost import Remote.Helper.Special import Annex.Ssh import Annex.UUID +import Utility.SshHost data DdarRepo = DdarRepo { ddarRepoConfig :: RemoteGitConfig @@ -109,9 +110,8 @@ store ddarrepo = fileStorer $ \k src _p -> do liftIO $ boolSystem "ddar" params {- Convert remote DdarRepo to host and path on remote end -} -splitRemoteDdarRepo :: DdarRepo -> (String, String) -splitRemoteDdarRepo ddarrepo = - (host, ddarrepo') +splitRemoteDdarRepo :: DdarRepo -> (SshHost, String) +splitRemoteDdarRepo ddarrepo = (either error id $ mkSshHost host, ddarrepo') where (host, remainder) = span (/= ':') (ddarRepoLocation ddarrepo) ddarrepo' = drop 1 remainder @@ -127,7 +127,7 @@ ddarRemoteCall ddarrepo cmd params where (host, ddarrepo') = splitRemoteDdarRepo ddarrepo localParams = Param [cmd] : Param (ddarRepoLocation ddarrepo) : params - remoteParams = Param host : Param "ddar" : Param [cmd] : Param ddarrepo' : params + remoteParams = Param (fromSshHost host) : Param "ddar" : Param [cmd] : Param ddarrepo' : params {- Specialized ddarRemoteCall that includes extraction command and flags -} ddarExtractRemoteCall :: DdarRepo -> Key -> Annex (String, [CommandParam]) @@ -169,7 +169,7 @@ ddarDirectoryExists ddarrepo where (host, ddarrepo') = splitRemoteDdarRepo ddarrepo params = - [ Param host + [ Param (fromSshHost host) , Param "test" , Param "-d" , Param ddarrepo' diff --git a/Remote/GCrypt.hs b/Remote/GCrypt.hs index 78ab6ed79..06dd026bb 100644 --- a/Remote/GCrypt.hs +++ b/Remote/GCrypt.hs @@ -49,6 +49,7 @@ import Utility.Rsync import Utility.Tmp import Logs.Remote import Utility.Gpg +import Utility.SshHost remote :: RemoteType remote = RemoteType { @@ -159,8 +160,9 @@ rsyncTransport r gc let rsyncpath = if "/~/" `isPrefixOf` path then drop 3 path else path - opts <- sshOptions (host, Nothing) gc [] - return (rsyncShell $ Param "ssh" : opts, host ++ ":" ++ rsyncpath, AccessShell) + let sshhost = either error id (mkSshHost host) + opts <- sshOptions (sshhost, Nothing) gc [] + return (rsyncShell $ Param "ssh" : opts, fromSshHost sshhost ++ ":" ++ rsyncpath, AccessShell) othertransport = return ([], loc, AccessDirect) noCrypto :: Annex a diff --git a/Remote/Helper/Ssh.hs b/Remote/Helper/Ssh.hs index dff16b656..7f8a17b05 100644 --- a/Remote/Helper/Ssh.hs +++ b/Remote/Helper/Ssh.hs @@ -19,6 +19,7 @@ import Remote.Helper.Messages import Messages.Progress import Utility.Metered import Utility.Rsync +import Utility.SshHost import Types.Remote import Types.Transfer import Config @@ -29,9 +30,12 @@ import Config toRepo :: Git.Repo -> RemoteGitConfig -> [CommandParam] -> Annex [CommandParam] toRepo r gc sshcmd = do let opts = map Param $ remoteAnnexSshOptions gc - let host = fromMaybe (giveup "bad ssh url") $ Git.Url.hostuser r + let host = maybe + (giveup "bad ssh url") + (either error id . mkSshHost) + (Git.Url.hostuser r) params <- sshOptions (host, Git.Url.port r) gc opts - return $ params ++ Param host : sshcmd + return $ params ++ Param (fromSshHost host) : sshcmd {- Generates parameters to run a git-annex-shell command on a remote - repository. -} diff --git a/Remote/Rsync.hs b/Remote/Rsync.hs index 22ef0b2cf..e05feeff9 100644 --- a/Remote/Rsync.hs +++ b/Remote/Rsync.hs @@ -38,6 +38,7 @@ import Types.Transfer import Types.Creds import Annex.DirHashes import Utility.Tmp +import Utility.SshHost import qualified Data.Map as M @@ -120,7 +121,8 @@ rsyncTransport gc url case fromNull ["ssh"] (remoteAnnexRsyncTransport gc) of "ssh":sshopts -> do let (port, sshopts') = sshReadPort sshopts - userhost = takeWhile (/=':') url + userhost = either error id $ mkSshHost $ + takeWhile (/=':') url -- Connection caching (Param "ssh":) <$> sshOptions (userhost, port) gc diff --git a/Utility/SshHost.hs b/Utility/SshHost.hs new file mode 100644 index 000000000..d8a8da11d --- /dev/null +++ b/Utility/SshHost.hs @@ -0,0 +1,29 @@ +{- ssh hostname sanitization + - + - When constructing a ssh command with a hostname that may be controlled + - by an attacker, prevent the hostname from starting with "-", + - to prevent tricking ssh into arbitrary command execution via + - eg "-oProxyCommand=" + - + - Copyright 2017 Joey Hess <id@joeyh.name> + - + - License: BSD-2-clause + -} + +module Utility.SshHost (SshHost, mkSshHost, fromSshHost) where + +newtype SshHost = SshHost String + +-- | Smart constructor for a legal hostname or IP address. +-- In some cases, it may be prefixed with "user@" to specify the remote +-- user at the host. +-- +-- For now, we only filter out the problem ones, because determining an +-- actually legal hostnames is quite complicated. +mkSshHost :: String -> Either String SshHost +mkSshHost h@('-':_) = Left $ + "rejecting ssh hostname that starts with '-' : " ++ h +mkSshHost h = Right (SshHost h) + +fromSshHost :: SshHost -> String +fromSshHost (SshHost h) = h diff --git a/git-annex.cabal b/git-annex.cabal index 5b95a16b7..39881ce68 100644 --- a/git-annex.cabal +++ b/git-annex.cabal @@ -1047,6 +1047,7 @@ Executable git-annex Utility.Shell Utility.SimpleProtocol Utility.SshConfig + Utility.SshHost Utility.Su Utility.SystemDirectory Utility.TList -- 2.13.3Attachment: signature.asc
Description: PGP signature
--- End Message ---