Merge pull request #2431 from gitbutlerapp/add-git-executor-security-layer

add initial trait types for askpass socket servers

gitbutler-git/Cargo.toml

@@ -28,11 +28,11 @@ tokio = ["dep:tokio"]
 git2 = { workspace = true, optional = true }
 thiserror = { workspace = true, optional = true }
 serde = { workspace = true, optional = true }
-tokio = { workspace = true, optional = true, features = ["process"]}
+tokio = { workspace = true, optional = true, features = ["process", "rt", "process", "time", "io-util", "net", "fs"]}
 rand = { version = "0.8.5", optional = true }
 
 [dev-dependencies]
-tokio = { workspace = true, features = ["rt", "rt-multi-thread", "process"]}
+tokio = { workspace = true, features = ["rt-multi-thread"]}
 
 [target."cfg(unix)".dependencies]
-nix = { version = "0.27.1", optional = true, features = ["process", "socket"] }
+nix = { version = "0.27.1", optional = true, features = ["process", "socket", "user"] }

gitbutler-git/src/backend/cli/executor.rs

@@ -1,4 +1,5 @@
 use crate::prelude::*;
+use core::time::Duration;
 
 #[cfg(any(test, feature = "tokio"))]
 pub mod tokio;
@@ -8,7 +9,34 @@ pub mod tokio;
 /// There is no `arg0` passed; it's up to the implementation
 /// to decide how to execute the command. For example,
 /// `git status` would be passed as `["status"]`.
