Skip to content

Commit

Permalink
fix: Prevent usernames with leading - from being passed to SSH
Browse files Browse the repository at this point in the history
This detects ambiguous usernames in dangerous cases where they
would be passed to external commands to form SSH connections, if
they would be misinterpreted as option arguments.

This change is analogous to b06a0dd, hardening `gix-transport` and
applications that use it against options smuggled in URLs, but for
the non-mandatory username portion of a URL, rather than the host
and path portions that were covered there.

For example, commands like these no longer pass `-F...` options to
`ssh`:

    gix clone 'ssh://-Fconfigfile@example.com/abc'
    gix clone -- '-Fconfigfile@example.com:abc/def'

Instead, they refuse to run `ssh`, producing the error:

    Error: Username '-Fconfigfile' could be mistaken for a command-line argument
  • Loading branch information
EliahKagan committed Apr 12, 2024
1 parent 5428609 commit f56ad39
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
13 changes: 10 additions & 3 deletions gix-transport/src/client/blocking_io/ssh/program_kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,12 @@ impl ProgramKind {
}
}
};
let host_as_ssh_arg = match url.user() {
let host_maybe_with_user_as_ssh_arg = match url.user() {
Some(user) => {
// FIXME: See the fixme comment on Url::user_argument_safe() about its return type.
if url.user_argument_safe() != Some(user) {
return Err(ssh::invocation::Error::AmbiguousUserName { user: user.into() });
}
let host = url.host().expect("present in ssh urls");
format!("{user}@{host}")
}
Expand All @@ -75,8 +79,11 @@ impl ProgramKind {
}
};

// 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"))
// Try to force ssh to yield English messages (for parsing later).
Ok(prepare
.arg(host_maybe_with_user_as_ssh_arg)
.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.
Expand Down
7 changes: 7 additions & 0 deletions gix-url/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,13 @@ impl Url {
///
/// Use this method if the user or a portion of the URL that begins with it will be passed to a command-line application.
pub fn user_argument_safe(&self) -> Option<&str> {
// FIXME: A return value of None from this method, or host_argument_safe(), is ambiguous: the user (or host) is
// either present but unsafe, or absent. Furthermore, in practice the value is usually needed even if unsafe,
// in order to report it in an error message. In gix-transport, the ambiguity makes it easy to write a new bug
// while using this interface for user_argument_safe(). In contrast, in host_argument_safe(), the ambiguity is
// much less of a problem, because the host is expected to be present. Yet the host() method must still be
// called when handling the None case, to include it in the error. If possible, both methods should be replaced
// by methods with a richer return type (a new enum). If not, the ambiguity should be prominently documented.
self.user().filter(|user| !looks_like_argument(user.as_bytes()))
}

Expand Down

0 comments on commit f56ad39

Please sign in to comment.