From ca10cc12c1fe40c3ca5c020a219d96aa1f06de92 Mon Sep 17 00:00:00 2001
From: bojanserafimov <bojan.serafimov7@gmail.com>
Date: Tue, 31 May 2022 14:14:09 -0400
Subject: [PATCH] Close file descriptors for redo process (#1834)

---
 Cargo.lock                | 11 +++++++++
 pageserver/Cargo.toml     |  1 +
 pageserver/src/walredo.rs | 49 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/Cargo.lock b/Cargo.lock
index e39375c221..6f8382de27 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -363,6 +363,16 @@ dependencies = [
  "textwrap 0.14.2",
 ]
 
+[[package]]
+name = "close_fds"
+version = "0.3.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3bc416f33de9d59e79e57560f450d21ff8393adcf1cdfc3e6d8fb93d5f88a2ed"
+dependencies = [
+ "cfg-if",
+ "libc",
+]
+
 [[package]]
 name = "cmake"
 version = "0.1.48"
@@ -1789,6 +1799,7 @@ dependencies = [
  "bytes",
  "chrono",
  "clap 3.0.14",
+ "close_fds",
  "const_format",
  "crc32c",
  "crossbeam-utils",
diff --git a/pageserver/Cargo.toml b/pageserver/Cargo.toml
index 290f52e0b2..d78d3622c4 100644
--- a/pageserver/Cargo.toml
+++ b/pageserver/Cargo.toml
@@ -60,6 +60,7 @@ metrics = { path = "../libs/metrics" }
 utils = { path = "../libs/utils" }
 remote_storage = { path = "../libs/remote_storage" }
 workspace_hack = { version = "0.1", path = "../workspace_hack" }
+close_fds = "0.3.2"
 
 [dev-dependencies]
 hex-literal = "0.3"
diff --git a/pageserver/src/walredo.rs b/pageserver/src/walredo.rs
index d263bf0e9a..cad211b1bd 100644
--- a/pageserver/src/walredo.rs
+++ b/pageserver/src/walredo.rs
@@ -28,6 +28,7 @@ use std::fs::OpenOptions;
 use std::io::prelude::*;
 use std::io::{Error, ErrorKind};
 use std::os::unix::io::AsRawFd;
+use std::os::unix::prelude::CommandExt;
 use std::path::PathBuf;
 use std::process::Stdio;
 use std::process::{Child, ChildStderr, ChildStdin, ChildStdout, Command};
@@ -554,6 +555,40 @@ impl PostgresRedoManager {
     }
 }
 
+///
+/// Command with ability not to give all file descriptors to child process
+///
+trait CloseFileDescriptors: CommandExt {
+    ///
+    /// Close file descriptors (other than stdin, stdout, stderr) in child process
+    ///
+    fn close_fds(&mut self) -> &mut Command;
+}
+
+impl<C: CommandExt> CloseFileDescriptors for C {
+    fn close_fds(&mut self) -> &mut Command {
+        unsafe {
+            self.pre_exec(move || {
+                // SAFETY: Code executed inside pre_exec should have async-signal-safety,
+                // which means it should be safe to execute inside a signal handler.
+                // The precise meaning depends on platform. See `man signal-safety`
+                // for the linux definition.
+                //
+                // The set_fds_cloexec_threadsafe function is documented to be
+                // async-signal-safe.
+                //
+                // Aside from this function, the rest of the code is re-entrant and
+                // doesn't make any syscalls. We're just passing constants.
+                //
+                // NOTE: It's easy to indirectly cause a malloc or lock a mutex,
+                // which is not async-signal-safe. Be careful.
+                close_fds::set_fds_cloexec_threadsafe(3, &[]);
+                Ok(())
+            })
+        }
+    }
+}
+
 ///
 /// Handle to the Postgres WAL redo process
 ///
@@ -610,6 +645,7 @@ impl PostgresRedoProcess {
             config.write_all(b"shared_preload_libraries=neon\n")?;
             config.write_all(b"neon.wal_redo=on\n")?;
         }
+
         // Start postgres itself
         let mut child = Command::new(conf.pg_bin_dir().join("postgres"))
             .arg("--wal-redo")
@@ -620,6 +656,19 @@ impl PostgresRedoProcess {
             .env("LD_LIBRARY_PATH", conf.pg_lib_dir())
             .env("DYLD_LIBRARY_PATH", conf.pg_lib_dir())
             .env("PGDATA", &datadir)
+            // The redo process is not trusted, so it runs in seccomp mode
+            // (see seccomp in zenith_wal_redo.c). We have to make sure it doesn't
+            // inherit any file descriptors from the pageserver that would allow
+            // an attacker to do bad things.
+            //
+            // The Rust standard library makes sure to mark any file descriptors with
+            // as close-on-exec by default, but that's not enough, since we use
+            // libraries that directly call libc open without setting that flag.
+            //
+            // One example is the pidfile of the daemonize library, which doesn't
+            // currently mark file descriptors as close-on-exec. Either way, we
+            // want to be on the safe side and prevent accidental regression.
+            .close_fds()
             .spawn()
             .map_err(|e| {
                 Error::new(