Disable the syscall sandbox for bitcoin-qt and remove gui-related syscalls #24758

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2204-gui-box-🌊 changing 5 files +6 −9
  1. MarcoFalke commented at 12:42 pm on April 4, 2022: member

    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)

  2. MarcoFalke commented at 12:44 pm on April 4, 2022: member

    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

  3. hebasto commented at 12:49 pm on April 4, 2022: member
    Concept ACK.
  4. laanwj commented at 1:40 pm on April 4, 2022: member

    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.

  5. DrahtBot added the label Utils/log/libs on Apr 4, 2022
  6. MarcoFalke commented at 2:10 pm on April 4, 2022: member

    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.

  7. vincenzopalazzo commented at 2:25 pm on April 4, 2022: none
    Concept ACK
  8. ryanofsky commented at 4:52 pm on April 4, 2022: member

    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.

  9. ryanofsky approved
  10. ryanofsky commented at 5:02 pm on April 4, 2022: member

    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

  11. jarolrod commented at 4:48 am on April 5, 2022: member
    concept ack
  12. laanwj commented at 10:00 am on April 5, 2022: member

    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.

  13. laanwj commented at 10:04 am on April 5, 2022: member

    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

  14. MarcoFalke force-pushed on Apr 5, 2022
  15. laanwj commented at 11:18 am on April 5, 2022: member

    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.

  16. in src/util/syscall_sandbox.cpp:824 in fa45bb70b8 outdated
    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) {
    


    laanwj commented at 11:21 am on April 5, 2022:
    nit: Could move this entire code block to the caller site instead of making it a bool arg.

    MarcoFalke commented at 11:42 am on April 5, 2022:
    Done
  17. init: Disable syscall sandbox in the bitcoin-qt process fa0c2aa826
  18. Remove gui-only syscalls
    * 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.
    fabdf9f870
  19. MarcoFalke force-pushed on Apr 5, 2022
  20. laanwj merged this on Apr 6, 2022
  21. laanwj closed this on Apr 6, 2022

  22. MarcoFalke deleted the branch on Apr 6, 2022
  23. sidhujag referenced this in commit c34e7a0468 on Apr 6, 2022
  24. DrahtBot locked this on Apr 6, 2023

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-10-04 13:12 UTC

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