From d80b5f69772a6e36b0131d3a538e896a8a6a29b1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 24 Sep 2023 16:00:34 +0200 Subject: [PATCH 1/3] feat: add `Url::host_argument_safe()` and `Url::path_argument_safe()` This will not provide values if they could be confused for an argument to to a commaneline application. --- gix-url/src/lib.rs | 36 ++++++++++++++++++++++++++++++++++++ gix-url/tests/access/mod.rs | 20 ++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index a5f1ba1544..add7b176b2 100644 --- a/gix-url/src/lib.rs +++ b/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, /// 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.get(0) == 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 diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index 72833afd7f..afaa017f08 100644 --- a/gix-url/tests/access/mod.rs +++ b/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(()) +} From b06a0dd781accad317fdec5f86f069df4c21875c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 24 Sep 2023 11:07:18 +0200 Subject: [PATCH 2/3] fix: prevent hosts or paths that look like arguments to be passed to invoked commands. See https://secure.phabricator.com/T12961 for more details. --- gix-transport/src/client/blocking_io/file.rs | 6 +++++ .../src/client/blocking_io/ssh/mod.rs | 23 +++++++++++++------ .../client/blocking_io/ssh/program_kind.rs | 17 ++++++++++---- .../src/client/blocking_io/ssh/tests.rs | 23 +++++++++++++++---- gix-transport/src/client/git/mod.rs | 15 ++++++++++++ gix-transport/src/client/non_io_types.rs | 2 ++ gix-transport/src/lib.rs | 1 - gix-url/src/lib.rs | 6 ++--- 8 files changed, 74 insertions(+), 19 deletions(-) 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 From 114e91c5dbec1a1345fbb84a5625ab5c55abf8a5 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 24 Sep 2023 17:36:12 +0200 Subject: [PATCH 3/3] improve journey test to validate handling ambiguous of ssh-hosts --- tests/journey/gix.sh | 17 +++++++++++++++++ .../no-repo/pack/clone/fail-ambigous-host | 1 + .../no-repo/pack/receive/fail-ambigous-host | 1 + 3 files changed, 19 insertions(+) create mode 100644 tests/snapshots/plumbing/no-repo/pack/clone/fail-ambigous-host create mode 100644 tests/snapshots/plumbing/no-repo/pack/receive/fail-ambigous-host diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index 9a4165677f..ce359b134d 100644 --- a/tests/journey/gix.sh +++ b/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" diff --git a/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambigous-host b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambigous-host new file mode 100644 index 0000000000..52e6b005a9 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambigous-host b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambigous-host new file mode 100644 index 0000000000..473adbb4a2 --- /dev/null +++ b/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 \ No newline at end of file