-pub trait GitExecutor {
+///
+/// The executor also provides a means for spinning up
+/// ad-hoc socket servers, necessary for the authorization
+/// utilities that are passed to Git and SSH to communicate
+/// with the host process to exchange credentials. We also
+/// implement a simple layer of security over this communication
+/// layer to avoid unintended leakage of credentials.
+///
+/// Note that this security layer is _not_ impervious
+/// to determined attackers. It is merely a means to
+/// avoid unintended connections to the socket server
+/// or simple, generic attacks. The threat model assumes
+/// that more sophisticated attacks targeting the host system
+/// are out of scope for this project given that the
+/// communication layer is not a far cry from the user
+/// inputting the credentials manually, directly into the
+/// CLI utility.
+///
+/// # Safety
+///
+/// This trait is marked as unsafe due to the platform-specific
+/// invariants described in [`GitExecutor::create_askpass_server`].
+/// These invariants are not enforced by the typesystem, and while
+/// we have some loose checks to ensure that the invariants are upheld,
+/// we cannot guarantee that they are upheld in all cases. Thus, it is
+/// up to the implementor to ensure that the invariants are upheld.
+#[allow(unsafe_code)]
+pub unsafe trait GitExecutor {
     /// The error type returned by this executor,
     /// specifically in cases where the execution fails.
     ///
@@ -16,6 +44,9 @@ pub trait GitExecutor {
     /// the exit code is non-zero.
     type Error: core::error::Error + core::fmt::Debug + Send + Sync + 'static;
 
+    /// The type of the handle returned by [`GitExecutor::create_askpass_server`].
+    type ServerHandle: AskpassServer + Send + Sync + 'static;
+
     /// Executes the given Git command with the given arguments.
     /// `git` is never passed as the first argument (arg 0).
     ///
@@ -51,4 +82,117 @@ pub trait GitExecutor {
 
         self.execute_raw(&args, Some(envs)).await
     }
+
+    /// Creates a named pipe server that is compatible with
+    /// the `askpass` utility (see `bin/askpass.rs` and platform-specific
+    /// adjacent sources).
+    ///
+    /// ## Unix
+    ///
+    /// On Unix-like systems (including MacOS), this is a unix
+    /// domain socket. The path of the socket is returned as
+    /// a handle type that is format-able as a string which is
+    /// passed to the askpass utility as `GITBUTLER_ASKPASS_SOCKET`.
+    ///
+    /// The socket itself should be created as read/write for the user
+    /// with no access to group or everyone (`0600` or `u+rw ag-a`).
+    ///
+    /// Upon the handle being dropped, the socket must be closed and
+    /// the socket file SHOULD be best-effort unlinked.
+    ///
+    /// Given that this invariant must be upheld, this method is marked
+    /// as unsafe.
+    ///
+    /// ## Windows
+    ///
+    /// On Windows, this is a named pipe. The handle returned must be
+    /// format-able as a string which is passed to the askpass utility
+    /// as `GITBUTLER_ASKPASS_SOCKET` and corresponds to the named
+    /// pipe.
+    ///
+    /// The pipe name MUST start with `\.\pipe\LOCAL\`. Given that this
+    /// invariant must be upheld, this method is marked as unsafe.
+    ///
+    /// Upon the handle being dropped, the pipe must be closed.
+    ///
+    /// # Safety
+    ///
+    /// This method is marked as unsafe due to the platform-specific
+    /// invariants described above that must be upheld by all implementations.
+    /// These invariants are not enforced by the typesystem, and while
+    /// we have some loose checks to ensure that the invariants are upheld,
+    /// we cannot guarantee that they are upheld in all cases. Thus, it is
+    /// up to the implementor to ensure that the invariants are upheld.
+    ///
+    /// If for some reason these invariants are not possible to uphold,
+    /// please open an issue on the repository to discuss this issue.
+    async unsafe fn create_askpass_server<F>(&self) -> Result<Self::ServerHandle, Self::Error>;
+}
+
+/// A handle to a server created by [`GitExecutor::create_askpass_server`].
+///
+/// When formatted as a string, the result should be the connection string
+/// necessary for the askpass utility to connect (e.g. a unix domain socket path
+/// or a windows named pipe name; see [`GitExecutor::create_askpass_server`] for
+/// more information).
+///
+/// Upon dropping the handle, the server should be closed.
+pub trait AskpassServer: core::fmt::Display {
+    /// The type of error that is returned by [`AskpassServer::next`].
+    type Error: core::error::Error + core::fmt::Debug + Send + Sync + 'static;
+
+    /// The type of the socket yielded by the incoming iterator.
+    type SocketHandle: Socket + Send + Sync + 'static;
+
+    /// Waits for a connection to the server to be established.
+    async fn next(&self, timeout: Option<Duration>) -> Result<Self::SocketHandle, Self::Error>;
+}
+
+#[cfg(unix)]
+type PidInner = i32;
+
+#[cfg(windows)]
+type PidInner = u32;
+
+/// The type of a process ID (platforms-specific)
+pub type Pid = PidInner;
+
+/// The type of a user ID (unix-specific).
+#[cfg(unix)]
+pub type Uid = u32;
+
+/// Platform-specific credentials for a connection to a server created by
+/// [`GitExecutor::create_askpass_server`]. This is passed to the callback
+/// provided to [`GitExecutor::create_askpass_server`] when a connection
+/// is established.
+pub trait Socket {
+    /// The error type returned by I/O operations on this socket.
+    type Error: core::error::Error + core::fmt::Debug + Send + Sync + 'static;
+
+    /// The process ID of the connecting client.
+    fn pid(&self) -> Result<Pid, Self::Error>;
+
+    /// The user ID of the connecting client.
+    #[cfg(unix)]
+    fn uid(&self) -> Result<Uid, Self::Error>;
+
+    /// Reads a line from the socket. Must not include the newline.
+    ///
+    /// The returned line must not include a newline, and any
+    /// trailing carriage return (`\r`) must be stripped.
+    ///
+    /// Implementations are allowed to simply call `.trim()` on the
+    /// line, as whitespace is not significant in the protocol.
+    async fn read_line(&mut self) -> Result<String, Self::Error>;
+
+    /// Writes a line to the socket. The write must
+    /// complete fully before returning (i.e. implementations
+    /// should use something akin to `write_all`).
+    ///
+    /// The input line will not include a newline; one must be
+    /// added. Newlines should never include a carriage return (`\r`).
+    ///
+    /// Unlike `read_line`, implementations are not allowed to
+    /// modify the line prior to sending aside from appending a newline.
+    async fn write_line(&mut self, line: &str) -> Result<(), Self::Error>;
 }

gitbutler-git/src/backend/cli/executor/tokio.rs

@@ -1,14 +1,18 @@
 //! A [Tokio](https://tokio.rs)-based [`GitExecutor`] implementation.
 
 use crate::prelude::*;
+use core::time::Duration;
+use std::{fs::Permissions, os::unix::fs::PermissionsExt};
 use tokio::process::Command;
 
 /// A [`GitExecutor`] implementation using the `git` command-line tool
 /// via [`tokio::process::Command`].
 pub struct TokioExecutor;
 
-impl super::GitExecutor for TokioExecutor {
+#[allow(unsafe_code)]
+unsafe impl super::GitExecutor for TokioExecutor {
     type Error = std::io::Error;
+    type ServerHandle = TokioAskpassServer;
 
     async fn execute_raw(
         &self,
@@ -29,4 +33,92 @@ impl super::GitExecutor for TokioExecutor {
             String::from_utf8_lossy(&output.stderr).trim().into(),
         ))
     }
+
+    #[cfg(unix)]
+    async unsafe fn create_askpass_server<F>(&self) -> Result<Self::ServerHandle, Self::Error> {
+        let connection_string =
+            std::env::temp_dir().join(format!("gitbutler-askpass-{}", rand::random::<u64>()));
+
+        let listener = tokio::net::UnixListener::bind(&connection_string)?;
+
+        tokio::fs::set_permissions(&connection_string, Permissions::from_mode(0o0600)).await?;
+
+        Ok(TokioAskpassServer {
+            server: Some(listener),
+            connection_string: connection_string.to_string_lossy().into(),
+        })
+    }
+}
+
+#[cfg(unix)]
+impl super::Socket for tokio::io::BufStream<tokio::net::UnixStream> {
+    type Error = std::io::Error;
+
+    fn pid(&self) -> Result<super::Pid, Self::Error> {
+        self.get_ref()
+            .peer_cred()
+            .unwrap()
+            .pid()
+            .ok_or(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                "no pid available for peer connection",
+            ))
+    }
+
+    fn uid(&self) -> Result<super::Uid, Self::Error> {
+        Ok(self.get_ref().peer_cred().unwrap().uid())
+    }
+
+    async fn read_line(&mut self) -> Result<String, Self::Error> {
+        let mut buf = String::new();
+        <Self as tokio::io::AsyncBufReadExt>::read_line(self, &mut buf).await?;
+        Ok(buf)
+    }
+
+    async fn write_line(&mut self, line: &str) -> Result<(), Self::Error> {
+        <Self as tokio::io::AsyncWriteExt>::write_all(self, line.as_bytes()).await?;
+        <Self as tokio::io::AsyncWriteExt>::write_all(self, b"\n").await?;
+        Ok(())
+    }
+}
+
+/// A tokio-based [`AskpassServer`] implementation.
+#[cfg(unix)]
+pub struct TokioAskpassServer {
+    // Always Some until dropped.
+    server: Option<tokio::net::UnixListener>,
+    connection_string: String,
+}
+
+#[cfg(unix)]
+impl super::AskpassServer for TokioAskpassServer {
+    type Error = std::io::Error;
+    #[cfg(unix)]
+    type SocketHandle = tokio::io::BufStream<tokio::net::UnixStream>;
+
+    async fn next(&self, timeout: Option<Duration>) -> Result<Self::SocketHandle, Self::Error> {
+        let res = if let Some(timeout) = timeout {
+            tokio::time::timeout(timeout, self.server.as_ref().unwrap().accept()).await?
+        } else {
+            self.server.as_ref().unwrap().accept().await
+        };
+
+        Ok(res.map(|(s, _)| tokio::io::BufStream::new(s))?)
+    }
+}
+
+#[cfg(unix)]
+impl core::fmt::Display for TokioAskpassServer {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        self.connection_string.fmt(f)
+    }
+}
+
+#[cfg(unix)]
+impl Drop for TokioAskpassServer {
+    fn drop(&mut self) {
+        drop(self.server.take());
+        // best-effort
+        std::fs::remove_file(&self.connection_string).ok();
+    }
 }