It is basically impossible (and a bit out of scope) for us to maintain a sandbox for the qt library. I am not sure if it is possible to only sandbox a few threads in a process, but I doubt this will add no practical benefit anyway, so I am disabling the sandbox for the whole bitcoin-qt process.
See also #24690 (comment)
On current master:
0$ ./src/qt/bitcoin-qt -sandbox=log-and-abort
1ERROR: The syscall "memfd_create" (syscall number 319) is not allowed by the syscall sandbox in thread "main". Please report.
2terminate called without an active exception
3Aborted (core dumped)
With this pull:
All good
Concept ACK.
I am not sure if it is possible to only sandbox a few threads in a process, but I doubt this will add no practical benefit anyway
This is definitely possible. It’s why there are different syscall profiles in the first place. It makes sense to sandbox threads that interface with the outside world e.g. the network thread, or RPC threads. It doesn’t make much sense to sandbox the GUI thread as it interfaces with the user.
Ok. I believe, currently the syscall sandbox in Bitcoin Core is set up so that it registers as early as possible (thus locking down all threads with the “default sandbox”) and then later locking down each newly created thread with additional restrictions.
To work around this with the GUI, the initial lock-down for the main thread would need to be skipped. Presumably the restriction on the main thread, once it gets renamed to shutoff as well.
I am not sure if it is worth it to make those changes, give that multiprocess will fix this as well.
I am not sure if it is possible to only sandbox a few threads in a process
Maybe I’m misreading, but I don’t understand this comment. The whole design of practicalswift’s sandboxing implementation is premised on allowing specific threads to make specific syscalls, so obviously this is possible and we’re already doing it. If there are cases where the same thread in bitcoind is doing something different when a GUI is present, these seem like bugs that should be fixed. Or I guess I don’t understand where these cases would ever be expected.
I do understand that the main GUI thread is doing a lot of things out of our control and may be easier not to sandbox, though.
Reading earlier discussion it seems like problems are in fact happening on the main gui thread not other threads.
It does seem like instead of disabling sandboxing entirely, it would be better to just not sandbox the gui thread, or allow extra syscalls on the gui thread. But maybe this is more complicated to implement.
Concept ACK in any case, since I think I understand this now, and Code review ACK 33335a3c6eab616823a380241b58495b88b89e1d
It does seem like instead of disabling sandboxing entirely, it would be better to just not sandbox the gui thread, or allow extra syscalls on the gui thread. But maybe this is more complicated to implement.
The point is that we cannot have a complete view of what the GUI thread does. By nature it calls into the operating system to do things like rendering. In the worst case this involves, say, proprietary video drivers. Or special constructions for input devices. Just adding syscalls when they come up is a bottomless pit of despair.
This is a fundamental problem with system-call based sandboxing. It’s a layer violation. Sandboxing other threads also suffers from this problem, but less. E.g., libraries that are dynamically linked (say, glibc) may change the system calls they use at any point (this happens pretty often). GUI is just especially bad in this regard.
I agree that the other threads could still be sandboxed, ideally, because even if they do have callbacks to Qt code they shouldn’t be doing any rendering or input handling.
To work around this with the GUI, the initial lock-down for the main thread would need to be skipped. Presumably the restriction on the main thread, once it gets renamed to shutoff as well.
Sounds like a good solution to me. The difference would be that part of the initialization process wouldn’t be sandboxed either. This is fine, sandboxing is mostly interesting for threads that communicate with anything untrusted.
This is a fundamental problem with system-call based sandboxing. It’s a layer violation.
As an aside, one project that attempts to work around these kind of fragility of syscall sandboxing, in the Linux kernel, is https://landlock.io/ . It groups certain logical actions as being allowed or not instead of micro-managing syscalls. But it’s still under heavy development I don’t think it makes sense to start using that at this point.
Also, even then the GUI is probably… weird
Code review ACK fa45bb70b83cb57967bc3a57d7da0f972facb864
Code review ACK fabdf9f870a4c07cb3548c3b385438f02179ea88
Ostensibly you could still do SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION); in the core thread started in BitcoinApplication::startThread() (InitExecutor, if I follow the code correctly). It handles the larger part of the initialization/shutdown process. But it doesn’t matter too much.
820@@ -823,7 +821,9 @@ bool SetupSyscallSandbox(bool log_syscall_violation_before_terminating)
821 return false;
822 }
823 }
824- SetSyscallSandboxPolicy(SyscallSandboxPolicy::INITIALIZATION);
825+ if (use_for_main_thread) {
* Revert "util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem)"
This reverts commit f05a4cdf5a0363e1c12f00c034afb60e7ea0c775.
* Revert "util: add linkat to syscall sandbox (AllowFileSystem)"
This reverts commit 9809db3577f0fa618bea42635b1581e628a30395.