Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:seccomp-bpf-2020-11 changing 27 files +1125 −1
  1. practicalswift commented at 9:27 am on November 25, 2020: contributor

    Add experimental syscall sandboxing using seccomp-bpf (Linux secure computing mode).

    Enable filtering of system calls using seccomp-bpf: allow only explicitly allowlisted (expected) syscalls to be called.

    The syscall sandboxing implemented in this PR is an experimental feature currently available only under Linux x86-64.

    To enable the experimental syscall sandbox the -sandbox=<mode> option must be passed to bitcoind:

     0  -sandbox=<mode>
     1       Use the experimental syscall sandbox in the specified mode
     2       (-sandbox=log-and-abort or -sandbox=abort). Allow only expected
     3       syscalls to be used by bitcoind. Note that this is an
     4       experimental new feature that may cause bitcoind to exit or crash
     5       unexpectedly: use with caution. In the "log-and-abort" mode the
     6       invocation of an unexpected syscall results in a debug handler
     7       being invoked which will log the incident and terminate the
     8       program (without executing the unexpected syscall). In the
     9       "abort" mode the invocation of an unexpected syscall results in
    10       the entire process being killed immediately by the kernel without
    11       executing the unexpected syscall.
    

    The allowed syscalls are defined on a per thread basis.

    I’ve used this feature since summer 2020 and I find it to be a helpful testing/debugging addition which makes it much easier to reason about the actual capabilities required of each type of thread in Bitcoin Core.


    Quick start guide:

     0$ ./configure
     1$ src/bitcoind -regtest -debug=util -sandbox=log-and-abort
     2 32021-06-09T12:34:56Z Experimental syscall sandbox enabled (-sandbox=log-and-abort): bitcoind will terminate if an unexpected (not allowlisted) syscall is invoked.
     4 52021-06-09T12:34:56Z Syscall filter installed for thread "addcon"
     62021-06-09T12:34:56Z Syscall filter installed for thread "dnsseed"
     72021-06-09T12:34:56Z Syscall filter installed for thread "net"
     82021-06-09T12:34:56Z Syscall filter installed for thread "msghand"
     92021-06-09T12:34:56Z Syscall filter installed for thread "opencon"
    102021-06-09T12:34:56Z Syscall filter installed for thread "init"
    1112# A simulated execve call to show the sandbox in action:
    132021-06-09T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report.
    1415Aborted (core dumped)
    16$
    

    About seccomp and seccomp-bpf:

    In computer security, seccomp (short for secure computing mode) is a facility in the Linux kernel. seccomp allows a process to make a one-way transition into a “secure” state where it cannot make any system calls except exit(), sigreturn(), and read() and write() to already-open file descriptors. Should it attempt any other system calls, the kernel will terminate the process with SIGKILL or SIGSYS. In this sense, it does not virtualize the system’s resources but isolates the process from them entirely.

    […]

    seccomp-bpf is an extension to seccomp that allows filtering of system calls using a configurable policy implemented using Berkeley Packet Filter rules. It is used by OpenSSH and vsftpd as well as the Google Chrome/Chromium web browsers on Chrome OS and Linux. (In this regard seccomp-bpf achieves similar functionality, but with more flexibility and higher performance, to the older systrace—which seems to be no longer supported for Linux.)

  2. laanwj commented at 9:29 am on November 25, 2020: member
    Concept ACK, nice work!
  3. practicalswift force-pushed on Nov 25, 2020
  4. DrahtBot added the label Build system on Nov 25, 2020
  5. DrahtBot added the label GUI on Nov 25, 2020
  6. DrahtBot added the label P2P on Nov 25, 2020
  7. DrahtBot added the label RPC/REST/ZMQ on Nov 25, 2020
  8. DrahtBot added the label Utils/log/libs on Nov 25, 2020
  9. DrahtBot added the label UTXO Db and Indexes on Nov 25, 2020
  10. DrahtBot added the label Validation on Nov 25, 2020
  11. laanwj removed the label GUI on Nov 25, 2020
  12. laanwj removed the label P2P on Nov 25, 2020
  13. laanwj removed the label RPC/REST/ZMQ on Nov 25, 2020
  14. laanwj removed the label UTXO Db and Indexes on Nov 25, 2020
  15. laanwj removed the label Validation on Nov 25, 2020
  16. DrahtBot commented at 3:48 pm on November 25, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22956 (validation: log CChainState::CheckBlockIndex() consistency checks by jonatack)
    • #21943 (Dedup and RAII-fy the creation of a copy of CConnman::vNodes by vasild)
    • #21878 (Make all networking code mockable by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  17. jb55 commented at 6:58 pm on November 25, 2020: member
    Very cool, Concept ACK
  18. practicalswift force-pushed on Nov 25, 2020
  19. emilengler commented at 8:01 pm on November 26, 2020: contributor

    Concept ACK, however I have some things to improve:

    1. Why must it be a compile time feature? Can’t we just add a cmdline option called -enable-seccomp. See Chromiums --enable-unveil.
    2. Don’t be that seccomp specific in terms of autotools configuration and naming. Keep in mind that there is also pledge(2) on OpenBSD which could be a nice addition as well.
  20. in configure.ac:75 in 54df39b5c4 outdated
    66@@ -67,6 +67,16 @@ case $host in
    67   ;;
    68 esac
    69 
    70+AC_ARG_ENABLE([syscall-sandbox],
    71+  [AS_HELP_STRING([--enable-syscall-sandbox],
    


    emilengler commented at 6:05 am on November 27, 2020:
    Why not make syscomp opt-out? I think secure by default is a better idea :-)

    emilengler commented at 6:59 am on November 30, 2020:
    IIRC there was a discussion about something similar here #17419 However it is a bit weird and in transparent IMO to have things enabled in a dev build and others in a release build (beside some warnings of course).

    laanwj commented at 2:55 pm on March 16, 2021:
    I don’t think enabling it by default should be considered in the PR first introducing this. It is absolutely something that needs experimentation and testing and fine-tuning, in different environments.
  21. practicalswift force-pushed on Dec 15, 2020
  22. practicalswift force-pushed on Dec 17, 2020
  23. laanwj commented at 12:10 pm on December 17, 2020: member

    I’d suggest adding an old one but IIRC there hasn’t ever been an RCE like vulnerability that could be reintroduced.

    Maybe the UPnP vulnerability (TALOS-2015-0035, CVE-2015-6031)? I should still have a PoC exploit somewhere. I think downgrading UPnP should be enough to reintroduce it.

    I don’t currently have the time to play around with this but if someone is interested I can dig it up.

  24. in src/compat/seccomp-bpf.h:37 in dbad7bdb94 outdated
    32+#ifdef HAVE_LINUX_SECCOMP_H
    33+# include <linux/seccomp.h>
    34+#endif
    35+#ifndef SECCOMP_MODE_FILTER
    36+# define SECCOMP_MODE_FILTER    2 /* uses user-supplied filter. */
    37+# define SECCOMP_RET_KILL    0x00000000U /* kill the task immediately */
    


    practicalswift commented at 9:19 pm on December 20, 2020:

    Note to self:

    We should probably use SECCOMP_RET_KILL_PROCESS action (kill the entire process) instead of SECCOMP_RET_KILL_THREAD/SECCOMP_RET_KILL (kill the task) on syscall violation.

    The SECCOMP_RET_KILL_PROCESS action was introduced in Linux 4.14. Could read /proc/sys/kernel/seccomp/actions_avail to see available actions.

    Perhaps this feature should be possible to enable only for systems running Linux 4.14 or newer.

    Context: https://lkml.org/lkml/2017/8/11/807


    practicalswift commented at 3:13 pm on March 16, 2021:
    Now using SECCOMP_RET_KILL_PROCESS.
  25. laanwj commented at 9:35 am on January 14, 2021: member

    I don’t currently have the time to play around with this but if someone is interested I can dig it up.

    As people have asked, here you go: https://dev.visucore.com/bitcoin/2015_10_upnpc_poc.tar.xz

    It’s my entire working environment and contains my exploit (poc*.py), some notes, someone else’s exploit (upnp_exploit1.py), and some exploitable binaries.

    If you have any questions feel free to ask on IRC though I literally haven’t looked at it for 5 years.

  26. practicalswift force-pushed on Feb 10, 2021
  27. in src/util/syscall_sandbox.cpp:417 in 3bb52b6677 outdated
    416+    case SyscallSandboxPolicy::SHUTOFF: // Thread: main thread (state: shutoff)
    417+        seccomp_policy_builder.AllowFileSystem();
    418+        break;
    419+    }
    420+
    421+    const SyscallSandboxDefaultAction default_action = std::getenv("EXPERIMENTAL_SYSCALL_SANDBOX_MODE_KILL_THREAD") != nullptr ? SyscallSandboxDefaultAction::KILL_THREAD : SyscallSandboxDefaultAction::DEBUG_SIGNAL_HANDLER;
    


    practicalswift commented at 8:38 pm on February 10, 2021:

    Another self-review:

    Instead of opt-ing out of debug mode via an environment variable we should make it the other way around: the debug mode should be opt-in via --enable-debug.

    In other words: SECCOMP_RET_KILL_PROCESS should be the default action, and SECCOMP_RET_TRAP should only be used if compiled with --enable-debug.

    Background:

    When using SECCOMP_RET_KILL_PROCESS the kernel immediately kills off the offending process in case of a syscall violation.

    When using SECCOMP_RET_TRAP the kernel sends a SIGSYS signal to the offending process in case of a syscall violation: that allows us to print a user-friendly error message via a signal handler. Note that the attacker may be able to control the execution in the signal handler (although still with restricted syscall access), so this SECCOMP_RET_TRAP mode should really only be used when debugging.


    laanwj commented at 2:44 pm on March 16, 2021:
    Not sure on this. I do think feedback to the user is very important. I am slightly worried when if our syscall profile isn’t 100% up to date (my experience is that this can easily happen especially with dynamically linked glibc), something starts killing the process without any means to diagnose or debug, that’s extremely frustrating.

    practicalswift commented at 7:11 pm on March 16, 2021:

    Yes, I guess this is the classical trade-off between security vs debuggability/ease-of-use :)

    Making SECCOMP_RET_KILL_PROCESS the default is slightly better from a security perspective (no risk that the attacker is getting a “second chance” via the signal handler) whereas making SECCOMP_RET_TRAP the default is slightly better from a debuggability/ease-of-use perspective.

    I’ll let others chime in here: I’ll happily adjust to the consensus opinion :)

    The following shows the differences in output between the two modes of operation in case of a simulated syscall violation.

    When using ./configure --enable-syscall-sandbox:

    0$ make distclean
    1$ ./autogen.sh
    2$ ./configure --enable-syscall-sandbox
    3$ make
    4$ src/bitcoind
    52021-03-16T12:34:56Z Bitcoin Core version v21.99.0-[…]-dirty (release build)
    67Bad system call (core dumped)
    

    When using ./configure --enable-syscall-sandbox --enable-debug:

     0$ make distclean
     1$ ./autogen.sh
     2$ ./configure --enable-syscall-sandbox --enable-debug
     3$ make
     4$ src/bitcoind
     52021-03-16T12:34:56Z Bitcoin Core version v21.99.0-[…]-dirty (debug build)
     6 7ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report. Exiting.
     82021-03-16T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report. Exiting.
     9terminate called without an active exception
    10Aborted (core dumped)
    

    practicalswift commented at 11:44 am on May 15, 2021:

    @laanwj Now updated to default to debug mode as you suggested :)

    Updated OP to describe the two modes of operation:

    • Debug mode (enabled via configure flag --with-syscall-sandbox): If a non-allowlisted syscall is called a debug handler will be invoked. Debug info will be printed (ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report. Exiting.) and std::terminate is called. Note that the attacker may be able to control the execution in the signal handler (although still with restricted syscall access), so this mode should really only be used when debugging.
    • Kill without debug mode (enabled via configure flag --with-syscall-sandbox=kill-without-debug): If a non-allowlisted syscall is called the kernel will immediately kill the offending process.
  28. practicalswift force-pushed on Mar 12, 2021
  29. practicalswift force-pushed on Mar 16, 2021
  30. practicalswift renamed this:
    draft: Add syscall sandboxing using seccomp-bpf (Linux secure computing mode)
    Add syscall sandboxing using seccomp-bpf (Linux secure computing mode)
    on Mar 16, 2021
  31. practicalswift marked this as ready for review on Mar 16, 2021
  32. in configure.ac:1254 in af327aac2e outdated
    1230@@ -1220,7 +1231,7 @@ AC_LINK_IFELSE(
    1231     [ AC_MSG_RESULT(no) ]
    1232 )
    1233 
    1234-AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM], [std::system or ::wsystem])
    1235+AC_DEFINE([HAVE_SYSTEM], [(HAVE_STD__SYSTEM || HAVE_WSYSTEM) && !USE_SYSCALL_SANDBOX], [std::system or ::wsystem, and no syscall sandbox])
    


    laanwj commented at 3:45 pm on March 16, 2021:

    This makes sense I guess, I suppose we can think of a way to make external signing work within a sandbox, but the obvious and most secure is to disable the creation of external processes.

    As for notifications: just use ZMQ, or RPC based polling. No need to support process based notifications in sandbox mode imo, if you’re taking security seriously you wouldn’t expose that kind of escape hatch.

  33. practicalswift force-pushed on Mar 16, 2021
  34. practicalswift force-pushed on Mar 16, 2021
  35. practicalswift commented at 7:45 pm on March 23, 2021: contributor
    After a few months in draft mode this PR is now ready for code review: no longer marked as draft! :)
  36. practicalswift force-pushed on Mar 27, 2021
  37. practicalswift force-pushed on Mar 28, 2021
  38. practicalswift force-pushed on Mar 28, 2021
  39. jonatack commented at 2:28 pm on March 28, 2021: member
    Concept ACK, thanks for working on this!
  40. practicalswift force-pushed on Mar 29, 2021
  41. DrahtBot added the label Needs rebase on Apr 13, 2021
  42. practicalswift force-pushed on Apr 24, 2021
  43. DrahtBot removed the label Needs rebase on Apr 24, 2021
  44. practicalswift force-pushed on Apr 24, 2021
  45. practicalswift force-pushed on Apr 25, 2021
  46. practicalswift force-pushed on Apr 25, 2021
  47. practicalswift force-pushed on Apr 25, 2021
  48. practicalswift force-pushed on Apr 25, 2021
  49. practicalswift force-pushed on Apr 25, 2021
  50. practicalswift force-pushed on Apr 25, 2021
  51. practicalswift force-pushed on Apr 25, 2021
  52. practicalswift force-pushed on Apr 26, 2021
  53. practicalswift force-pushed on Apr 27, 2021
  54. DrahtBot added the label Needs rebase on Apr 27, 2021
  55. practicalswift force-pushed on Apr 27, 2021
  56. DrahtBot removed the label Needs rebase on Apr 27, 2021
  57. practicalswift force-pushed on Apr 28, 2021
  58. practicalswift force-pushed on Apr 28, 2021
  59. DrahtBot added the label Needs rebase on May 5, 2021
  60. practicalswift force-pushed on May 12, 2021
  61. DrahtBot removed the label Needs rebase on May 12, 2021
  62. DrahtBot added the label Needs rebase on May 15, 2021
  63. practicalswift force-pushed on May 15, 2021
  64. DrahtBot removed the label Needs rebase on May 15, 2021
  65. practicalswift force-pushed on May 16, 2021
  66. practicalswift force-pushed on May 17, 2021
  67. practicalswift commented at 7:27 pm on May 20, 2021: contributor
    @laanwj Thanks for reviewing. I believe all feedback has be addressed (sorry it took a while). Let me know if there is anything more I can do :)
  68. DrahtBot added the label Needs rebase on May 21, 2021
  69. 0xB10C commented at 11:40 am on May 21, 2021: member
    Concept ACK!
  70. practicalswift force-pushed on May 22, 2021
  71. DrahtBot removed the label Needs rebase on May 22, 2021
  72. fanquake added this to the "Blockers" column in a project

  73. ariard commented at 3:31 am on May 27, 2021: member
    Concept ACK, great work!
  74. in configure.ac:75 in 3636a5c1f0 outdated
    70@@ -71,6 +71,24 @@ case $host in
    71   ;;
    72 esac
    73 
    74+AC_ARG_WITH([syscall-sandbox],
    75+  [AS_HELP_STRING([--with-syscall-sandbox=yes|no|kill-without-debug],
    


    ryanofsky commented at 5:28 pm on June 1, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (3636a5c1f03fd01da2a78cad0cba7ab69cadf074)

    It seems like -sandbox=<mode> should be a runtime option, not a build option. This way we can build and one distribute one binary with experimental sandbox support that can be disabled or enabled in different modes, instead of having to build multiple binaries with hardcoded sandbox modes.

    There still would need to be a --with-seccomp option to be able to build on platforms without BPF, but the implementation and use of --with-seccomp should simpler than this option and not require complicating the build with system/boost process/multiprocess interactions.


    practicalswift commented at 10:25 pm on June 4, 2021:

    Great feedback. Thanks!

    I’ve now added a -sandbox=<mode> parameter. --with-syscall-sandbox has been renamed --with-sandbox.

    Available -sandbox modes are:

    • -sandbox=disallow-and-log (disallow unexpected syscalls and log the violation, then continue execution)
    • -sandbox=log-and-kill (log the violation and kill the process)
    • -sandbox=kill (kill the process immediately in case of a violation (without any logging))

    To enable the syscall sandbox --with-sandbox is required at compile time and -sandbox=<mode> needs to be specified at run time.

    This dual opt-in approach is chosen since this is an experimental feature. We can consider relaxing this in the future when this feature has matured and has been extensively tested in many different environments. Note that --with-sandbox=no (no sandbox support) is intentionally the default.

    There still would need to be a –with-seccomp option to be able to build on platforms without BPF, but the implementation and use of –with-seccomp should simpler than this option and not require complicating the build with system/boost process/multiprocess interactions.

    Adding -sandbox=<mode> allowed for a much simpler --with-sandbox, but note that the system/boost process/multiprocess interaction is still there and I’m not sure what the best approach would be if we were to get rid of it:

    0AC_MSG_ERROR(The options --with-sandbox (which does not allow execve) and --enable-external-signer (which uses execve) are currently incompatible.)
    12AC_MSG_ERROR(The options --with-sandbox (which does not allow execve) and --enable-multiprocess (which uses execve) are currently incompatible.)
    34AC_DEFINE([HAVE_SYSTEM], [(HAVE_STD__SYSTEM || HAVE_WSYSTEM) && !USE_SYSCALL_SANDBOX], [std::system or ::wsystem, and no syscall sandbox])
    

    Isn’t say --with-sandbox (no execve) and --enable-external-signer (yes execve) mutually exclusive by design? :)

    Or is the suggestion to InitError if any “execve/HAVE_SYSTEM depending” option is passed to bitcoind when -sandbox=<mode> is used?

    Also, note that --with-sandbox will make the functional test be run under the syscall sandbox (using -sandbox=log-and-kill). Thus if we allowed for say --with-sandbox --enable-external-signer then all external signer functional tests would need to be disabled, no?

    Please advice :)

  75. in src/util/syscall_sandbox.cpp:50 in 3636a5c1f0 outdated
    45+namespace {
    46+// The syscall sandbox feature is currently a Linux x86_64-only feature.
    47+std::string GetLinuxSyscallName(const uint32_t syscall_number)
    48+{
    49+    // Linux x86_64 syscalls listed in syscall number order without gaps.
    50+    static const std::vector<std::string> SYSCALL_NAMES{"read", "write", "open", "close", "stat", "fstat", "lstat", "poll", "lseek", "mmap", "mprotect", "munmap", "brk", "rt_sigaction", "rt_sigprocmask", "rt_sigreturn", "ioctl", "pread64", "pwrite64", "readv", "writev", "access", "pipe", "select", "sched_yield", "mremap", "msync", "mincore", "madvise", "shmget", "shmat", "shmctl", "dup", "dup2", "pause", "nanosleep", "getitimer", "alarm", "setitimer", "getpid", "sendfile", "socket", "connect", "accept", "sendto", "recvfrom", "sendmsg", "recvmsg", "shutdown", "bind", "listen", "getsockname", "getpeername", "socketpair", "setsockopt", "getsockopt", "clone", "fork", "vfork", "execve", "exit", "wait4", "kill", "uname", "semget", "semop", "semctl", "shmdt", "msgget", "msgsnd", "msgrcv", "msgctl", "fcntl", "flock", "fsync", "fdatasync", "truncate", "ftruncate", "getdents", "getcwd", "chdir", "fchdir", "rename", "mkdir", "rmdir", "creat", "link", "unlink", "symlink", "readlink", "chmod", "fchmod", "chown", "fchown", "lchown", "umask", "gettimeofday", "getrlimit", "getrusage", "sysinfo", "times", "ptrace", "getuid", "syslog", "getgid", "setuid", "setgid", "geteuid", "getegid", "setpgid", "getppid", "getpgrp", "setsid", "setreuid", "setregid", "getgroups", "setgroups", "setresuid", "getresuid", "setresgid", "getresgid", "getpgid", "setfsuid", "setfsgid", "getsid", "capget", "capset", "rt_sigpending", "rt_sigtimedwait", "rt_sigqueueinfo", "rt_sigsuspend", "sigaltstack", "utime", "mknod", "uselib", "personality", "ustat", "statfs", "fstatfs", "sysfs", "getpriority", "setpriority", "sched_setparam", "sched_getparam", "sched_setscheduler", "sched_getscheduler", "sched_get_priority_max", "sched_get_priority_min", "sched_rr_get_interval", "mlock", "munlock", "mlockall", "munlockall", "vhangup", "modify_ldt", "pivot_root", "_sysctl", "prctl", "arch_prctl", "adjtimex", "setrlimit", "chroot", "sync", "acct", "settimeofday", "mount", "umount2", "swapon", "swapoff", "reboot", "sethostname", "setdomainname", "iopl", "ioperm", "create_module", "init_module", "delete_module", "get_kernel_syms", "query_module", "quotactl", "nfsservctl", "getpmsg", "putpmsg", "afs_syscall", "tuxcall", "security", "gettid", "readahead", "setxattr", "lsetxattr", "fsetxattr", "getxattr", "lgetxattr", "fgetxattr", "listxattr", "llistxattr", "flistxattr", "removexattr", "lremovexattr", "fremovexattr", "tkill", "time", "futex", "sched_setaffinity", "sched_getaffinity", "set_thread_area", "io_setup", "io_destroy", "io_getevents", "io_submit", "io_cancel", "get_thread_area", "lookup_dcookie", "epoll_create", "epoll_ctl_old", "epoll_wait_old", "remap_file_pages", "getdents64", "set_tid_address", "restart_syscall", "semtimedop", "fadvise64", "timer_create", "timer_settime", "timer_gettime", "timer_getoverrun", "timer_delete", "clock_settime", "clock_gettime", "clock_getres", "clock_nanosleep", "exit_group", "epoll_wait", "epoll_ctl", "tgkill", "utimes", "vserver", "mbind", "set_mempolicy", "get_mempolicy", "mq_open", "mq_unlink", "mq_timedsend", "mq_timedreceive", "mq_notify", "mq_getsetattr", "kexec_load", "waitid", "add_key", "request_key", "keyctl", "ioprio_set", "ioprio_get", "inotify_init", "inotify_add_watch", "inotify_rm_watch", "migrate_pages", "openat", "mkdirat", "mknodat", "fchownat", "futimesat", "newfstatat", "unlinkat", "renameat", "linkat", "symlinkat", "readlinkat", "fchmodat", "faccessat", "pselect6", "ppoll", "unshare", "set_robust_list", "get_robust_list", "splice", "tee", "sync_file_range", "vmsplice", "move_pages", "utimensat", "epoll_pwait", "signalfd", "timerfd_create", "eventfd", "fallocate", "timerfd_settime", "timerfd_gettime", "accept4", "signalfd4", "eventfd2", "epoll_create1", "dup3", "pipe2", "inotify_init1", "preadv", "pwritev", "rt_tgsigqueueinfo", "perf_event_open", "recvmmsg", "fanotify_init", "fanotify_mark", "prlimit64", "name_to_handle_at", "open_by_handle_at", "clock_adjtime", "syncfs", "sendmmsg", "setns", "getcpu", "process_vm_readv", "process_vm_writev", "kcmp", "finit_module", "sched_setattr", "sched_getattr", "renameat2", "seccomp", "getrandom", "memfd_create", "kexec_file_load", "bpf", "execveat", "userfaultfd", "membarrier", "mlock2", "copy_file_range", "preadv2", "pwritev2", "pkey_mprotect", "pkey_alloc", "pkey_free", "statx"};
    


    ryanofsky commented at 5:51 pm on June 1, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (3636a5c1f03fd01da2a78cad0cba7ab69cadf074)

    Not important, but instead of hardcoding the syscall order for one platform in a packed vector, it could be less fragile to populate a std::map<int, std::string> of the syscalls using SYS or __NR macros, see https://stackoverflow.com/questions/23249373/how-to-obtain-linux-syscall-name-from-the-syscall-number, https://unix.stackexchange.com/questions/338650/why-are-linux-system-call-numbers-in-x86-and-x86-64-different. It should also cut back the record breaking 4301 character line length.


    practicalswift commented at 4:15 pm on June 4, 2021:

    Good point! Now addressed.

    It should also cut back the record breaking 4301 character line length.

    :D

  76. in src/bitcoin-cli.cpp:1029 in 3636a5c1f0 outdated
    1024@@ -1024,6 +1025,8 @@ __declspec(dllexport) int main(int argc, char* argv[])
    1025 int main(int argc, char* argv[])
    1026 {
    1027 #endif
    1028+    EnableSyscallSandbox(SyscallSandboxPolicy::INITIALIZATION);
    1029+    DisableFurtherSyscallSandboxRestrictions();
    


    ryanofsky commented at 5:55 pm on June 1, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (3636a5c1f03fd01da2a78cad0cba7ab69cadf074)

    I don’t understand why these DisableFurtherSyscallSandboxRestrictions calls are added to so many binaries. It seems like they could significantly lessen the effects of sandboxing. It might make sense if there were TODO comments saying what needs to be done to remove these, or just general comments explaining why they are needed.


    practicalswift commented at 4:52 pm on June 4, 2021:

    The DisableFurtherSyscallSandboxRestrictions is (or rather: was) used to make any further/future EnableSyscallSandbox calls that be into no-ops.

    In other words to “lock” the sandbox to the sandbox policy currently loaded. (Or disable the sandbox if no sandbox is loaded: in other words a call to DisableFurtherSyscallSandboxRestrictions without any preceding EnableSyscallSandbox.)

    The quoted code above for example loads the SyscallSandboxPolicy::INITIALIZATION policy on bitcoin-cli startup and then locks that policy so that no matter what functions that bitcoin-cli may call that uses EnableSyscallSandbox we’re guaranteed to run under SyscallSandboxPolicy::INITIALIZATION.

    SyscallSandboxPolicy::INITIALIZATION is the super-set of all sandbox policies in Bitcoin Core, so the code above basically says “limit me to the syscall set we expect to be used in (any part of) Bitcoin Core, but make sure no further syscall sandbox restrictions are done in code I may call” (in other words: ignore any upcoming EnableSyscallSandbox calls).

    The above is how it used to be, and I agree that was a bit confusing, so I’ve now updated things:

    Instead of enabling the sandbox on first call to void EnableSyscallSandbox(policy) I’ve now added a separate setup function bool SetupSyscallSandbox(…) (called at most once per binary) to enable the sandbox, and added a separate void SetSyscallSandboxPolicy(policy) to load a specific policy.

    In other words the sandbox is opt-in via bool SetupSyscallSandbox(…).

    A call to SetupSyscallSandbox(…) is made in bitcoind if -sandbox=<mode> is used.

    In this PR I’m now limiting the syscall sandbox support to the bitcoind binary to keep things focused. I suggest we wait with the other binaries to keep things focused. We can gradually enable the sandbox for other binaries over time in future PR:s as we see fit when we’ve gained some experience with the experimental bitcoind syscall sandbox support. Sounds reasonable? :)

  77. in src/util/syscall_sandbox.cpp:76 in 3636a5c1f0 outdated
    71+    std::terminate();
    72+}
    73+
    74+void InstallSyscallSandboxDebugHandler()
    75+{
    76+    static std::atomic<bool> syscall_reporter_installed{false};
    


    ryanofsky commented at 6:06 pm on June 1, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (3636a5c1f03fd01da2a78cad0cba7ab69cadf074)

    Use of std::atomic here does not seem to make sense. If multiple threads can call InstallSyscallSandboxDebugHandler for the first time simultaneously, then a Mutex and LOCK need to be used to prevent the later threads from returning before the first thread finishes calling sigaction and sigprocmask. If multiple threads can’t call InstallSyscallSandboxDebugHandler for the first time simultaneously, then this can just be a normal bool instead of an atomic bool.


    practicalswift commented at 9:22 pm on June 4, 2021:
    Addressed: in the updated code there is no need for locking at all since the setup is done in SetupSyscallSandbox which is guaranteed to only be called once. And you’re absolutely right that std::atomic<bool> did not make much sense here :)
  78. in src/util/syscall_sandbox.cpp:88 in 3636a5c1f0 outdated
    83+    sigemptyset(&mask);
    84+    sigaddset(&mask, SIGSYS);
    85+    action.sa_sigaction = &SyscallSandboxDebugSignalHandler;
    86+    action.sa_flags = SA_SIGINFO;
    87+    if (sigaction(SIGSYS, &action, nullptr) < 0) {
    88+        perror("sigaction");
    


    ryanofsky commented at 6:10 pm on June 1, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (3636a5c1f03fd01da2a78cad0cba7ab69cadf074)

    Throughout this file is there a reason error handling code can’t use normal InitError or LogPrintf functions or raise exceptions, to send errors to debug.log and the GUI instead of bypassing them and going to stderr?


    practicalswift commented at 9:26 pm on June 4, 2021:
    Good point. The initialization/setup logic has now been moved to init.cpp and it is now using the expected InitError and LogPrintf. SetSyscallSandboxPolicy is now throwing std::runtime_error in case of errors.
  79. ryanofsky approved
  80. ryanofsky commented at 6:20 pm on June 1, 2021: member

    Very light code review ACK 3636a5c1f03fd01da2a78cad0cba7ab69cadf074 (I didn’t review bpf code in detail yet) and concept ACK.

    Great work here! This seems like a really nice feature.

  81. practicalswift force-pushed on Jun 2, 2021
  82. practicalswift force-pushed on Jun 2, 2021
  83. practicalswift force-pushed on Jun 3, 2021
  84. practicalswift force-pushed on Jun 4, 2021
  85. practicalswift force-pushed on Jun 5, 2021
  86. emilengler commented at 8:27 am on June 8, 2021: contributor

    I recently thought about this again and found the following information interesting, not sure if it was already pointed out:

    A further issue with Linux sandboxes in particular (seccomp(2) and friends) is due to the instability of the Linux ecosystem itself. Generic libc functions are implemented differently depending on whether you’re using Alpine (musl) or Debian (glibc). This means that the same libc function may require different system calls.

    While this is about C, it also applies to C++ in many parts. There’s no guarantee that the current approaches works on all libc++ implementation. Also there is no guarantee that for example clangs libc++ might use different syscalls in future versions. This could lead to an unsustainable and unportable design of the implementation.

    Maybe it’s better focusing on BSDs sandboxing mechanisms (pledge(2) and capsicum(2)) instead as they are more somewhat stable compared to Linux.

  87. emilengler commented at 1:20 pm on June 8, 2021: contributor

    Maybe it’s better focusing on BSDs sandboxing mechanisms (pledge(2) and capsicum(2)) instead as they are more somewhat stable compared to Linux.

    Turns out this won’t work either due to third party libs including the C++ standard library still have that issue pointed out above. Maybe focus on FS restriction instead rather than syscall restriction

  88. practicalswift commented at 1:45 pm on June 9, 2021: contributor

    @emilengler

    The syscall sandbox works on an allowlist basis. Thus if the kernel provides two different syscalls to achieve a certain action, and both those syscalls are used by the different standard library implementations then both syscalls should be put in the allowlist (if we want to allow said action).

    That is inherent in all types of syscall sandboxing designs, no matter if Linux seccomp-bpf or some BSD equivalent is used.

    As have been noted in previous discussions above syscall sandboxing is a type of feature that needs experimentation and fine-tuning for different environments to catch potential diversity in syscall use across systems. Such experimentation was required when sandboxing using seccomp-bpf was introduced in projects such as Chromium, OpenSSH and Tor, and it will be required for Bitcoin Core too. There is really no way around it :)

    That’s why this is labeled an experimental feature that is disabled by default, and is enabled only after “dual opt-in” (compile-time opt-in via --with-sandbox and run-time opt-in via -sandbox). To be clear: this is very much not a feature for the average end-user at this stage :)

    I think having this feature in CI only would be a very good start: that would allow us to catch introduction of crazy syscall usage (say opening a network connection in a thread that is not supposed to do so, etc.), and get a chance to play with sandboxing in practice.

    FWIW I’ve been using this feature for roughly a year in different environments. I’ve found this feature to be a very helpful testing/debugging addition which makes it much easier to reason about the actual capabilities required of each type of thread in Bitcoin Core.

  89. practicalswift force-pushed on Jun 11, 2021
  90. practicalswift force-pushed on Jun 12, 2021
  91. DrahtBot added the label Needs rebase on Jun 16, 2021
  92. practicalswift force-pushed on Jun 22, 2021
  93. practicalswift force-pushed on Jun 22, 2021
  94. DrahtBot removed the label Needs rebase on Jun 22, 2021
  95. practicalswift force-pushed on Jun 22, 2021
  96. practicalswift force-pushed on Jun 22, 2021
  97. ryanofsky commented at 6:07 pm on June 28, 2021: member

    Thanks for all the followups! The new Setup call is easier to understand, and it’s great to see configuration moved from build time to runtime so there is more usage flexibility and less chaos in the build script.

    Main remaining suggestion would be to rename --with-sandbox option to --with-seccomp to follow more a traditional autoconf convention of using --with-* options for dependencies and --enable-* options for features. This would also be more futureproof for supporting alternative sandboxing mechanisms on linux, and different sandboxing mechanisms on other platforms.

    Other suggestions:

    • The --with- option should just be automatically detected like other dependencies, and not required to be specified manually. Sandboxing is experimental, but it’s already disabled by default, and doesn’t need to be disabled twice as much. Autodetection would also allow dropping the cirrus CI changes in this PR and avoid the new sandboxing feature creating a new build and test variant.
    • The --with-sandbox and --enable-multiprocess are currently incompatible build error would be good to drop. The multiprocess and sandboxing features for now maybe don’t work together at runtime, but there’s no build conflict.
    • Various #if defined(USE_SYSCALL_SANDBOX) conditionals seem unnecessary. It seems like the only one actually needed is in util/syscall_sandbox.cpp
    • There should probably be a TestDisallowedSandboxCall or similar function in syscall_sandbox.cpp to avoid platform-specific code in rpc/misc.cpp
    • The skip_if_bitcoind_syscall_sandbox() function in the python framework would should probably be replaced by a disable_syscall_sandbox() function. It’s nice how this PR changes the test framework to automatically pass -sandbox=log-and-kill to bitcoind. But for the tests that don’t work with sandboxing, it would be better to just not pass the sandboxing option instead of skipping the test entirely.

    Light code review ACK and approach ACK 3131ec1fbb3db5d81502bcf349d20eb28ddb0ca4.

  98. DrahtBot added the label Needs rebase on Jun 29, 2021
  99. practicalswift force-pushed on Jun 29, 2021
  100. DrahtBot removed the label Needs rebase on Jun 29, 2021
  101. in configure.ac:77 in 3535c27eb8 outdated
    70@@ -71,6 +71,12 @@ case $host in
    71   ;;
    72 esac
    73 
    74+AC_ARG_WITH([seccomp],
    75+  [AS_HELP_STRING([--with-seccomp],
    76+  [enable experimental syscall sandbox feature (default is yes if seccomp-bpf is detected under Linux x86_64)])],
    77+  [use_syscall_sandbox=$withval],
    


    ryanofsky commented at 4:11 pm on July 2, 2021:

    For a little more legibility here would suggest:

    • Replacing current uses of use_syscall_sandbox with seccomp_found in --with-seccomp code
    • Before the AM_CONDITIONAL, adding
    0dnl Currently only enable -sandbox=<mode> feature if seccomp is found.
    1dnl In the future, sandboxing could be also be supported with other
    2dnl sandboxing mechanisms besides seccomp
    3use_syscall_sandbox=$seccomp_found
    

    practicalswift commented at 7:26 am on July 8, 2021:
    Good idea! Fixed! :)
  102. in src/init.cpp:565 in 3535c27eb8 outdated
    560@@ -560,6 +561,10 @@ void SetupServerArgs(ArgsManager& argsman)
    561     hidden_args.emplace_back("-daemonwait");
    562 #endif
    563 
    564+#if defined(USE_SYSCALL_SANDBOX)
    565+    argsman.AddArg("-sandbox=<mode>", "Use the experimental syscall sandbox in the specified mode (\"disallow-and-log\", \"log-and-kill\" or \"kill\"). Allow only allowlisted (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 \"disallow-and-log\" mode an invokation of an unexpected syscall will be disallowed and logged. In the \"log-and-kill\" mode an invokation of an unexpected syscall will terminate bitcoind after logging the syscall violation. In the \"kill\" mode an invokation of an unexpected syscall will cause bitcoind to be killed immediately by the kernel.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    


    ryanofsky commented at 4:26 pm on July 2, 2021:

    Do the syscalls actually fail in the “disallow-and-log” mode? Would be good to describe it in the help a little more, or change it to a more suggestive name like “log-and-fail” if it does fail, or something like “log” or “log-warning” if it doesn’t fail.

    Also should s/invokation/invocation/


    practicalswift commented at 9:35 pm on July 8, 2021:

    Great feedback!

    I’ve decided to remove the disallow-and-log mode: I think the modes log-and-kill and kill should be more than enough (to start with at least).

    disallow-and-log was meant to disallow the invoked unexpected syscall (by not executing it), log the attempted syscall invocation and then continue execution. Note however that continued normal execution cannot be guaranteed: the exact behaviour depends on the specific error handling at the syscall invocation call site. In some cases a failed syscall will be handled gracefully and in some places it won’t. The “may or may not continue execution” part makes this mode potentially very confusing in practice so I think it makes sense to simply remove it for now.

    The typo has been fixed. Thanks!

  103. ryanofsky commented at 4:31 pm on July 2, 2021: member
    Updates look good! I started reviewing this in more detail even though more changes may be in progress. Added a few new suggestions below.
  104. practicalswift force-pushed on Jul 7, 2021
  105. practicalswift force-pushed on Jul 7, 2021
  106. practicalswift force-pushed on Jul 8, 2021
  107. practicalswift force-pushed on Jul 8, 2021
  108. practicalswift force-pushed on Jul 8, 2021
  109. practicalswift force-pushed on Jul 8, 2021
  110. practicalswift force-pushed on Jul 8, 2021
  111. practicalswift force-pushed on Jul 9, 2021
  112. practicalswift force-pushed on Jul 9, 2021
  113. practicalswift force-pushed on Jul 9, 2021
  114. practicalswift force-pushed on Jul 11, 2021
  115. practicalswift force-pushed on Jul 11, 2021
  116. practicalswift force-pushed on Jul 11, 2021
  117. practicalswift commented at 9:12 am on July 13, 2021: contributor

    Thanks a lot for reviewing @ryanofsky: excellent feedback (as always). The suggested changes simplified the implementation significantly.

    I believe all comments have been addressed, and I think this PR should be ready for final review and testing :)

  118. in ci/test/00_setup_env_native_multiprocess.sh:14 in 333392fbb8 outdated
    10@@ -11,7 +11,8 @@ export DOCKER_NAME_TAG=ubuntu:20.04
    11 export PACKAGES="cmake python3 python3-pip llvm clang"
    12 export DEP_OPTS="DEBUG=1 MULTIPROCESS=1"
    13 export GOAL="install"
    14-export BITCOIN_CONFIG="--enable-debug CC=clang CXX=clang++"  # Use clang to avoid OOM
    15+# The multiprocess feature is currently incompatible with the experimental syscall sandbox feature (-sandbox=<mode>).
    


    ryanofsky commented at 8:52 pm on July 21, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    Both sandboxing and multiprocess features are runtime options that can be chosen when starting bitcoin, so ideally this would not be treated as build time conflict.

    Ideally could drop this --without-seccomp flag and only disable sandboxing at runtime if is needed, but I guess this would test changes, something like a test_runner sandbox option and:

    0export TEST_RUNNER_EXTRA="--nosandbox"
    

    practicalswift commented at 8:34 pm on July 31, 2021:

    I’ve now added an--nosandbox test runner option, and export TEST_RUNNER_EXTRA="--nosandbox" for this job.

    What would be the best way to detect usage of the multiprocess feature at runtime in AppInitParameterInteraction?


    ryanofsky commented at 0:43 am on August 18, 2021:

    re: #20487 (review)

    What would be the best way to detect usage of the multiprocess feature at runtime in AppInitParameterInteraction?

    You could add a bool ipc_enabled parameter to AppInitParameterInteraction and pass node.init->ipc() as the argument value where it’s called in AppInit, and false as the argument value where it’s called by the test framework.

    Handling this in AppInitParameterInteraction shouldn’t strictly be necessary to make tests pass, but it could be used to print a more helpful error message if there’s an attempt to use bitcoin-node IPC functionality when sandboxing is enabled. The echoipc RPC method is the only IPC functionality that’s merged so far, so it would be nice to make this change, but it could also be done later with #10102.

  119. in configure.ac:1458 in 333392fbb8 outdated
    1431+dnl The sanitizers introduce use of syscalls that are not typically used in bitcoind
    1432+dnl (such as execve when the sanitizers execute llvm-symbolizer).
    1433+if test x$use_sanitizers != x; then
    1434+    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>).)
    1435+   seccomp_found=no
    1436+fi
    


    ryanofsky commented at 8:58 pm on July 21, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    Do you think this will be necessary permanently? It seems like more ideally sandbox would be able to adjust itself to work with sanitizers, or give a descriptive runtime error if using -sandbox with some sanitizer mode is incompatible.


    practicalswift commented at 8:39 pm on July 31, 2021:
    I’m planning on tackling sanitizer compatibility in a follow-up PR. It is not entirely trivial since sanitizer use is not easy to detect at runtime (AFAIK) and the different sanitizers introduce the use of different sets of syscalls, but it should hopefully be doable some way. However, I prefer keeping that outside of this PR to keep things focused on the “standard” non-sanitizer case first :)
  120. in src/init.cpp:1032 in 333392fbb8 outdated
    1027+            "-signer",
    1028+            "-startupnotify",
    1029+            "-walletnotify",
    1030+        };
    1031+        for (const std::string& feature_using_execve : features_using_execve) {
    1032+            if (args.IsArgSet(feature_using_execve)) {
    


    ryanofsky commented at 9:21 pm on July 21, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    Should change this to if (!args.GetArg(feature_using_execve, "")).empty()), because IsArgSet won’t do the right thing for negated arguments.


    practicalswift commented at 4:47 pm on July 31, 2021:
    Thanks! Now fixed.
  121. in src/init.cpp:1014 in 333392fbb8 outdated
    1009@@ -1005,6 +1010,36 @@ bool AppInitParameterInteraction(const ArgsManager& args)
    1010         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
    1011     }
    1012 
    1013+#if defined(USE_SYSCALL_SANDBOX)
    1014+    if (args.IsArgSet("-sandbox")) {
    


    ryanofsky commented at 8:21 pm on July 26, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    Should probably add && !args.IsArgNegated("-sandbox")


    practicalswift commented at 4:47 pm on July 31, 2021:
    Thanks! Now fixed.
  122. in src/util/syscall_sandbox.cpp:25 in 333392fbb8 outdated
    20+
    21+#include <logging.h>
    22+#include <tinyformat.h>
    23+#include <util/threadnames.h>
    24+
    25+#if defined(USE_SYSCALL_SANDBOX)
    


    ryanofsky commented at 8:27 pm on July 26, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    USE_SYSCALL_SANDBOX macro seems to be used inconsistently in this file, some functions are undefined, some are undefined but don’t do anything. Clean thing to do would seem to remove all uses of USE_SYSCALL_SANDBOX in this file and just not compile it when sandboxing is not supported in the build.


    practicalswift commented at 6:51 pm on July 31, 2021:

    I don’t think not compiling it is an option since void SetSyscallSandboxPolicy(SyscallSandboxPolicy syscall_policy) must be present regardless of USE_SYSCALL_SANDBOX (to avoid ifdef:s at the various call sites).

    If USE_SYSCALL_SANDBOX is defined we currently need:

    0enum class SyscallSandboxPolicy;
    1void SetSyscallSandboxPolicy(SyscallSandboxPolicy syscall_policy);
    2[[nodiscard]] bool SetupSyscallSandbox(bool log_syscall_violation_before_terminating);
    3void TestDisallowedSandboxCall();
    

    If USE_SYSCALL_SANDBOX is not defined we only need:

    0enum class SyscallSandboxPolicy;
    1void SetSyscallSandboxPolicy(SyscallSandboxPolicy syscall_policy);
    

    In the latter case SetSyscallSandboxPolicy(SyscallSandboxPolicy syscall_policy) is a no-op.

    I’ve now simplified the macro use so that in the case of USE_SYSCALL_SANDBOX not being defined we get:

     0$ cpp -I src/ src/util/syscall_sandbox.cpp | grep -vE '^(#|$)'
     1enum class SyscallSandboxPolicy {
     2    INITIALIZATION,
     3    INITIALIZATION_DNS_SEED,
     4    INITIALIZATION_LOAD_BLOCKS,
     5    INITIALIZATION_MAP_PORT,
     6    INITIALIZATION_TOR_CONTROL,
     7    MESSAGE_HANDLER,
     8    NET,
     9    NET_ADD_CONNECTION,
    10    NET_HTTP_SERVER,
    11    NET_HTTP_SERVER_WORKER,
    12    NET_OPEN_CONNECTION,
    13    SCHEDULER,
    14    TX_INDEX,
    15    VALIDATION_SCRIPT_CHECK,
    16    SHUTOFF,
    17};
    18void SetSyscallSandboxPolicy(SyscallSandboxPolicy syscall_policy);
    19void SetSyscallSandboxPolicy(SyscallSandboxPolicy syscall_policy)
    20{
    21}
    

    Another option would be to add #if defined at all call sites along the lines of:

    0#if defined(USE_SYSCALL_SANDBOX)
    1SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET);
    2#endif // defined(USE_SYSCALL_SANDBOX)
    

    These are the call sites:

     0git grep SetSyscallSandbox -- ":(exclude)src/util/"
     1src/bitcoind.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::SHUTOFF);
     2src/checkqueue.h:                SetSyscallSandboxPolicy(SyscallSandboxPolicy::VALIDATION_SCRIPT_CHECK);
     3src/httpserver.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_HTTP_SERVER);
     4src/httpserver.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_HTTP_SERVER_WORKER);
     5src/index/base.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::TX_INDEX);
     6src/mapport.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION_MAP_PORT);
     7src/net.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET);
     8src/net.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION_DNS_SEED);
     9src/net.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_OPEN_CONNECTION);
    10src/net.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET_ADD_CONNECTION);
    11src/net.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::MESSAGE_HANDLER);
    12src/node/blockstorage.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION_LOAD_BLOCKS);
    13src/scheduler.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::SCHEDULER);
    14src/torcontrol.cpp:    SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION_TOR_CONTROL);
    

    WDYT? :)


    ryanofsky commented at 1:06 am on August 18, 2021:

    re: #20487 (review)

    WDYT? :)

    I like your new solution, minimizing ifdefs in the implementation while also not requiring them at call sites. Conceivably syscall_sandbox could be split up multiple files to avoid using macros, but there wouldn’t be a benefit with just one current sandboxing implementation.

  123. in src/util/syscall_sandbox.cpp:43 in 333392fbb8 outdated
    38+#define syscall_nr (offsetof(struct seccomp_data, nr))
    39+#define arch_nr (offsetof(struct seccomp_data, arch))
    40+#if defined(__x86_64__)
    41+#define REG_SYSCALL REG_RAX
    42+#define ARCH_NR AUDIT_ARCH_X86_64
    43+#else // defined(__x86_64__)
    


    ryanofsky commented at 10:54 pm on July 26, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    Some of this preprocessor magic seems strange. Can you maybe add a comment explaining, or linking to documentation?


    practicalswift commented at 4:48 pm on July 31, 2021:
    Good point. I’ve now cut down on the preprocessor use, added comments and documentation links. Hopefully this should be more clear now :)
  124. in src/util/syscall_sandbox.cpp:408 in 333392fbb8 outdated
    403+    if (info->si_code != SYS_SECCOMP) {
    404+        return;
    405+    }
    406+    const ucontext_t* ctx = static_cast<ucontext_t*>(void_context);
    407+    if (ctx == nullptr) {
    408+        return;
    


    ryanofsky commented at 10:57 pm on July 26, 2021:

    In commit “Add syscall sandboxing (seccomp-bpf)” (333392fbb886c56b62446490a81610ee821006e7)

    Are these two return conditions ever expected? It would be helpful to have a comment saying one way or the other, or maybe replacing these with assert statements if they are never expected.


    practicalswift commented at 4:50 pm on July 31, 2021:
    Good point: they are never expected. Now assert:ing instead to make our expectations clear. Thanks!
  125. ryanofsky approved
  126. ryanofsky commented at 11:18 pm on July 26, 2021: member

    Code review ACK 333392fbb886c56b62446490a81610ee821006e7 modulo BuildFilter and SetSyscallSandboxPolicy functions which do seem good as far as I can tell, but I don’t know enough yet to vouch for what they are doing in detail.

    This really does look great! It’s a very tidy and self-contained implementation. I left a few comments below but they are all minor and could be ignored.

  127. practicalswift force-pushed on Jul 29, 2021
  128. practicalswift force-pushed on Jul 31, 2021
  129. practicalswift force-pushed on Jul 31, 2021
  130. practicalswift force-pushed on Jul 31, 2021
  131. practicalswift force-pushed on Jul 31, 2021
  132. practicalswift force-pushed on Jul 31, 2021
  133. practicalswift force-pushed on Jul 31, 2021
  134. practicalswift commented at 8:38 pm on August 5, 2021: contributor

    I believe all feedback has been addressed: let me know if there is anything more I can do here :)

    Concept ACK status:

  135. practicalswift force-pushed on Aug 15, 2021
  136. ryanofsky approved
  137. ryanofsky commented at 3:10 pm on August 18, 2021: member

    Code review ACK 4646392e628ffd958d18d32cfdc44e53290de017 again with caveat that I’m not very familiar with BPF, and wouldn’t know if there was a subtle bug in the sandboxing implementation, even though the construction is pretty straightforward and makes sense as far as I can tell. Outside of the BPF filter construction, all the other build, test, config parsing and thread setup changes do look very good.

    Changes since last review: leaving sandboxing support in multiprocess build enabled (just disabling it specifically when needed at runtime), fixing handling of negated config options, adding many new explanatory comments and links in syscall_sandbox, adding new asserts, removing macro usages, tweaking BPF filter build code without changing the filter, adding python test –nosandbox option to be able to run tests without sandboxing.

  138. in src/util/syscall_sandbox.cpp:512 in 4646392e62 outdated
    481+        AllowUmask();
    482+    }
    483+
    484+    void AllowAddressSpaceAccess()
    485+    {
    486+        allowed_syscalls.insert(__NR_brk);     // change data segment size
    


    GeneFerneau commented at 8:53 pm on August 25, 2021:
    0        allowed_syscalls.emplace(__NR_brk);     // change data segment size
    

    GeneFerneau commented at 8:54 pm on August 25, 2021:
    Similar for other insertions to allowed_syscalls

    practicalswift commented at 2:07 pm on September 5, 2021:
    Note that __NR_brk is uint32_t so I don’t think there is any performance reason to use emplace here.
  139. in src/util/syscall_sandbox.cpp:724 in 4646392e62 outdated
    693+    // This function largely follows <https://outflux.net/teach-seccomp/step-3/seccomp-bpf.h>.
    694+    std::vector<sock_filter> BuildFilter(SyscallSandboxAction default_action)
    695+    {
    696+        std::vector<sock_filter> bpf_policy;
    697+        // See VALIDATE_ARCHITECTURE in seccomp-bpf.h referenced above.
    698+        bpf_policy.push_back(BPF_STMT(BPF_LD + BPF_W + BPF_ABS, offsetof(struct seccomp_data, arch)));
    


    GeneFerneau commented at 8:56 pm on August 25, 2021:
    0        bpf_policy.emplace_back(BPF_STMT(BPF_LD + BPF_W + BPF_ABS, offsetof(struct seccomp_data, arch)));
    

    Similar for other push_back calls to bpf_policy


    practicalswift commented at 2:10 pm on September 5, 2021:
    That doesn’t compile :)
  140. GeneFerneau commented at 9:06 pm on August 25, 2021: none

    Approach ACK 4646392

    Other than a few minor suggestions, everything looks great! Like @ryanofsky, I’m relatively new to seccomp, but from what I’ve read, looks good to me. Will keep reading up on seccomp, and do a more in-depth review.

    Not necessarily needed in this PR: what are your thoughts on #ifdefing x86_64 specific seccomp features/syscalls for easier support of other platforms like ARM and MIPS?

  141. practicalswift commented at 3:31 pm on September 5, 2021: contributor

    Approach ACK 4646392

    Thanks for reviewing!

    Not necessarily needed in this PR: what are your thoughts on #ifdefing x86_64 specific seccomp features/syscalls for easier support of other platforms like ARM and MIPS?

    The entire feature is currently #ifdef:ed on x86_64. That can be made more fine-grained in the future if/when support for this feature to the first non-x86_64 platform is added :)

  142. DrahtBot added the label Needs rebase on Sep 6, 2021
  143. practicalswift force-pushed on Sep 10, 2021
  144. DrahtBot removed the label Needs rebase on Sep 10, 2021
  145. practicalswift force-pushed on Sep 11, 2021
  146. practicalswift force-pushed on Sep 11, 2021
  147. practicalswift force-pushed on Sep 18, 2021
  148. in src/init.cpp:567 in 47473f8402 outdated
    562@@ -562,6 +563,10 @@ void SetupServerArgs(ArgsManager& argsman)
    563     hidden_args.emplace_back("-daemonwait");
    564 #endif
    565 
    566+#if defined(USE_SYSCALL_SANDBOX)
    567+    argsman.AddArg("-sandbox=<mode>", "Use the experimental syscall sandbox in the specified mode (-sandbox=log-and-kill or -sandbox=kill). Allow only allowlisted (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-kill\" 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 \"kill\" 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);
    


    laanwj commented at 6:37 pm on September 20, 2021:
    Would suggest shortening “Allow only allowlisted (expected)…” to “Allow only expected…”

    practicalswift commented at 3:22 pm on September 30, 2021:
    Makes sense. Fixed!
  149. in src/init.cpp:1032 in 47473f8402 outdated
    1027+    if (args.IsArgSet("-sandbox") && !args.IsArgNegated("-sandbox")) {
    1028+        const std::string sandbox_arg{args.GetArg("-sandbox", "")};
    1029+        bool log_syscall_violation_before_terminating{false};
    1030+        if (sandbox_arg == "log-and-kill") {
    1031+            log_syscall_violation_before_terminating = true;
    1032+        } else if (sandbox_arg == "kill") {
    


    laanwj commented at 6:38 pm on September 20, 2021:
    Please add a comment in here, leaving this block empty looks as if something is missing.

    practicalswift commented at 3:22 pm on September 30, 2021:
    Makes sense. Fixed!
  150. in src/init.cpp:1030 in 47473f8402 outdated
    1022@@ -1018,6 +1023,36 @@ bool AppInitParameterInteraction(const ArgsManager& args)
    1023         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
    1024     }
    1025 
    1026+#if defined(USE_SYSCALL_SANDBOX)
    1027+    if (args.IsArgSet("-sandbox") && !args.IsArgNegated("-sandbox")) {
    1028+        const std::string sandbox_arg{args.GetArg("-sandbox", "")};
    1029+        bool log_syscall_violation_before_terminating{false};
    1030+        if (sandbox_arg == "log-and-kill") {
    


    laanwj commented at 6:42 pm on September 20, 2021:
    I’d personally prefer “log-and-abort” and “abort” as options instead of “log-and-kill” and “kill”. I know it’s not a translated message, but it’s slightly friendlier anyhow.

    practicalswift commented at 3:22 pm on September 30, 2021:
    Good idea: “abort” sounds better than “kill”. Fixed!
  151. in src/util/syscall_sandbox.h:14 in 47473f8402 outdated
     9+    // 1. Initialization
    10+    INITIALIZATION,
    11+    INITIALIZATION_DNS_SEED,
    12+    INITIALIZATION_LOAD_BLOCKS,
    13+    INITIALIZATION_MAP_PORT,
    14+    INITIALIZATION_TOR_CONTROL,
    


    laanwj commented at 6:47 pm on September 20, 2021:
    I don’t think Torcontrol should be under “initialization”, it’s always active and keeps connecting to Tor and spinning up its onion service as long as bitcoind is running.

    practicalswift commented at 3:23 pm on September 30, 2021:
    Makes sense. Fixed!
  152. in src/util/syscall_sandbox.cpp:44 in 47473f8402 outdated
    39+#if !defined(__x86_64__)
    40+#error Syscall sandbox is an experimental feature currently available only under Linux x86-64.
    41+#endif // defined(__x86_64__)
    42+
    43+// Portability note: SYSCALLS_LINUX_X86_64 is Linux x86_64 specific.
    44+const std::map<uint32_t, std::string> SYSCALLS_LINUX_X86_64{
    


    laanwj commented at 6:53 pm on September 20, 2021:
    I’m not entirely convinced it’s a good idea to maintain a full list of Linux syscalls here. This definitely doesn’t scale to multiple architectures. Yes, it’s nice to able to print the name instead of the number in case of failures (that’s all it’s used for, right?), but this should be a rare enough event and we can look up the syscall number ourselves if a user reports something.

    practicalswift commented at 2:09 pm on October 1, 2021:

    Yes, it is only used for friendly printing of syscall names.

    It might seem like a minor thing, but when developing this feature that translation (from syscall number to syscall name) has been extremely helpful. FWIW I tried developing this feature without it first and that was… very cumbersome! :)

    I’ve now added a comment clarifying how LINUX_SYSCALLS (previously SYSCALLS_LINUX_X86_64) is used:

     0// This list of syscalls in LINUX_SYSCALLS is only used to map syscall numbers to syscall names in
     1// order to be able to print user friendly error messages which include the syscall name in addition
     2// to the syscall number.
     3//
     4// Example output in case of a syscall violation where the syscall is present in LINUX_SYSCALLS:
     5//
     6// ```
     7// 2021-06-09T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report.
     8// ```
     9//
    10// Example output in case of a syscall violation where the syscall is not present in LINUX_SYSCALLS:
    11//
    12// ```
    13// 2021-06-09T12:34:56Z ERROR: The syscall "*unknown*" (syscall number 314) is not allowed by the syscall sandbox in thread "msghand". Please report.
    14// ``
    15//
    16// LINUX_SYSCALLS contains two types of syscalls:
    17// 1.) Syscalls that are present under all architectures or relevant Linux kernel versions for which
    18//     we support the syscall sandbox feature (currently only Linux x86-64). Examples include read,
    19//     write, open, close, etc.
    20// 2.) Syscalls that are present under a subset of architectures or relevant Linux kernel versions
    21//     for which we support the syscall sandbox feature. This type of syscalls should be added to
    22//     LINUX_SYSCALLS conditional on availability like in the following example:
    23//         ...
    24//         #if defined(__NR_arch_dependent_syscall)
    25//             {__NR_arch_dependent_syscall, "arch_dependent_syscall"},
    26//         #endif // defined(__NR_arch_dependent_syscall)
    27//         ...
    

    Note that if LINUX_SYSCALLS is incomplete for some reason (new architecture or new syscall), then it will fallback on the behaviour you’re suggesting (just printing the syscall number).

    I suggest we keep it as-is for now and revisit that decision when/if syscall sandbox support for the next architecture (non-x86-64) is added. Could that work for now? :)


    laanwj commented at 6:57 pm on October 4, 2021:

    OK, I get your point better now, it’s not needed to replicate this entire structure for every architecture, because it doesn’t have the raw numbers, it uses the __NR_ constants.

    I agree it can be useful, it too bad querying a syscall name isn’t a standard libc call, or outside that, a standard linux command that the error message can be run through. To add insult to injury, the syscall numbers are in a different file on every architecture, it’s not like you can just grep /usr/include/unistd.h and be done with it.

    The thing is that even now, new syscalls are often added to the Linux kernel. It seems really annoying to track this. But maybe it’s not needed? How often do we use a new syscall.


    MarcoFalke commented at 11:52 am on October 6, 2021:

    or outside that, a standard linux command

    Is there a standard linux command for a popular operating system? Maybe it would be enough to say: To get the syscall name, use grep ... on the latest Ubuntu/Debian?


    MarcoFalke commented at 12:35 pm on October 11, 2021:
    It looks like gdb is enough to print the name of the violating syscall. See for example #23248#issue-1022522427, which prints mincore.
  153. in configure.ac:1972 in 47473f8402 outdated
    1968@@ -1933,6 +1969,7 @@ echo
    1969 echo "Options used to compile and link:"
    1970 echo "  external signer = $use_external_signer"
    1971 echo "  multiprocess    = $build_multiprocess"
    1972+echo "  with experimental syscall sandbox support (-sandbox=<mode>) = $use_syscall_sandbox"
    


    laanwj commented at 6:56 pm on September 20, 2021:
    This is supposed to be a summary, not documentation :smile: Please shorten this to something like echo " with experimental syscall sandbox support"

    practicalswift commented at 3:23 pm on September 30, 2021:
    Makes sense. Fixed!
  154. in src/logging.cpp:163 in 47473f8402 outdated
    159@@ -160,6 +160,7 @@ const CLogCategoryDesc LogCategories[] =
    160     {BCLog::I2P, "i2p"},
    161     {BCLog::IPC, "ipc"},
    162     {BCLog::LOCK, "lock"},
    163+    {BCLog::UTIL, "util"},
    


    laanwj commented at 6:58 pm on September 20, 2021:
    Ok, yes, adding a util category makes sense. We might want to migrate at least “rand” into here as well, at some point (not in this PR though).
  155. laanwj commented at 7:02 pm on September 20, 2021: member
    Code review and lightly tested ACK. Sorry for only getting around to this now. It looks good to me. Found no serious issues but left some nits.
  156. in src/rpc/misc.cpp:432 in 47473f8402 outdated
    427+        {},
    428+        RPCResult{RPCResult::Type::NONE, "", ""},
    429+        RPCExamples{
    430+            HelpExampleCli("invokedisallowedsyscall", "") + HelpExampleRpc("invokedisallowedsyscall", "")},
    431+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
    432+            if (!Params().IsTestChain()) {
    


    laanwj commented at 7:19 pm on September 20, 2021:
    Good idea to only allow this on test chains. I was about to comment that this call (which aborts the process in an ugly way) should be behind some test flag, but this is already okay imo.
  157. practicalswift force-pushed on Sep 30, 2021
  158. practicalswift force-pushed on Oct 1, 2021
  159. Add syscall sandboxing (seccomp-bpf) 4747da3a5b
  160. practicalswift force-pushed on Oct 1, 2021
  161. laanwj commented at 7:14 pm on October 4, 2021: member
    Code review and lightly tested ACK 4747da3a5b639b5a336b737e7e3cbf060cf2efcf
  162. laanwj merged this on Oct 4, 2021
  163. laanwj closed this on Oct 4, 2021

  164. sidhujag referenced this in commit 48e1f25efb on Oct 4, 2021
  165. in src/util/syscall_sandbox.cpp:865 in 4747da3a5b
    860+    const SyscallSandboxAction default_action = g_syscall_sandbox_log_violation_before_terminating ? SyscallSandboxAction::INVOKE_SIGNAL_HANDLER : SyscallSandboxAction::KILL_PROCESS;
    861+    std::vector<sock_filter> filter = seccomp_policy_builder.BuildFilter(default_action);
    862+    const sock_fprog prog = {
    863+        .len = static_cast<uint16_t>(filter.size()),
    864+        .filter = filter.data(),
    865+    };
    


    MarcoFalke commented at 8:14 am on October 5, 2021:

    https://en.cppreference.com/w/cpp/language/aggregate_initialization

    Neither I, nor CI could find a compiler that rejects this C++20 code, so does that mean we are allowed to use it in new code now?


    MarcoFalke commented at 8:32 am on October 5, 2021:

    Looks like this is supported since clang 3.2 and gcc 4.7, but maybe not msvc?

    See #23183

    msvc: error C7555: use of designated initializers requires at least '/std:c++20'


    theuni commented at 4:52 pm on October 25, 2021:

    FYI adding -Wpedantic turns this into a warning: $ g++ -Wall -std=c++17 -pedantic pedantic.cpp -c -o pedantic.o

    pedantic.cpp: In function ‘int main()’: pedantic.cpp:8:13: warning: C++ designated initializers only available with ‘-std=c++2a’ or ‘-std=gnu++2a’ [-Wpedantic] 8 | foo bar{.a = 1};

    I actually thought we had -Wpedantic on by default, I’m not sure why we don’t. @fanquake @dongcarl is there an obvious reason I’m forgetting?

    Edit: Clang also has -Wc++20-designator.

    Edit2: pushed a quick branch to demonstrate the changes needed to build bitcoind (only, everything else untested) with -Wpedantic, which is meant to warn about the use of compiler exensions: https://github.com/theuni/bitcoin/tree/build-pedantic

  166. MarcoFalke referenced this in commit c79d9fb2f6 on Oct 5, 2021
  167. sidhujag referenced this in commit 6360e632d2 on Oct 5, 2021
  168. laanwj referenced this in commit 89b910711c on Oct 5, 2021
  169. ryanofsky commented at 6:14 pm on October 5, 2021: member

    Just curious, but merging this seems to have caused a few build problems that required followups: #23178 #23179 #23196, and I guess some test problems in local developer checkouts.

    I’m wondering if there are any CI improvements that might have caught these problems. Are too many of the CI builds using old kernels, and could adding CI builds with newer kernels catch these problems? Are too many of the CI builds using depends and could adding CI builds with system dependencies catch these problems? Is there some extended draftbot build that might have caught these problems?

    It seems like it could make sense to run some extended CI on PRs that modify configure.ac or build scripts, but not clear what might have been useful in this case.

  170. laanwj commented at 6:48 pm on October 5, 2021: member

    could adding CI builds with newer kernels catch these problems

    For the record, it’s not the running kernel version that makes a difference here. But the kernel headers. What would have helped find the problem is compiling with older kernel headers. We also should have done a GUIX build.

    It’s the first time these matter at all for our compilation. Besides syscall(SYS_getrandom… for random context gathering, there is no code that directly interfaces with the Linux kernel at all.

    I think it’s a good lesson that we need to make sure to define everything we need from the kernel ourselves, to support compilation on a wide range of installations. I knew about this problem but underestimated how much it’s still a problem nowadays.

  171. ryanofsky commented at 7:20 pm on October 5, 2021: member
    I just don’t understand the basics here. What CI build could you theoretically add to catch these problems? Would it be a CI build with a really old kernel and really old headers? A really new kernel and new headers? An old kernel with new headers? And new kernel with old headers?
  172. sidhujag referenced this in commit 5c90f6b9a3 on Oct 5, 2021
  173. MarcoFalke added the label DrahtBot Guix build requested on Oct 6, 2021
  174. MarcoFalke removed the label DrahtBot Guix build requested on Oct 6, 2021
  175. MarcoFalke commented at 9:10 am on October 6, 2021: member

    Are too many of the CI builds using old kernels, and could adding CI builds with newer kernels catch these problems?

    Generally developers tend to run the latest software, so this is usually not something that will catch a lot of fish as a CI task. In fact, CI tasks with bleeding edge software might tend to break more often. (I do run them, but not part of this project).

    Is there some extended draftbot build that might have caught these problems?

    Simply adding the “DrahtBot Guix build requested” label would have caught this. I think any pull request that is tagged with “Build system” should be Guix-built as well. As DrahtBot is running on limited resources, it might take a day or two to create the guix build, but I think this is an acceptable delay. If there is an “emergency-fix” devs can always do a local guix build.

  176. MarcoFalke commented at 10:06 am on October 6, 2021: member
    And when it comes to preventing build errors via the CI, I think this is not something we should try to achieve at all costs. Due to compiler bugs (or just software bugs in general), there will always be at least one software configuration and build flag configuration that will fail to build. I think it is reasonable to have tests for common software configurations on the latest LTS releases of operating systems. Though, when it comes to supporting any software configuration, waiting for an actual user to report the issue instead of integrating into the CI works probably better.
  177. ryanofsky commented at 12:29 pm on October 6, 2021: member

    Thanks for the replies. So is the answer to my question of what CI build would catch all these problems (#23178 #23179 #23196) that any CI build using newer kernel headers would catch all these problems? That all CI builds are using older headers and practicalswift was also using older headers, and this slipped past review?

    My motivation for asking is just to understand when I am reviewing a PR what class of errors I could expect CI to catch, and what class of errors I should be testing manually or asking the PR author about. In this case, I guess I failed to ask practicalswift how is this code affected when new syscalls are added? And in the future, when I’m reviewing a PR that makes significant changes to the build system, I can request GUIX builds?

    I don’t really have opinions on what CI should check for. I just want to understand what CI is checking for so I can review PRs better. For example, it is useful to know if CI is only checking old kernels and we are depending on developers to manually test newer kernels. It is also useful to know more generally if CI builds tend to use older or different dependencies than actual the GUIX release.

  178. MarcoFalke commented at 12:41 pm on October 6, 2021: member

    I can request GUIX builds?

    Sure, just add the label.

    It is also useful to know more generally if CI builds tend to use older or different dependencies than actual the GUIX release.

    CI is a mix of cross-compile depends builds that try to mimic guix (older LTS releases with older gcc-8) and recent clang with native sanitizer builds from system packages (except for msan, which is also using depends). Obviously Ubuntu/Debian ship different compilers than guix, so the CI is just a proxy and not a replacement for a guix build.

  179. laanwj removed this from the "Blockers" column in a project

  180. in test/functional/test_framework/test_framework.py:476 in 4747da3a5b
    470@@ -468,6 +471,10 @@ def get_bin_from_version(version, bin_name, bin_default):
    471             extra_args = [[]] * num_nodes
    472         if versions is None:
    473             versions = [None] * num_nodes
    474+        if self.is_syscall_sandbox_compiled() and not self.disable_syscall_sandbox:
    475+            for i in range(len(extra_args)):
    476+                if versions[i] is None or versions[i] >= 219900:
    


    Sjors commented at 10:44 am on November 15, 2021:
    This is actually not in the v22.0 binary and also not backported (yet). I made a commit to bump the version: https://github.com/bitcoin/bitcoin/pull/19013/commits/4672c1ffeae515ee457968af5235a8e4418e0df5
  181. DrahtBot locked this on Nov 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me