--- Begin Message ---
----- Forwarded message from Joey Hess <i...@joeyh.name> -----
Date: Thu, 17 Aug 2017 22:42:27 -0400
From: Joey Hess <i...@joeyh.name>
To: Richard Hartmann <ric...@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 jo
From cb521ac529f7072ed94d5cece78a098eac1aa715 Mon Sep 17 00:00:00 2001
From: Joey Hess <jo...@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 <i...@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 <i...@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.3
signature.asc
Description: PGP signature
--- End Message ---
