Initial attempt at internalizing self-approval into the plugin

stouset · stouset · commit 91c763da8fee · 2018-06-28T13:28:30.000-07:00
diff --git a/sample/bin/sudo_approve b/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,26 +41,19 @@ pair() {
     clear
 
     # prompt the user to approve
-    socat STDIO unix-connect:"${socket}"
+    cat <(echo "${token}") - | socat STDIO unix-connect:"${socket}"
 }
 
 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}"
+    declare -ri pid="${2}"
 
-    # 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 -r socket="${socket_path}/${pid}.sock"
 
     declare -i socket_uid socket_gid
     socket_uid="$(stat -c '%u' "${socket}")"
@@ -72,32 +66,22 @@ main() {
     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
+    if [[ ! ${SUDO_PAIR_TOKEN:-} ]]; then
+        declare -x SUDO_PAIR_TOKEN
+        token=$(socat STDOUT unix-connect:"${socket}")
+        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_user}" "${0}" "${pid}"
     elif [[ $(( 8#${socket_mode} & 8#020 )) -ne 0 ]]; then
-        sudo -g "${socket_group}" "${0}" "${uid}" "${pid}"
+        sudo -g "${socket_group}" "${0}" "${pid}"
     else
         echo "The socket for this sudo session is neither user- nor group-writable."
         exit 2
diff --git a/sudo_pair/Cargo.toml b/sudo_pair/Cargo.toml
@@ -19,6 +19,7 @@ crate-type = ["cdylib"]
 
 [dependencies]
 libc        = '0'
+rand        = '0'
 error-chain = '0'
 sudo_plugin = { version = "1.1", path = "../sudo_plugin" }
 
diff --git a/sudo_pair/README.md b/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/error-chain][error-chain]
 
@@ -284,6 +290,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
 [airtight-hatchway]: https://blogs.msdn.microsoft.com/oldnewthing/20060508-22/?p=31283
diff --git a/sudo_pair/src/lib.rs b/sudo_pair/src/lib.rs
@@ -28,6 +28,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)]
@@ -67,6 +68,8 @@
 // #![cfg_attr(feature="cargo-clippy", warn(clippy_cargo))]
 
 extern crate libc;
+extern crate rand;
+
 #[macro_use] extern crate error_chain;
 #[macro_use] extern crate sudo_plugin;
 
@@ -117,54 +120,49 @@ impl SudoPair {
         // TODO: convert all outgoing errors to be unauthorized errors
         let options = PluginOptions::from(&plugin.plugin_options);
 
-        let mut pair = Self {
+        let mut session = Self {
             plugin,
             options,
             socket: None,
         };
 
-        if pair.is_sudoing_to_user_and_group() {
+        // 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)
+        }
+
+        // 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() {
             bail!(ErrorKind::Unauthorized(
                 "sudo_pair doesn't support sudoing to both a user and a group".into()
             ));
         }
 
-        if pair.is_exempt() {
-            return Ok(pair)
-        }
+        let template_spec = session.template_spec();
 
-        let template_spec = pair.template_spec();
-
-        pair.local_pair_prompt(&template_spec);
-        pair.remote_pair_connect()?;
-        pair.remote_pair_prompt(&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.
-        //
-        // 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.
+        // We want to know the actual user on the other end of the
+        // socket in order to enforce restrictions around self-approval.
         //
-        // 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) {
@@ -260,19 +258,47 @@ 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,
+        ).chain_err(|| ErrorKind::Unauthorized("unable to connect to a pair".into()))?;
+
+        let peer_euid = socket.peer_euid()
+            .chain_err(|| ErrorKind::Unauthorized("unable determine uid of pair".into()))?;
+
+        if peer_euid == self.plugin.user_info.uid {
+            // TODO: log or abort
         }
 
+        socket
+            .write_all(token)
+            .chain_err(|| ErrorKind::Unauthorized("unable to ask pair for approval".into()))?;
+
+        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(),
         ).chain_err(|| ErrorKind::Unauthorized("unable to connect to a pair".into()))?;
 
+        let mut response : [u8; 16] = [0; 16];
+        let _ = socket.read_exact(&mut response)
+            .chain_err(|| ErrorKind::Unauthorized("unable to ask pair for approval".into()))?;
+
+        // non-constant comparison is fine here since a comparison
+        // failure results in an immediate exit
+        if response != *token {
+            bail!("denied by pair");
+        }
+
         self.socket = Some(socket);
 
         Ok(())
@@ -452,19 +478,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)
         )
     }
 
@@ -614,6 +629,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 {
@@ -647,6 +677,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),
         }
     }
 }
diff --git a/sudo_pair/src/socket.rs b/sudo_pair/src/socket.rs
@@ -121,6 +121,60 @@ impl Socket {
         self.socket.shutdown(Shutdown::Both)
     }
 
+    #[cfg(any(
+        target_os = "linux",
+        target_os = "android",
+        target_os = "fuchsia",
+    ))]
+    pub(crate) fn peer_euid(&self) -> Result<uid_t> {
+        unsafe {
+            let cred : libc::ucred     = mem::uninitialized();
+            let size : libc::socklen_t = mem::size_of::<libc::ucred>() as _;
+
+            if libc::getsockopt(
+                self.socket.as_raw_fd(),
+                libc::SOL_SOCKET,
+                libc::SO_PEERCRED,
+                &mut cred as _,
+                &mut size,
+            ) == -1 {
+                return Err(Error::last_os_error());
+            }
+
+            // `getsockopt` should fill out the `ucred` pointer
+            // completely, no more and no less
+            debug_assert!(size == mem::size_of::<libc::ucred>() as _);
+
+            Ok(cred.uid)
+        }
+    }
+
+    #[cfg(any(
+        target_os = "macos",
+        target_os = "ios",
+        target_os = "freebsd",
+        target_os = "dragonfly",
+        target_os = "openbsd",
+        target_os = "netbsd",
+        target_os = "bitrig",
+    ))]
+    pub(crate) fn peer_euid(&self) -> Result<uid_t> {
+        unsafe {
+            let mut uid : uid_t = mem::uninitialized();
+            let mut gid : gid_t = mem::uninitialized();
+
+            if libc::getpeereid(
+                self.socket.as_raw_fd(),
+                &mut uid,
+                &mut gid
+            ) == -1 {
+                return Err(Error::last_os_error());
+            }
+
+            Ok(uid)
+        }
+    }
+
     fn unlink(path: &Path) -> Result<()> {
         match fs::metadata(&path).map(|md| md.file_type().is_socket()) {
             // file exists, is a socket; delete it
