better transport argument sanitization

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

@@ -48,6 +48,13 @@ pub struct Url {
     /// The port to use when connecting to a host. If `None`, standard ports depending on `scheme` will be used.
     pub port: Option<u16>,
     /// The path portion of the URL, usually the location of the git repository.
+    ///
+    /// # 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.
+    ///
+    /// If this value is going to be used in a command-line application, call [Self::path_argument_safe()] instead.
     pub path: bstr::BString,
 }
 
@@ -123,9 +130,34 @@ impl Url {
         self.password.as_deref()
     }
     /// Returns the host mentioned in the url, if present.
+    ///
+    /// # 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.
+    ///
+    /// 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> {
         self.host.as_deref()
     }
+
+    /// Return the host of this URL if present *and* if it can't be mistaken for a command-line argument.
+    ///
+    /// Use this method if the host is going to be passed to a command-line application.
+    pub fn host_argument_safe(&self) -> Option<&str> {
+        self.host().filter(|host| !looks_like_argument(host.as_bytes()))
+    }
+
+    /// Return the path of this URL *and* if it can't be mistaken for a command-line argument.
+    /// Note that it always begins with a slash, which is ignored for this comparison.
+    ///
+    /// Use this method if the path is going to be passed to a command-line application.
+    pub fn path_argument_safe(&self) -> Option<&BStr> {
+        self.path
+            .get(1..)
+            .and_then(|truncated| (!looks_like_argument(truncated)).then_some(self.path.as_ref()))
+    }
+
     /// Returns true if the path portion of the url is `/`.
     pub fn path_is_root(&self) -> bool {
         self.path == "/"
@@ -146,6 +178,10 @@ impl Url {
     }
 }
 
+fn looks_like_argument(b: &[u8]) -> bool {
+    b.first() == Some(&b'-')
+}
+
 /// Transformation
 impl Url {
     /// Turn a file url like `file://relative` into `file:///root/relative`, hence it assures the url's path component is absolute, using

gix-url/tests/access/mod.rs

@@ -29,3 +29,23 @@ mod canonicalized {
         Ok(())
     }
 }
+
+#[test]
+fn host_argument_safe() -> crate::Result {
+    let url = gix_url::parse("ssh://-oProxyCommand=open$IFS-aCalculator/foo".into())?;
+    assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator"));
+    assert_eq!(url.host_argument_safe(), None);
+    assert_eq!(url.path, "/foo");
+    assert_eq!(url.path_argument_safe(), Some("/foo".into()));
+    Ok(())
+}
+
+#[test]
+fn path_argument_safe() -> crate::Result {
+    let url = gix_url::parse("ssh://foo/-oProxyCommand=open$IFS-aCalculator".into())?;
+    assert_eq!(url.host(), Some("foo"));
+    assert_eq!(url.host_argument_safe(), Some("foo"));
+    assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator");
+    assert_eq!(url.path_argument_safe(), None);
+    Ok(())
+}

tests/journey/gix.sh

@@ -345,6 +345,12 @@ title "gix commit-graph"
               }
             )
           )
+          (with "an ambiguous ssh host which could be mistaken for an argument"
+              it "fails without trying to pass it to command-line programs" && {
+                WITH_SNAPSHOT="$snapshot/fail-ambigous-host" \
+                expect_run $WITH_FAILURE "$exe_plumbing" free pack receive 'ssh://-oProxyCommand=open$IFS-aCalculator/foo'
+              }
+          )
           fi
         )
         elif [[ "$kind" = "small" ]]; then
@@ -355,6 +361,17 @@ title "gix commit-graph"
         fi
       )
     )
+    if test "$kind" = "max" || test "$kind" = "max-pure"; then
+    (with "the 'clone' sub-command"
+        snapshot="$snapshot/clone"
+        (with "an ambiguous ssh host which could be mistaken for an argument"
+            it "fails without trying to pass it to command-line programs" && {
+              WITH_SNAPSHOT="$snapshot/fail-ambigous-host" \
+              expect_run $WITH_FAILURE "$exe_plumbing" clone 'ssh://-oProxyCommand=open$IFS-aCalculator/foo'
+            }
+        )
+    )
+    fi
     (with "the 'index' sub-command"
       snapshot="$snapshot/index"
       title "gix free pack index create"

tests/snapshots/plumbing/no-repo/pack/clone/fail-ambigous-host

@@ -0,0 +1 @@
+Error: Host name '-oProxyCommand=open$IFS-aCalculator' could be mistaken for a command-line argument

tests/snapshots/plumbing/no-repo/pack/receive/fail-ambigous-host

@@ -0,0 +1 @@
+Error: Host name '-oProxyCommand=open$IFS-aCalculator' could be mistaken for a command-line argument