Skip to content

Commit

Permalink
fix: prevent hosts or paths that look like arguments to be passed to …
Browse files Browse the repository at this point in the history
…invoked commands.

See https://secure.phabricator.com/T12961 for more details.
  • Loading branch information
Byron committed Sep 24, 2023
1 parent d80b5f6 commit b06a0dd
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 19 deletions.
6 changes: 6 additions & 0 deletions gix-transport/src/client/blocking_io/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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(),
Expand Down
23 changes: 16 additions & 7 deletions gix-transport/src/client/blocking_io/ssh/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down Expand Up @@ -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,
},
}
}

Expand Down Expand Up @@ -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()
Expand Down
17 changes: 13 additions & 4 deletions gix-transport/src/client/blocking_io/ssh/program_kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
23 changes: 19 additions & 4 deletions gix-transport/src/client/blocking_io/ssh/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]),
Expand Down
15 changes: 15 additions & 0 deletions gix-transport/src/client/git/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions gix-transport/src/client/non_io_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion gix-transport/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions gix-url/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -179,7 +179,7 @@ impl Url {
}

fn looks_like_argument(b: &[u8]) -> bool {
b.get(0) == Some(&b'-')
b.first() == Some(&b'-')
}

/// Transformation
Expand Down

0 comments on commit b06a0dd

Please sign in to comment.