Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9168 closed enhancement (implemented)

GSOC seccomp stage 1

Reported by: ctoader Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay gsoc seccomp sandbox
Cc: ctoader Actual Points:
Parent ID: #5756 Points:
Reviewer: Sponsor:

Description

Review and merge phase 1 of seccomp work.

Remote branch: https://github.com/cristiantoader/tor-gsoc-capabilities

Child Tickets

Change History (16)

comment:1 Changed 6 years ago by ctoader

Status: newneeds_review

comment:2 Changed 6 years ago by nickm

Keywords: tor-relay gsoc seccomp sandbox added
Milestone: Tor: 0.2.5.x-final
Status: needs_reviewneeds_revision

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. :)

comment:3 Changed 6 years ago by nickm

Oh, another thing: I bet if we test with libevent 2.1, we will find that we also need to permit signalfd and eventfd.

comment:4 Changed 6 years ago by ctoader

Status: needs_revisionneeds_review

comment:5 Changed 6 years ago by ctoader

Status: needs_reviewneeds_revision

Implemented changes.

comment:6 Changed 6 years ago by 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.
  • 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)

comment:7 Changed 6 years ago by nickm

Oh! We need to add mlockall to the list of permitted syscalls.

comment:8 in reply to:  7 Changed 6 years ago by nickm

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.

comment:9 in reply to:  6 Changed 6 years ago by ctoader

Status: needs_revisionneeds_review

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.

comment:10 Changed 6 years ago by nickm

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.

comment:11 Changed 6 years ago by nickm

I've added a couple of fixes on branch "gsoc-ctoader-cap-phase1" in my public repository.

The code to log the syscall isn't working for me. Does x86_64 have a different syscall calling convention?

comment:12 Changed 6 years ago by ctoader

Pushed fixes for syscall logging, added a few missing syscalls, and tested with and without ORPort for 32 and 64 bits Linux.

Is this version ok to merge?

comment:13 Changed 6 years ago by nickm

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.

comment:14 Changed 6 years ago by ctoader

Sorry for the late reply. This looks fine for me. Please let me know when you merge the changes into master.

comment:15 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay, merged it. (Hooray!)

comment:16 Changed 6 years ago by nickm

Parent ID: #5756
Note: See TracTickets for help on using tickets.