Merge bitcoin/bitcoin#27896: Remove the syscall sandbox

32e2ffc Remove the syscall sandbox (fanquake)

Pull request description:

  After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e [firejail](https://github.com/netblue30/firejail).

  There is more related discussion in #24771.

  Note that given where it's used, the sandbox also gets dragged into the kernel.

  If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.

  Closes #24771.

ACKs for top commit:
  davidgumberg:
     crACK bitcoin/bitcoin@32e2ffc
  achow101:
    ACK 32e2ffc
  dergoegge:
    ACK 32e2ffc

Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16

Author: achow101

ci/test/00_setup_env_i686_multiprocess.sh

@@ -15,4 +15,3 @@ export GOAL="install"
 export BITCOIN_CONFIG="--enable-debug CC='clang -m32' CXX='clang++ -m32' \
 LDFLAGS='--rtlib=compiler-rt -lgcc_s' CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE'"
 export TEST_RUNNER_ENV="BITCOIND=bitcoin-node"
-export TEST_RUNNER_EXTRA="--nosandbox"

ci/test/00_setup_env_native_valgrind.sh

@@ -11,7 +11,7 @@ export CONTAINER_NAME=ci_native_valgrind
 export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libsqlite3-dev"
 export USE_VALGRIND=1
 export NO_DEPENDS=1
-export TEST_RUNNER_EXTRA="--nosandbox --exclude feature_init,rpc_bind,feature_bind_extra"  # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
+export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra"  # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
 export GOAL="install"
 # Temporarily pin dwarf 4, until using Valgrind 3.20 or later
 export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++ CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'"  # TODO enable GUI

configure.ac

@@ -96,12 +96,6 @@ case $host in
   ;;
 esac
 
-AC_ARG_WITH([seccomp],
-  [AS_HELP_STRING([--with-seccomp],
-  [enable experimental syscall sandbox feature (-sandbox), default is yes if seccomp-bpf is detected under Linux x86_64])],
-  [seccomp_found=$withval],
-  [seccomp_found=auto])
-
 AC_ARG_ENABLE([c++20],
   [AS_HELP_STRING([--enable-c++20],
   [enable compilation in c++20 mode (disabled by default)])],
@@ -1540,36 +1534,6 @@ if test "$use_external_signer" != "no"; then
 fi
 AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = "yes"])
 
