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

1. 
   fanquake commented at 0: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:

   0ERROR: 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. DrahtBot locked this on Oct 30, 2022

Contributors
fanquake achow101 laanwj practicalswift

Review Requested
achow101

Labels
Utils/log/libs