GSOC seccomp stage 1
Review and merge phase 1 of seccomp work.
Remote branch: https://github.com/cristiantoader/tor-gsoc-capabilities
Trac:
Username: ctoader
Activity
changed milestone to %Tor: 0.2.5.x-final
Initial comments:
- Please use the same file header header style as the rest of Tor.
- Make sure that the code passes "make check-spaces" with no warnings.
- By convention, stuff in src/common shouldn't include or use stuff in src/or. So instead of calling get_options() there, have main.c do "if (get_options()->Sandbox) tor_global_sandbox();"
- Why do the periodic events need special sandboxing?
- In C, if you need to declare a function with no arguments, you have to say "int foo(void)". Unlike Java or C++, if you say "int foo()", you're declaring a function with an unspecified number of arguments, not a no-argument function.
- We should strip out the unused , #if 0 parts before merging.
- You don't need to check the return value of get_options(); it is not allowed to fail.
- You don't need to use do { } while(0) and break statements to achieve single-point-of-return. We don't believe in single-point-of-return. Just use "return" in the middle of the function if that is clearest. If there's significant cleanup, you can also use the "goto end" pattern.
- By convention, we document every function; see doc/HACKING for more information.
- Function documentation should explain what the function does in enough detail that somebody else could implement the function correctly using only the documentation.
- Why is filters.h a separate header? Why is it a header at all?
- I don't think it's allowed to call fprintf from a signal callback. Also, fprintf() isn't likely to work at runtime for most Tor users.
- It looks like this patch would break compilation on all non-Linux hosts.
- Somebody (me?) needs to write the configure.in magic here.
- Is "log and ignore" the right approach to disallowed syscalls? "Log and exit" might also be a good choice.
- What happened to the magic "socketcall" syscall number? We needed it for test_filter -- do we not need it for general_filter? It seems that maybe we don't, so long as we're calling seccomp_rule_add ... but the code here seems to be calling seccomp_rule_add_exact (why?).
- We should check the return value of tor_global_sandbox, and probably exit with an error if it fails.
- We should probably warn if the user turns on Sandbox, but we were built without sandbox support.
After that's done and we merge this, I'd say the next steps should be trying to make the sandbox more restrictive if we can: making the filters apply to specific files, only turning on execve when we must, and so on. But that's another ticket. :)
Trac:
Keywords: N/A deleted, tor-relay gsoc seccomp sandbox added
Status: needs_review to needs_revision
Milestone: N/A to Tor: 0.2.5.x-final-
Follow-up review:
- log_err() won't work any better from a signal handler than fprintf(). It calls snprintf, which isn't in the signal-safe functions list. Also, exit() isn't on the signal-safe functions list, either, although _exit() is. If you have a look at "man sigaction" or "man 7 signal", it should list the functions which can be safely called.
- Based on that, I wonder if there shouldn't be configurable levels of sandboxing configurable, where one of them records the forbidden system calls and continues, and one of them just aborts. Let's talk about the right interface for that.
- Is the socketcall() thing still necessary now that we switched to add from add_exact?
Other notes:
- I still need to write the autoconf/automake magic here.
- There should be documentation for the Sandbox option in doc/tor.1.txt
- Probably we should squash before merging. (The branch is 3184 lines long, but the diff is only 339 lines long)
-
Replying to nickm:
Oh! We need to add mlockall to the list of permitted syscalls.
(Unless the call to mlockall happens before we install the filter, I guess.
Replying to nickm:
Follow-up review:
- log_err() won't work any better from a signal handler than fprintf(). It calls snprintf, which isn't in the signal-safe functions list. Also, exit() isn't on the signal-safe functions list, either, although _exit() is. If you have a look at "man sigaction" or "man 7 signal", it should list the functions which can be safely called. I am now using only write and snprintf; snprintf is not part of the signal-safe functions list but otherwise I didn't know how to add the syscall number that caused the SIGSYS. This works fine on my PC. If you want me to, I can just leave a write with a static message, or maybe you have another suggestion.
- Based on that, I wonder if there shouldn't be configurable levels of sandboxing configurable, where one of them records the forbidden system calls and continues, and one of them just aborts. Let's talk about the right interface for that. Ok so I left a symbol definition called _DEBUGGING_CLOSE, but you probably had something more complex in mind but I couldn't reach you today; right now I would recommend to always close, mainly because whenever a syscall fails the program doesn't recover; if it happens in the periodic callback functions it ends up spamming error messages.
- Is the socketcall() thing still necessary now that we switched to add from add_exact? Unfortunately yes, I will keep looking into this.. I'll add the change in the next version when I'll make the filter more restrictive.. Or if there's no rush I'll keep looking into this these days.
Other notes:
- I still need to write the autoconf/automake magic here.
- There should be documentation for the Sandbox option in doc/tor.1.txt I have written it.
- Probably we should squash before merging. (The branch is 3184 lines long, but the diff is only 339 lines long)
Also added mlockall to the syscall list, but a standard run with no command line parameters works just fine without it.
Trac:
Username: ctoader
Status: needs_revision to needs_review-
Hardcoding 34 as the length of that string is not so cool. strlen() or sizeof() both make more sense. That message should probably also be declared "static const".
stdout may have been closed. (It will have been closed if we're running in "daemon mode".)
This stuff should be easy enough to fix up though... if I have time this morning, I'll try to get it tidied and squashed and merged.
-
I've added a few more commits in my branch "gsoc-ctoader-cap-phase1":
- Replace __DEBUGGING_CLOSE with DEBUGGING_CLOSE
- On sandbox violation, write to log file too
- Add a changes file
I've also made a "gsoc-ctoader-cap-phase1-squashed" branch that I propose to merge: it has the same content as the other branch, but the commits are squashed into one.
If you're okay with the changes, I'll merge the squashed branch to master, and after that you'll want to rebase your phase2 branch onto master. You can do this with something like "git rebase gsoc-ctoader-cap --onto master". That rewrites the branch to replay every commit since "gsoc-ctoader-cap" onto the current state of the branch "master". You might want to do this on a copy of the phase2 branch.
-
Trac:
Parent: N/A to #5756 (moved) moved to tpo/core/tor#9168 (closed)