-dnl Do not compile with syscall sandbox support when compiling under the sanitizers.
-dnl The sanitizers introduce use of syscalls that are not typically used in bitcoind
-dnl (such as execve when the sanitizers execute llvm-symbolizer).
-if test "$use_sanitizers" != ""; then
-  AC_MSG_WARN([Specifying --with-sanitizers forces --without-seccomp since the sanitizers introduce use of syscalls not allowed by the bitcoind syscall sandbox (-sandbox=<mode>).])
-  seccomp_found=no
-fi
-if test "$seccomp_found" != "no"; then
-  AC_MSG_CHECKING([for seccomp-bpf (Linux x86-64)])
-  AC_PREPROC_IFELSE([AC_LANG_PROGRAM([[
-      @%:@include <linux/seccomp.h>
-    ]], [[
-      #if !defined(__x86_64__)
-      #  error Syscall sandbox is an experimental feature currently available only under Linux x86-64.
-      #endif
-    ]])],[
-      AC_MSG_RESULT([yes])
-      seccomp_found="yes"
-      AC_DEFINE([USE_SYSCALL_SANDBOX], [1], [Define this symbol to build with syscall sandbox support.])
-    ],[
-      AC_MSG_RESULT([no])
-      seccomp_found="no"
-  ])
-fi
-dnl Currently only enable -sandbox=<mode> feature if seccomp is found.
-dnl In the future, sandboxing could be also be supported with other
-dnl sandboxing mechanisms besides seccomp.
-use_syscall_sandbox=$seccomp_found
-AM_CONDITIONAL([ENABLE_SYSCALL_SANDBOX], [test "$use_syscall_sandbox" != "no"])
-
 dnl Check for reduced exports
 if test "$use_reduce_exports" = "yes"; then
   AX_CHECK_COMPILE_FLAG([-fvisibility=hidden], [CORE_CXXFLAGS="$CORE_CXXFLAGS -fvisibility=hidden"],
@@ -2009,7 +1973,6 @@ echo
 echo "Options used to compile and link:"
 echo "  external signer = $use_external_signer"
 echo "  multiprocess    = $build_multiprocess"
-echo "  with experimental syscall sandbox support = $use_syscall_sandbox"
 echo "  with libs       = $build_bitcoin_libs"
 echo "  with wallet     = $enable_wallet"
 if test "$enable_wallet" != "no"; then

src/Makefile.am

@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
   util/sock.h \
   util/spanparsing.h \
   util/string.h \
-  util/syscall_sandbox.h \
   util/syserror.h \
   util/thread.h \
   util/threadinterrupt.h \
@@ -741,7 +740,6 @@ libbitcoin_util_a_SOURCES = \
   util/spanparsing.cpp \
   util/strencodings.cpp \
   util/string.cpp \
-  util/syscall_sandbox.cpp \
   util/time.cpp \
   util/tokenpipe.cpp \
   $(BITCOIN_CORE_H)
@@ -976,7 +974,6 @@ libbitcoinkernel_la_SOURCES = \
   util/serfloat.cpp \
   util/strencodings.cpp \
   util/string.cpp \
-  util/syscall_sandbox.cpp \
   util/syserror.cpp \
   util/thread.cpp \
   util/threadnames.cpp \

src/bitcoind.cpp

@@ -24,7 +24,6 @@
 #include <util/check.h>
 #include <util/exception.h>
 #include <util/strencodings.h>
-#include <util/syscall_sandbox.h>
 #include <util/syserror.h>
 #include <util/threadnames.h>
 #include <util/tokenpipe.h>
@@ -242,7 +241,6 @@ static bool AppInit(NodeContext& node)
         daemon_ep.Close();
     }
 #endif
-    SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
     return fRet;
 }
 

src/checkqueue.h

@@ -7,7 +7,6 @@
 
 #include <sync.h>
 #include <tinyformat.h>
-#include <util/syscall_sandbox.h>
 #include <util/threadnames.h>
 
 #include <algorithm>
@@ -149,7 +148,6 @@ class CCheckQueue
         for (int n = 0; n < threads_num; ++n) {
             m_worker_threads.emplace_back([this, n]() {
                 util::ThreadRename(strprintf("scriptch.%i", n));
-                SetSyscallSandboxPolicy(SyscallSandboxPolicy::VALIDATION_SCRIPT_CHECK);
                 Loop(false /* worker thread */);
             });
         }

src/httpserver.cpp

@@ -18,7 +18,6 @@
 #include <shutdown.h>
 #include <sync.h>
 #include <util/strencodings.h>
-#include <util/syscall_sandbox.h>
 #include <util/threadnames.h>
 #include <util/translation.h>
 
@@ -297,7 +296,6 @@ static void http_reject_request_cb(struct evhttp_request* req, void*)
 static void ThreadHTTP(struct event_base* base)
 {
     util::ThreadRename("http");
-    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_HTTP_SERVER);
     LogPrint(BCLog::HTTP, "Entering http event loop\n");
     event_base_dispatch(base);
     // Event loop will be interrupted by InterruptHTTPServer()
@@ -350,7 +348,6 @@ static bool HTTPBindAddresses(struct evhttp* http)
 static void HTTPWorkQueueRun(WorkQueue<HTTPClosure>* queue, int worker_num)
 {
     util::ThreadRename(strprintf("httpworker.%i", worker_num));
-    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_HTTP_SERVER_WORKER);
     queue->Run();
 }
 

src/index/base.cpp

@@ -14,7 +14,6 @@
 #include <node/interface_ui.h>
 #include <shutdown.h>
 #include <tinyformat.h>
-#include <util/syscall_sandbox.h>
 #include <util/thread.h>
 #include <util/translation.h>
 #include <validation.h> // For g_chainman
