Refine and strengthen diagnostics for problematic URLs

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

@@ -291,7 +291,7 @@ pub fn connect(
 mod tests {
     mod ssh {
         mod connect {
-            use crate::{client::blocking_io::ssh::connect, Protocol};
+            use crate::{client::blocking_io::ssh, Protocol};
 
             #[test]
             fn path() {
@@ -304,10 +304,29 @@ mod tests {
                     ("[email protected]:~/repo", "~/repo"),
                 ] {
                     let url = gix_url::parse((*url).into()).expect("valid url");
-                    let cmd = connect(url, Protocol::V1, Default::default(), false).expect("parse success");
+                    let cmd = ssh::connect(url, Protocol::V1, Default::default(), false).expect("parse success");
                     assert_eq!(cmd.path, expected, "the path will be substituted by the remote shell");
                 }
             }
+
+            #[test]
+            fn ambiguous_host_disallowed() {
+                for url in [
+                    "ssh://-oProxyCommand=open$IFS-aCalculator/foo",
+                    "user@-oProxyCommand=open$IFS-aCalculator:username/repo",
+                ] {
+                    let url = gix_url::parse((*url).into()).expect("valid url");
+                    let options = ssh::connect::Options {
+                        command: Some("unrecognized".into()),
+                        disallow_shell: false,
+                        kind: None,
+                    };
+                    assert!(matches!(
+                        ssh::connect(url, Protocol::V1, options, false),
+                        Err(ssh::Error::AmbiguousHostName { host }) if host == "-oProxyCommand=open$IFS-aCalculator",
+                    ));
+                }
+            }
         }
     }
 }

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

@@ -1,5 +1,7 @@
 use std::process::Stdio;
 
+use gix_url::ArgumentSafety::*;
+
 use crate::{client::blocking_io, Protocol};
 
 /// The error used in [`connect()`].
@@ -42,6 +44,8 @@ pub mod invocation {
     #[derive(Debug, thiserror::Error)]
     #[allow(missing_docs)]
     pub enum Error {
+        #[error("Username '{user}' could be mistaken for a command-line argument")]
+        AmbiguousUserName { user: String },
         #[error("Host name '{host}' could be mistaken for a command-line argument")]
         AmbiguousHostName { host: String },
         #[error("The 'Simple' ssh variant doesn't support {function}")]
@@ -116,9 +120,11 @@ pub fn connect(
                 .stdin(Stdio::null())
                 .with_shell()
                 .arg("-G")
-                .arg(url.host_argument_safe().ok_or_else(|| Error::AmbiguousHostName {
-                    host: url.host().expect("set in ssh urls").into(),
-                })?),
+                .arg(match url.host_as_argument() {
+                    Usable(host) => host,
+                    Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?,
+                    Absent => panic!("BUG: host should always be present in SSH URLs"),
+                }),
         );
         gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check");
         kind = if cmd.status().ok().map_or(false, |status| status.success()) {

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

@@ -2,6 +2,8 @@ use std::{ffi::OsStr, io::ErrorKind};
 
 use bstr::{BString, ByteSlice, ByteVec};
 
+use gix_url::ArgumentSafety::*;
+
 use crate::{
     client::{ssh, ssh::ProgramKind},
     Protocol,
@@ -60,23 +62,21 @@ impl ProgramKind {
                 }
             }
         };
-        let host_as_ssh_arg = match url.user() {
-            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()
-            }
+
+        let host_maybe_with_user_as_ssh_arg = match (url.user_as_argument(), url.host_as_argument()) {
+            (Usable(user), Usable(host)) => format!("{user}@{host}"),
+            (Usable(user), Dangerous(host)) => format!("{user}@{host}"), // The `user@` makes it safe.
+            (Absent, Usable(host)) => host.into(),
+            (Dangerous(user), _) => Err(ssh::invocation::Error::AmbiguousUserName { user: user.into() })?,
+            (_, Dangerous(host)) => Err(ssh::invocation::Error::AmbiguousHostName { host: host.into() })?,
+            (_, Absent) => panic!("BUG: host should always be present in SSH URLs"),
         };
 
-        // Try to force ssh to yield english messages (for parsing later)
-        Ok(prepare.arg(host_as_ssh_arg).env("LANG", "C").env("LC_ALL", "C"))
+        Ok(prepare
+            .arg(host_maybe_with_user_as_ssh_arg)
+            // Try to force ssh to yield English messages (for parsing later).
+            .env("LANG", "C")
+            .env("LC_ALL", "C"))
     }
 
     /// Note that the caller has to assure that the ssh program is launched in English by setting the locale.

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

@@ -144,22 +144,63 @@ 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() {
+        fn ambiguous_user_is_disallowed_explicit_ssh() {
+            assert!(matches!(
+                try_call(ProgramKind::Ssh, "ssh://-arg@host/p", Protocol::V2),
+                Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-arg"
+            ))
+        }
+
+        #[test]
+        fn ambiguous_user_is_disallowed_implicit_ssh() {
+            assert!(matches!(
+                try_call(ProgramKind::Ssh, "-arg@host:p/q", Protocol::V2),
+                Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-arg"
+            ))
+        }
+
+        #[test]
+        fn ambiguous_host_is_allowed_with_user_explicit_ssh() {
             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() {
+        fn ambiguous_host_is_allowed_with_user_implicit_ssh() {
+            assert_eq!(
+                call_args(ProgramKind::Ssh, "user@-arg:p/q", Protocol::V2),
+                joined(&["ssh", "-o", "SendEnv=GIT_PROTOCOL", "user@-arg"])
+            );
+        }
+
+        #[test]
+        fn ambiguous_host_is_disallowed_without_user() {
             assert!(matches!(
                 try_call(ProgramKind::Ssh, "ssh://-arg/p", Protocol::V2),
                 Err(ssh::invocation::Error::AmbiguousHostName { host }) if host == "-arg"
             ));
         }
 
+        #[test]
+        fn ambiguous_user_and_host_remain_disallowed_together_explicit_ssh() {
+            assert!(matches!(
+                try_call(ProgramKind::Ssh, "ssh://-arg@host/p", Protocol::V2),
+                Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-arg"
+            ));
+        }
+
+        #[test]
+        fn ambiguous_user_and_host_remain_disallowed_together_implicit_ssh() {
+            assert!(matches!(
+                try_call(ProgramKind::Ssh, "-userarg@-hostarg:p/q", Protocol::V2),
+                Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-userarg"
+            ));
+        }
+
         #[test]
         fn simple_cannot_handle_any_arguments() {
             assert!(matches!(

gix-url/src/lib.rs

@@ -55,6 +55,27 @@ pub fn expand_path(user: Option<&expand_path::ForUser>, path: &BStr) -> Result<P
     })
 }
 
+/// Classification of a portion of a URL by whether it is *syntactically* safe to pass as an argument to a command-line program.
+///
+/// Various parts of URLs can be specified to begin with `-`. If they are used as options to a command-line application
+/// such as an SSH client, they will be treated as options rather than as non-option arguments as the developer intended.
+/// This is a security risk, because URLs are not always trusted and can often be composed or influenced by an attacker.
+/// See <https://secure.phabricator.com/T12961> for details.
+///
+/// # Security Warning
+///
+/// This type only expresses known *syntactic* risk. It does not cover other risks, such as passing a personal access
+/// token as a username rather than a password in an application that logs usernames.
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
+pub enum ArgumentSafety<'a> {
+    /// May be safe. There is nothing to pass, so there is nothing dangerous.
+    Absent,
+    /// May be safe. The argument does not begin with a `-` and so will not be confused as an option.
+    Usable(&'a str),
+    /// Dangerous! Begins with `-` and could be treated as an option. Use the value in error messages only.
+    Dangerous(&'a str),
+}
+
 /// A URL with support for specialized git related capabilities.
 ///
 /// Additionally there is support for [deserialization](Url::from_bytes()) and [serialization](Url::to_bstring()).
@@ -85,12 +106,12 @@ pub struct Url {
     pub port: Option<u16>,
     /// The path portion of the URL, usually the location of the git repository.
     ///
-    /// # Security-Warning
+    /// # 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.
+    /// If this value is ever going to be passed to a command-line application, call [Self::path_argument_safe()] instead.
     pub path: BString,
 }
 
@@ -164,48 +185,101 @@ impl Url {
 
 /// Access
 impl Url {
-    /// Returns the user mentioned in the url, if present.
+    /// Return the username mentioned in the URL, if present.
+    ///
+    /// # Security Warning
+    ///
+    /// URLs allow usernames to start with `-` which makes it possible to mask command-line arguments as username 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 ever going to be passed to a command-line application, call [Self::user_argument_safe()] instead.
     pub fn user(&self) -> Option<&str> {
         self.user.as_deref()
     }
-    /// Returns the password mentioned in the url, if present.
+
+    /// Classify the username of this URL by whether it is safe to pass as a command-line argument.
+    ///
+    /// Use this method instead of [Self::user()] if the host is going to be passed to a command-line application.
+    /// If the unsafe and absent cases need not be distinguished, [Self::user_argument_safe()] may also be used.
+    pub fn user_as_argument(&self) -> ArgumentSafety<'_> {
+        match self.user() {
+            Some(user) if looks_like_command_line_option(user.as_bytes()) => ArgumentSafety::Dangerous(user),
+            Some(user) => ArgumentSafety::Usable(user),
+            None => ArgumentSafety::Absent,
+        }
+    }
+
+    /// Return the username of this URL if present *and* if it can't be mistaken for a command-line argument.
+    ///
+    /// Use this method or [Self::user_as_argument()] instead of [Self::user()] if the host is going to be
+    /// passed to a command-line application. Prefer [Self::user_as_argument()] unless the unsafe and absent
+    /// cases need not be distinguished from each other.
+    pub fn user_argument_safe(&self) -> Option<&str> {
+        match self.user_as_argument() {
+            ArgumentSafety::Usable(user) => Some(user),
+            _ => None,
+        }
+    }
+
+    /// Return the password mentioned in the url, if present.
     pub fn password(&self) -> Option<&str> {
         self.password.as_deref()
     }
-    /// Returns the host mentioned in the url, if present.
+
+    /// Return the host mentioned in the URL, if present.
     ///
-    /// # Security-Warning
+    /// # 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.
+    /// If this value is ever going to be passed to a command-line application, call [Self::host_as_argument()]
+    /// or [Self::host_argument_safe()] instead.
     pub fn host(&self) -> Option<&str> {
         self.host.as_deref()
     }
 
+    /// Classify the host of this URL by whether it is safe to pass as a command-line argument.
+    ///
+    /// Use this method instead of [Self::host()] if the host is going to be passed to a command-line application.
+    /// If the unsafe and absent cases need not be distinguished, [Self::host_argument_safe()] may also be used.
+    pub fn host_as_argument(&self) -> ArgumentSafety<'_> {
+        match self.host() {
+            Some(host) if looks_like_command_line_option(host.as_bytes()) => ArgumentSafety::Dangerous(host),
+            Some(host) => ArgumentSafety::Usable(host),
+            None => ArgumentSafety::Absent,
+        }
+    }
+
     /// 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.
+    /// Use this method or [Self::host_as_argument()] instead of [Self::host()] if the host is going to be
+    /// passed to a command-line application. Prefer [Self::host_as_argument()] unless the unsafe and absent
+    /// cases need not be distinguished from each other.
     pub fn host_argument_safe(&self) -> Option<&str> {
-        self.host().filter(|host| !looks_like_argument(host.as_bytes()))
+        match self.host_as_argument() {
+            ArgumentSafety::Usable(host) => Some(host),
+            _ => None,
+        }
     }
 
-    /// Return the path of this URL *and* if it can't be mistaken for a command-line argument.
+    /// Return the path of this URL *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.
+    /// Use this method instead of accessing [Self::path] directly if the path is going to be passed to a
+    /// command-line application, unless it is certain that the leading `/` will always be included.
     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()))
+            .and_then(|truncated| (!looks_like_command_line_option(truncated)).then_some(self.path.as_ref()))
     }
 
-    /// Returns true if the path portion of the url is `/`.
+    /// Return true if the path portion of the URL is `/`.
     pub fn path_is_root(&self) -> bool {
         self.path == "/"
     }
-    /// Returns the actual or default port for use according to the url scheme.
+
+    /// Return the actual or default port for use according to the URL scheme.
     /// Note that there may be no default port either.
     pub fn port_or_default(&self) -> Option<u16> {
         self.port.or_else(|| {
@@ -221,13 +295,13 @@ impl Url {
     }
 }
 
-fn looks_like_argument(b: &[u8]) -> bool {
+fn looks_like_command_line_option(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
+    /// Turn a file URL like `file://relative` into `file:///root/relative`, hence it assures the URL's path component is absolute, using
     /// `current_dir` if necessary.
     pub fn canonicalized(&self, current_dir: &std::path::Path) -> Result<Self, gix_path::realpath::Error> {
         let mut res = self.clone();
@@ -287,7 +361,7 @@ impl Url {
 
 /// Deserialization
 impl Url {
-    /// Parse a URL from `bytes`
+    /// Parse a URL from `bytes`.
     pub fn from_bytes(bytes: &BStr) -> Result<Self, parse::Error> {
         parse(bytes)
     }