sandbox: add `newfstatat` & `copy_file_range` to allowed filesystem syscalls #23179

1. 
   fanquake commented at 12:44 AM on October 5, 2021: member

   Similar to #23178, this is a follow up to #20487, which has broken running the unit tests for some developers. Fix this by adding newfstatat to the list of allowed filesystem related calls.

2. sandbox: add newfstatat to allowed filesystem syscalls ee08741c9c
3. fanquake added the label Utils/log/libs on Oct 5, 2021
4. fanquake requested review from practicalswift on Oct 5, 2021
5. fanquake requested review from achow101 on Oct 5, 2021
6. 
   achow101 commented at 1:07 AM on October 5, 2021: member

   Closer, but now missing copy_file_range too:

   ERROR: The syscall "copy_file_range" (syscall number 326) is not allowed by the syscall sandbox in thread "httpworker.2". Please report.
   

7. sandbox: add copy_file_range to allowed filesystem syscalls 44d77d2213
8. 
   fanquake commented at 1:17 AM on October 5, 2021: member

   Closer, but now missing copy_file_range too:

   Added copy_file_range to the list of allowed file related syscalls. File related syscalls are already enabled for the HTTP worker threads.

9. 
   achow101 commented at 1:31 AM on October 5, 2021: member

   ACK 44d77d2213e6bd2e2f700dd8c3c3f932bc1bcb48

   Tested that this does fix the test issues I was running into.

10. fanquake renamed this:
    sandbox: add newfstatat to allowed filesystem syscalls
    sandbox: add `newfstatat` & `copy_file_range` to allowed filesystem syscalls
    on Oct 5, 2021
11. 
    laanwj commented at 6:07 AM on October 5, 2021: member

    Code review ACK 44d77d2213e6bd2e2f700dd8c3c3f932bc1bcb48

12. 
    laanwj commented at 6:32 AM on October 5, 2021: member

    Doing a GUIX build with #23179 and #23178 because I'm slightly afraid newfstatat & copy_file_range are new syscalls that need to be defined manually as well.

    If so, do you mind if I cherry-pick this into #23178?

13. 
    fanquake commented at 6:43 AM on October 5, 2021: member

    If so, do you mind if I cherry-pick this into #23178?

    Yes that's fine.

14. 
    laanwj commented at 6:49 AM on October 5, 2021: member

    Looks like it's not neccessary, GUIX build passes with those two PRs as-is.

15. in src/util/syscall_sandbox.cpp:539 in 44d77d2213

     559 | -        allowed_syscalls.insert(__NR_rmdir);      // delete a directory
     560 | -        allowed_syscalls.insert(__NR_stat);       // get file status
     561 | -        allowed_syscalls.insert(__NR_statfs);     // get filesystem statistics
     562 | -        allowed_syscalls.insert(__NR_statx);      // get file status (extended)
     563 | -        allowed_syscalls.insert(__NR_unlink);     // delete a name and possibly the file it refers to
     564 | +        allowed_syscalls.insert(__NR_access);          // check user's permissions for a file
    

    ---

    ---

    

    laanwj commented at 7:28 AM on October 5, 2021:

    I think it creates somewhat of a merge hotspot to re-align these comments every time something is added/removed.

16. practicalswift approved
17. 
    practicalswift commented at 9:11 AM on October 5, 2021: contributor

    cr ACK 44d77d2213e6bd2e2f700dd8c3c3f932bc1bcb48

    Thanks for improving the experimental syscall sandbox! :)

18. MarcoFalke merged this on Oct 5, 2021
19. MarcoFalke closed this on Oct 5, 2021

    ---

20. fanquake deleted the branch on Oct 5, 2021
21. sidhujag referenced this in commit 6360e632d2 on Oct 5, 2021
22. ryanofsky cross-referenced this on Oct 5, 2021 from issue Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift
23. laanwj cross-referenced this on Apr 5, 2022 from issue Outcome of the syscall sandbox experiment by laanwj
24. bitcoin locked this on Oct 30, 2022

Contributors

fanquakeachow101laanwjpracticalswift

Review Requested
achow101