diff --git a/gix-transport/src/client/blocking_io/file.rs b/gix-transport/src/client/blocking_io/file.rs index 599f56c23e..613fd23578 100644 --- a/gix-transport/src/client/blocking_io/file.rs +++ b/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(), diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 7c042dc28b..642aab9fd4 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/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() diff --git a/gix-transport/src/client/blocking_io/ssh/program_kind.rs b/gix-transport/src/client/blocking_io/ssh/program_kind.rs index f02d444444..70905829f6 100644 --- a/gix-transport/src/client/blocking_io/ssh/program_kind.rs +++ b/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,7 +53,7 @@ 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", }); @@ -62,8 +61,18 @@ impl ProgramKind { } }; 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) diff --git a/gix-transport/src/client/blocking_io/ssh/tests.rs b/gix-transport/src/client/blocking_io/ssh/tests.rs index f0820d14ed..4e4da78070 100644 --- a/gix-transport/src/client/blocking_io/ssh/tests.rs +++ b/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"]), diff --git a/gix-transport/src/client/git/mod.rs b/gix-transport/src/client/git/mod.rs index 2b950b44a4..d27f468ff8 100644 --- a/gix-transport/src/client/git/mod.rs +++ b/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" + ) + } } } diff --git a/gix-transport/src/client/non_io_types.rs b/gix-transport/src/client/non_io_types.rs index 807b22a8f5..a1dbb247c7 100644 --- a/gix-transport/src/client/non_io_types.rs +++ b/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 { diff --git a/gix-transport/src/lib.rs b/gix-transport/src/lib.rs index 5176125ec9..4ec2ea6155 100644 --- a/gix-transport/src/lib.rs +++ b/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, diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index add7b176b2..1d90689ae6 100644 --- a/gix-url/src/lib.rs +++ b/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 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 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