fix: prevent hosts or paths that look like arguments to be passed to invoked commands.

See https://secure.phabricator.com/T12961 for more details.

Author: Byron

gix-transport/src/client/blocking_io/file.rs

@@ -211,6 +211,11 @@ impl client::Transport for SpawnProcessOnDemand {
         };
         cmd.stdin = Stdio::piped();
         cmd.stdout = Stdio::piped();
+        if self.path.first() == Some(&b'-') {
+            return Err(client::Error::AmbiguousPath {
+                path: self.path.clone(),
+            });
+        }
         let repo_path = if self.ssh_cmd.is_some() {
             cmd.args.push(service.as_str().into());
             gix_quote::single(self.path.as_ref()).to_os_str_lossy().into_owned()
@@ -225,6 +230,7 @@ impl client::Transport for SpawnProcessOnDemand {
         }
         cmd.envs(std::mem::take(&mut self.envs));
 
+        gix_features::trace::debug!(command = ?cmd, "gix_transport::SpawnProcessOnDemand");
         let mut child = cmd.spawn().map_err(|err| client::Error::InvokeProgram {
             source: err,
             command: cmd_name.into_owned(),

gix-transport/src/client/blocking_io/ssh/mod.rs

@@ -8,6 +8,8 @@ use crate::{client::blocking_io, Protocol};
 pub enum Error {
     #[error("The scheme in \"{}\" is not usable for an ssh connection", .0.to_bstring())]
     UnsupportedScheme(gix_url::Url),
+    #[error("Host name '{host}' could be mistaken for a command-line argument")]
+    AmbiguousHostName { host: String },
 }
 
 impl crate::IsSpuriousError for Error {}
@@ -37,12 +39,17 @@ pub mod invocation {
 
     /// The error returned when producing ssh invocation arguments based on a selected invocation kind.
     #[derive(Debug, thiserror::Error)]
-    #[error("The 'Simple' ssh variant doesn't support {function}")]
-    pub struct Error {
-        /// The simple command that should have been invoked.
-        pub command: OsString,
-        /// The function that was unsupported
-        pub function: &'static str,
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error("Host name '{host}' could be mistaken for a command-line argument")]
+        AmbiguousHostName { host: String },
+        #[error("The 'Simple' ssh variant doesn't support {function}")]
+        Unsupported {
+            /// The simple command that should have been invoked.
+            command: OsString,
+            /// The function that was unsupported
+            function: &'static str,
+        },
     }
 }
 
@@ -105,7 +112,9 @@ pub fn connect(
                 .stdin(Stdio::null())
                 .with_shell()
                 .arg("-G")
-                .arg(url.host().expect("always set for ssh urls")),
+                .arg(url.host_argument_safe().ok_or_else(|| Error::AmbiguousHostName {
+                    host: url.host().expect("set in ssh urls").into(),
+                })?),
         )
         .status()
         .ok()

gix-transport/src/client/blocking_io/ssh/program_kind.rs

@@ -31,7 +31,6 @@ impl ProgramKind {
         if disallow_shell {
             prepare.use_shell = false;
         }
-        let host = url.host().expect("present in ssh urls");
         match self {
             ProgramKind::Ssh => {
                 if desired_version != Protocol::V1 {
@@ -54,16 +53,26 @@ impl ProgramKind {
             }
             ProgramKind::Simple => {
                 if url.port.is_some() {
-                    return Err(ssh::invocation::Error {
+                    return Err(ssh::invocation::Error::Unsupported {
                         command: ssh_cmd.into(),
                         function: "setting the port",
                     });
                 }
             }
         };
         let host_as_ssh_arg = match url.user() {
-            Some(user) => format!("{user}@{host}"),
-            None => host.into(),
+            Some(user) => {
+                let host = url.host().expect("present in ssh urls");
+                format!("{user}@{host}")
+            }
+            None => {
+                let host = url
+                    .host_argument_safe()
+                    .ok_or_else(|| ssh::invocation::Error::AmbiguousHostName {
+                        host: url.host().expect("ssh host always set").into(),
+                    })?;
+                host.into()
+            }
         };
 
         // Try to force ssh to yield english messages (for parsing later)

gix-transport/src/client/blocking_io/ssh/tests.rs

@@ -144,13 +144,28 @@ mod program_kind {
                 assert!(call_args(kind, "ssh://user@host:43/p", Protocol::V2).ends_with("-P 43 user@host"));
             }
         }
+        #[test]
+        fn ambiguous_host_is_allowed_with_user() {
+            assert_eq!(
+                call_args(ProgramKind::Ssh, "ssh://user@-arg/p", Protocol::V2),
+                joined(&["ssh", "-o", "SendEnv=GIT_PROTOCOL", "user@-arg"])
+            );
+        }
+
+        #[test]
+        fn ambiguous_host_is_disallowed() {
+            assert!(matches!(
+                try_call(ProgramKind::Ssh, "ssh://-arg/p", Protocol::V2),
+                Err(ssh::invocation::Error::AmbiguousHostName { host }) if host == "-arg"
+            ));
+        }
 
         #[test]
         fn simple_cannot_handle_any_arguments() {
-            match try_call(ProgramKind::Simple, "ssh://user@host:42/p", Protocol::V2) {
-                Err(ssh::invocation::Error { .. }) => {}
-                _ => panic!("BUG: unexpected outcome"),
-            }
+            assert!(matches!(
+                try_call(ProgramKind::Simple, "ssh://user@host:42/p", Protocol::V2),
+                Err(ssh::invocation::Error::Unsupported { .. })
+            ));
             assert_eq!(
                 call_args(ProgramKind::Simple, "ssh://user@host/p", Protocol::V2),
                 joined(&["simple", "user@host"]),

gix-transport/src/client/git/mod.rs

@@ -165,6 +165,21 @@ mod message {
                 "git-upload-pack hello\\world\0host=host:404\0"
             )
         }
+
+        #[test]
+        fn with_strange_host_and_port() {
+            assert_eq!(
+                git::message::connect(
+                    Service::UploadPack,
+                    Protocol::V1,
+                    b"--upload-pack=attack",
+                    Some(&("--proxy=other-attack".into(), Some(404))),
+                    &[]
+                ),
+                "git-upload-pack --upload-pack=attack\0host=--proxy=other-attack:404\0",
+                "we explicitly allow possible `-arg` arguments to be passed to the git daemon - the remote must protect against exploitation, we don't want to prevent legitimate cases"
+            )
+        }
     }
 }
 

gix-transport/src/client/non_io_types.rs

@@ -138,6 +138,8 @@ mod error {
         Http(#[from] HttpError),
         #[error(transparent)]
         SshInvocation(SshInvocationError),
+        #[error("The repository path '{path}' could be mistaken for a command-line argument")]
+        AmbiguousPath { path: BString },
     }
 
     impl crate::IsSpuriousError for Error {

gix-transport/src/lib.rs

@@ -21,7 +21,6 @@ pub use gix_packetline as packetline;
 /// The version of the way client and server communicate.
 #[derive(Default, PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-#[allow(missing_docs)]
 pub enum Protocol {
     /// Version 0 is like V1, but doesn't show capabilities at all, at least when hosted without `git-daemon`.
     V0 = 0,

gix-url/src/lib.rs

@@ -52,7 +52,7 @@ pub struct Url {
     /// # Security-Warning
     ///
     /// URLs allow paths to start with `-` which makes it possible to mask command-line arguments as path which then leads to
-    /// the invocation of programs from an attacker controlled URL. See https://secure.phabricator.com/T12961 for details.
+    /// the invocation of programs from an attacker controlled URL. See <https://secure.phabricator.com/T12961> for details.
     ///
     /// If this value is going to be used in a command-line application, call [Self::path_argument_safe()] instead.
     pub path: bstr::BString,
@@ -134,7 +134,7 @@ impl Url {
     /// # Security-Warning
     ///
     /// URLs allow hosts to start with `-` which makes it possible to mask command-line arguments as host which then leads to
-    /// the invocation of programs from an attacker controlled URL. See https://secure.phabricator.com/T12961 for details.
+    /// the invocation of programs from an attacker controlled URL. See <https://secure.phabricator.com/T12961> for details.
     ///
     /// If this value is going to be used in a command-line application, call [Self::host_argument_safe()] instead.
     pub fn host(&self) -> Option<&str> {
@@ -179,7 +179,7 @@ impl Url {
 }
 
 fn looks_like_argument(b: &[u8]) -> bool {
-    b.get(0) == Some(&b'-')
+    b.first() == Some(&b'-')
 }
 
 /// Transformation