Initial attempt at internalizing self-approval into the plugin

sample/bin/sudo_approve

@@ -24,6 +24,7 @@ declare -r SUDO_SOCKET_PATH="/var/run/sudo_pair"
 
 pair() {
     declare -r socket="${1}"
+    declare -r token="${2}"
 
     # restore TTY settings on exit
     # shellcheck disable=SC2064
@@ -40,71 +41,67 @@ pair() {
     clear
 
     # prompt the user to approve
-    socat STDIO unix-connect:"${socket}"
+    #
+    # The `kill $!` trick warrants some explanation. `$!` evaluates to
+    # the `pid` of the most-recent subshell, so this kills the echo/cat
+    # subshell when the `socat` has finished. This is necessary because
+    # the subshell won't exit automatically when `socat` end of the pipe
+    # closes. It only closes when it tries to send more data to the pipe
+    # (which causes a SIGPIPE to get sent). So the `kill` ensures the
+    # subshell is killed without the user having to type something
+    # additional that would cause the command to exit.
+    #
+    # FIXME: Technically, `echo` puts a secret value into an process
+    # argument list, which isn't great. However, the window for
+    # exploitation here is microscopic.
+    { socat STDIO unix-connect:"${socket}"; kill $!; } < <(
+        echo -n "${token}" ; cat -
+    )
 }
 
 usage() {
-    echo "Usage: $(basename -- "$0") uid pid"
+    echo "Usage: $(basename -- "$0") pid"
     exit 1
 }
 
 main() {
     declare -r socket_path="${1}"
-    declare -ri uid="${2}"
-    declare -ri pid="${3}"
-
-    # if we're running this under `sudo`, we want to know the original
-    # user's `uid` from `SUDO_UID`; if not, it's jsut their normal `uid`
-    declare -i ruid
-    ruid="${SUDO_UID:-$(id -u)}"
-    declare -r ruid
-
-    declare -r socket="${socket_path}/${uid}.${pid}.sock"
-
-    declare -i socket_uid socket_gid
-    socket_uid="$(stat -c '%u' "${socket}")"
-    socket_gid="$(stat -c '%g' "${socket}")"
-    declare -r socket_uid socket_gid
-
-    declare socket_user socket_group socket_mode
-    socket_user="$(getent passwd "${socket_uid}" | cut -d: -f1)"
-    socket_group="$(getent group "${socket_gid}" | cut -d: -f1)"
-    socket_mode="$(stat -c '%a' "${socket}")"
-    declare -r socket_user socket_group socket_mode
-
-    # if the user approving the command is the same as the user who
-    # invoked `sudo` in the first place, abort
-    #
-    # another option would be to allow the session, but log it in a way
-    # that it immediately pages oncall security engineers; such an
-    # approach is useful in production systems in that it allows for a
-    # in-case-of-fire-break-glass workaround so engineers can respond to
-    # a outage in the middle of the night
-    #
-    # this responsibility will be moved into the plugin itself when time
-    # allots
-    if [[ "${uid}" -eq "${ruid}" ]]; then
-        echo "Users may not approve their own sudo session"
-        exit 1
+    declare -ri pid="${2}"
+
+    declare -r socket="${socket_path}/${pid}.sock"
+
+    declare -i socket_uid socket_gid socket_mode
+    IFS=, read socket_uid socket_gid socket_mode < <(
+      stat -c '%u,%g,%a' "${socket}"
+    )
+    declare -r socket_uid socket_gid socket_mode
+
+    # if we don't have an authentication token from the pipe yet, get
+    # one (and export it as an environment variable in case we have to
+    # call ourselves again under `sudo`)
+    if [[ -z ${SUDO_PAIR_TOKEN:-} ]]; then
+        declare -x SUDO_PAIR_TOKEN
+        SUDO_PAIR_TOKEN=$(socat unix-connect:"${socket}" STDOUT)
+        declare -r SUDO_PAIR_TOKEN
     fi
 
     # if we can write:          pair
     # if user-owner can write:  sudo to them and try again
     # if group-owner can write: sudo to them and try again
     # if none, die
     if [ -w "${socket}" ]; then
-        pair "${socket}"
+        pair "${socket}" "${SUDO_PAIR_TOKEN}"
     elif [[ $(( 8#${socket_mode} & 8#200 )) -ne 0 ]]; then
-        sudo -u "${socket_user}" "${0}" "${uid}" "${pid}"
+        sudo -u \#"${socket_uid}" "${0}" "${pid}"
     elif [[ $(( 8#${socket_mode} & 8#020 )) -ne 0 ]]; then
-        sudo -g "${socket_group}" "${0}" "${uid}" "${pid}"
+        sudo -g \#"${socket_gid}" "${0}" "${pid}"
     else
         echo "The socket for this sudo session is neither user- nor group-writable."
         exit 2
     fi
 }
 
 case "$#" in
-    2) main "${SUDO_SOCKET_PATH}" "$1" "$2" ;;
+    1) main "${SUDO_SOCKET_PATH}" "$1" ;;
     *) usage ;;
 esac

sample/etc/sudo.prompt.pair

@@ -6,4 +6,4 @@ Once approved, this terminal will mirror all output from the active sudo session
 
 Closing this terminal, losing your network connection to this host, or explicitly ending the session by typing <Ctrl-D> will cause the command being run under elevated privileges to terminate immediately.
 
-Approve? y/n [n]: 
+Approve? y/n [n]: 

sample/etc/sudo.prompt.user

@@ -2,6 +2,6 @@ Due to security and compliance requirements, this `sudo` session will require ap
 
 To continue, another engineer must run:
 
-    ssh -t '%h' 'sh -l -c "%b %u %p"'
+    ssh -t '%h' 'sh -l -c "%b %p"'
 
 If a suitable engineer is not available and you have an immediate and urgent need to run this command (e.g., a payments outage or other serious system issue), you may run the above command to approve your own session. Note that doing so will immediately page an oncall security engineer, so this capability should only be used in the event of an emergency.

sudo_pair/Cargo.toml

@@ -19,6 +19,7 @@ crate-type = ["cdylib"]
 
 [dependencies]
 libc        = '0'
+rand        = '0'
 failure     = '0'
 sudo_plugin = { version = "1.1", path = "../sudo_plugin" }
 

sudo_pair/README.md

@@ -140,6 +140,11 @@ The full list of options are as follows:
 
   Note that root is *always* exempt.
 
+* `self_approval` (default: false)
+  This is a boolean ("true" or "false") that controls whether or not users are allowed to approve their own commands. When a user approves their own command this way, a message is sent to syslog.
+
+  This capability is provided so that engineers can act unilaterally in the event of an emergency when no-one else is available. Since it is effectively a complete bypass of this plugin, the intent is that using this capability should invoke something drastic (e.g., immediately page an oncall security engineer).
+
 ## Prompts
 
 This plugin allows you to configure the prompts that are displayed to
@@ -258,6 +263,7 @@ Given the security-sensitive nature of this project, it is an explicit
 goal to have a minimal set of dependencies. Currently, those are:
 
 * [rust-lang/libc][libc]
+* [rust-lang-nursery/rand][rand]
 * [rust-lang-nursery/rust-bindgen][bindgen]
 * [rust-lang-nursery/failure][failure]
 * [rust-lang-nursery/error-chain][error-chain] (to be removed)
@@ -285,6 +291,7 @@ See [LICENSE-APACHE](LICENSE-APACHE) for details.
 
 [sudo_plugin_man]: https://www.sudo.ws/man/1.8.22/sudo_plugin.man.html
 [libc]: https://github.com/rust-lang/libc
+[rand]: https://github.com/rust-lang-nursery/rand
 [bindgen]: https://github.com/rust-lang-nursery/rust-bindgen
 [error-chain]: https://github.com/rust-lang-nursery/error-chain
 [failure]: https://github.com/rust-lang-nursery/failure

sudo_pair/src/lib.rs

@@ -26,6 +26,7 @@
 // TODO: various badges
 // TODO: fill out all fields of https://doc.rust-lang.org/cargo/reference/manifest.html
 // TODO: implement change_winsize
+// TODO: convert all outgoing errors to be unauthorized errors
 
 #![warn(bad_style)]
 #![warn(future_incompatible)]
@@ -66,6 +67,8 @@
 
 extern crate libc;
 extern crate failure;
+extern crate rand;
+
 #[macro_use] extern crate sudo_plugin;
 
 mod errors;
@@ -94,7 +97,7 @@ const DEFAULT_PAIR_PROMPT_PATH : &str       = "/etc/sudo_pair.prompt.pair";
 const DEFAULT_SOCKET_DIR       : &str       = "/var/run/sudo_pair";
 const DEFAULT_GIDS_ENFORCED    : [gid_t; 1] = [0];
 
-const DEFAULT_USER_PROMPT : &[u8] = b"%B '%p %u'\n";
+const DEFAULT_USER_PROMPT : &[u8] = b"%B '%p'\n";
 const DEFAULT_PAIR_PROMPT : &[u8] = b"%U@%h:%d$ %C\ny/n? [n]: ";
 
 sudo_io_plugin! {
@@ -115,53 +118,47 @@ struct SudoPair {
 
 impl SudoPair {
     fn open(plugin: &'static sudo_plugin::Plugin) -> Result<Self> {
-        // TODO: convert all outgoing errors to be unauthorized errors
-        let mut pair = Self {
+        let mut session = Self {
             plugin,
             options: PluginOptions::from(&plugin.plugin_options),
             socket:  None,
         };
 
-        if pair.is_exempt() {
-            return Ok(pair)
+        // sessions without a socket are bypassed entirely, so if the
+        // session is exempt we can go ahead and return what we already
+        // have
+        if session.is_exempt() {
+            return Ok(session)
         }
 
-        if pair.is_sudoing_to_user_and_group() {
+        // based on the current authorization model, allowing `-u` and
+        // `-g` simultaneously would let a user who can
+        // `sudo -g ${group}` approve a `sudo -u ${user} -g ${group}`
+        // even if they can't `sudo -u ${user}`, so we disable the
+        // capability entirely
+        if session.is_sudoing_to_user_and_group() {
             Err(ErrorKind::SudoToUserAndGroup)?;
         }
 
-        let template_spec = pair.template_spec();
-
-        pair.local_pair_prompt(&template_spec);
-        pair.remote_pair_connect()?;
-        pair.remote_pair_prompt(&template_spec)?;
+        let template_spec = session.template_spec();
 
-        // TODO(security): provide a configurable option to deny or log
-        // if the remote euid is the same as the local euid. For some
-        // reason I convinced myself that this is necessary to implement
-        // in the client and not the pair plugin, but I can't remember
-        // what the reasoning was at the moment.
+        // We want to know the actual user on the other end of the
+        // socket in order to enforce restrictions around self-approval.
         //
-        // Oh, now I remember. It *has* to be done on the client,
-        // because the approval script is run under `sudo` itself so
-        // that we can verify the pairer is also capable of doing the
-        // task the user invoking `sudo` is trying to do. Unfortunately,
-        // the OS APIs we have to determine the other side of the
-        // connection only tell us the *euid*, not the *uid*. So we end
-        // up with the euid of `root` which isn't helpful. So this kind
-        // of check *must* be done on the client.
-        //
-        // Except I have an idea for how to solve this plugin-side. Open
-        // a socket writable by all. When someone connects, get the
-        // credentials of the peer and send them a cryptographically-
-        // random token. Close the socket and reopen a new one as we
-        // currently do. Instead of expecting a `y`, expect the token.
-        // This binds their ability to approve the session (able to
-        // write to the socket) with their original identity (proven
-        // through providing the token from their original user). This
-        // shouldn't be too hard, but I haven't gotten around to it yet.
-
-        Ok(pair)
+        // To do this, we initially allow any user to connect to the
+        // socket. We then record their euid and then we hand them a
+        // cryptographically-random token. The socket is closed and a
+        // new one is opened with restricted permissions. When a client
+        // connects to this socket, we expect them to echo the token
+        // back to us before we ask for their approval.
+        let peer_token : [u8; 16] = rand::random();
+
+        session.local_pair_prompt(&template_spec);
+        session.remote_pair_connect_unprivileged(&peer_token)?;
+        session.remote_pair_connect_privileged(&peer_token)?;
+        session.remote_pair_prompt(&template_spec)?;
+
+        Ok(session)
     }
 
     fn close(&mut self, _: i64, _: i64) {
@@ -253,19 +250,53 @@ impl SudoPair {
             .ok_or_else(||self.plugin.stderr().write_all(&prompt));
     }
 
-    fn remote_pair_connect(&mut self) -> Result<()> {
-        if self.socket.is_some() {
-            return Ok(());
+    fn remote_pair_connect_unprivileged(&mut self, token: &[u8; 16]) -> Result<()> {
+        let mut socket = Socket::open(
+            self.socket_path(),
+            self.socket_uid(),
+            self.socket_gid(),
+            libc::S_IWUSR | libc::S_IWGRP | libc::S_IWOTH,
+        ).context(ErrorKind::CommunicationError)?;
+
+        let peer_euid = socket.peer_euid()
+            .context(ErrorKind::CommunicationError)?;
+
+        if peer_euid == self.plugin.user_info.uid {
+            // TODO: log or abort
         }
 
+        socket
+            .write_all(token)
+            .context(ErrorKind::CommunicationError)?;
+
+        Ok(())
+    }
+
+    fn remote_pair_connect_privileged(&mut self, token: &[u8; 16]) -> Result<()> {
         // TODO: clearly indicate when the socket path is missing
-        let socket = Socket::open(
+        let mut socket = Socket::open(
             self.socket_path(),
             self.socket_uid(),
             self.socket_gid(),
             self.socket_mode(),
         ).context(ErrorKind::CommunicationError)?;
 
+        let mut response : [u8; 16] = [0; 16];
+
+        // TODO: read_exact will cause this process to block
+        // indefinitely (even on Ctrl-C) until the correct number of
+        // bytes are read; this won't happen in normal circumstances,
+        // but a bug in (or untimely exit of) the approval script can
+        // cause this process to hang and require being killed
+        socket.read_exact(&mut response)
+            .context(ErrorKind::CommunicationError)?;
+
+        // non-constant comparison is fine here since a comparison
+        // failure results in an immediate exit
+        if response != *token {
+            Err(ErrorKind::SessionDeclined)?;
+        }
+
         self.socket = Some(socket);
 
         Ok(())
@@ -444,19 +475,8 @@ impl SudoPair {
     }
 
     fn socket_path(&self) -> PathBuf {
-        // we encode the originating `uid` into the pathname since
-        // there's no other (easy) way for the approval command to probe
-        // for this information
-        //
-        // note that we want the *`uid`* and not the `euid` here since
-        // we want to know who the real user is and not the `uid` of the
-        // owner of `sudo`
         self.options.socket_dir.join(
-            format!(
-                "{}.{}.sock",
-                self.plugin.user_info.uid,
-                self.plugin.user_info.pid,
-            )
+            format!("{}.sock", self.plugin.user_info.pid)
         )
     }
 
@@ -606,6 +626,21 @@ struct PluginOptions {
     ///
     /// Default: `[]` (however, root is *always* exempt)
     gids_exempted: HashSet<gid_t>,
+
+    /// `self_approval` is a boolean ("true" or "false") that controls
+    /// whether or not users are allowed to approve their own commands.
+    /// When a user approves their own command this way, a message is
+    /// sent to syslog.
+    ///
+    /// This capability is provided so that engineers can act
+    /// unilaterally in the event of an emergency when no-one else is
+    /// available. Since it is effectively a complete bypass of this
+    /// plugin, the intent is that using this capability should invoke
+    /// something drastic (e.g., immediately page an oncall security
+    /// engineer).
+    ///
+    /// Default: `false`
+    self_approval: bool,
 }
 
 impl PluginOptions {
@@ -639,6 +674,9 @@ impl<'a> From<&'a OptionMap> for PluginOptions {
 
             gids_exempted: map.get("gids_exempted")
                 .unwrap_or_default(),
+
+            self_approval: map.get("self_approval")
+                .unwrap_or(false),
         }
     }
 }