@@ -167,7 +166,6 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 
 void BaseIndex::ThreadSync()
 {
-    SetSyscallSandboxPolicy(SyscallSandboxPolicy::TX_INDEX);
     // Wait for a possible reindex-chainstate to finish until continuing
     // with the index sync
     while (!g_indexes_ready_to_sync) {

src/init.cpp

@@ -80,7 +80,6 @@
 #include <util/result.h>
 #include <util/strencodings.h>
 #include <util/string.h>
-#include <util/syscall_sandbox.h>
 #include <util/syserror.h>
 #include <util/thread.h>
 #include <util/threadnames.h>
@@ -630,10 +629,6 @@ void SetupServerArgs(ArgsManager& argsman)
     hidden_args.emplace_back("-daemonwait");
 #endif
 
-#if defined(USE_SYSCALL_SANDBOX)
-    argsman.AddArg("-sandbox=<mode>", "Use the experimental syscall sandbox in the specified mode (-sandbox=log-and-abort or -sandbox=abort). Allow only expected syscalls to be used by bitcoind. Note that this is an experimental new feature that may cause bitcoind to exit or crash unexpectedly: use with caution. In the \"log-and-abort\" mode the invocation of an unexpected syscall results in a debug handler being invoked which will log the incident and terminate the program (without executing the unexpected syscall). In the \"abort\" mode the invocation of an unexpected syscall results in the entire process being killed immediately by the kernel without executing the unexpected syscall.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-#endif // USE_SYSCALL_SANDBOX
-
     // Add the hidden options
     argsman.AddHiddenArgs(hidden_args);
 }
@@ -844,7 +839,7 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status)
     return true;
 }
 
-bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandbox)
+bool AppInitParameterInteraction(const ArgsManager& args)
 {
     const CChainParams& chainparams = Params();
     // ********************************************************* Step 2: parameter interactions
@@ -991,40 +986,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
     if (args.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) > 1)
         return InitError(Untranslated("Unknown rpcserialversion requested."));
 
-#if defined(USE_SYSCALL_SANDBOX)
-    if (args.IsArgSet("-sandbox") && !args.IsArgNegated("-sandbox")) {
-        const std::string sandbox_arg{args.GetArg("-sandbox", "")};
-        bool log_syscall_violation_before_terminating{false};
-        if (sandbox_arg == "log-and-abort") {
-            log_syscall_violation_before_terminating = true;
-        } else if (sandbox_arg == "abort") {
-            // log_syscall_violation_before_terminating is false by default.
-        } else {
-            return InitError(Untranslated("Unknown syscall sandbox mode (-sandbox=<mode>). Available modes are \"log-and-abort\" and \"abort\"."));
-        }
-        // execve(...) is not allowed by the syscall sandbox.
-        const std::vector<std::string> features_using_execve{
-            "-alertnotify",
-            "-blocknotify",
-            "-signer",
-            "-startupnotify",
-            "-walletnotify",
-        };
-        for (const std::string& feature_using_execve : features_using_execve) {
-            if (!args.GetArg(feature_using_execve, "").empty()) {
-                return InitError(Untranslated(strprintf("The experimental syscall sandbox feature (-sandbox=<mode>) is incompatible with %s (which uses execve).", feature_using_execve)));
-            }
-        }
-        if (!SetupSyscallSandbox(log_syscall_violation_before_terminating)) {
-            return InitError(Untranslated("Installation of the syscall sandbox failed."));
-        }
-        if (use_syscall_sandbox) {
-            SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION);
-        }
-        LogPrintf("Experimental syscall sandbox enabled (-sandbox=%s): bitcoind will terminate if an unexpected (not allowlisted) syscall is invoked.\n", sandbox_arg);
-    }
-#endif // USE_SYSCALL_SANDBOX
-
     // Also report errors from parsing before daemonization
     {
         KernelNotifications notifications{};

src/init.h

@@ -44,7 +44,7 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status);
  * @note This can be done before daemonization. Do not call Shutdown() if this function fails.
  * @pre Parameters should be parsed and config file should be read, AppInitBasicSetup should have been called.
  */
-bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandbox = true);
+bool AppInitParameterInteraction(const ArgsManager& args);
 /**
  * Initialization sanity checks.
  * @note This can be done before daemonization. Do not call Shutdown() if this function fails.