Opened 6 years ago

Closed 6 years ago

#9249 closed enhancement (implemented)

GSOC seccomp stage 2

Reported by: ctoader Owned by: nickm
Priority: Medium Milestone:
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 2 of seccomp capabilities filtering.

This version should implement much more restrictive filtering, mostly based on parameter filtering of the syscalls.

Remote: https://github.com/cristiantoader/tor-gsoc-capabilities
Branch: gsoc-cap-stage2

Child Tickets

Change History (21)

comment:1 Changed 6 years ago by ctoader

Type: defectenhancement

comment:2 Changed 6 years ago by nickm

Parent ID: #5756

comment:3 Changed 6 years ago by nickm

Quick review, as of dde3ed385bc9de8, since I think you asked for one:

  • LENGHT is mispelled. (Probably it should be ARRAY_LENGTH, and it should be in util.h).
  • filter_dynamic should probably be static; nothing needs to use it outside of sandbox.c, right?
  • Exposing sandbox_static_cfg_t, pfd_elem, and sandbox_filter_func_t in the header seems wrong: those are not part of the public sandbox interface, right? They only make sense for seccomp2.
  • Some of the restrictions in sandbox.c don't make sense for security; we should at least document why we're doing them. For example, why restrict clock_gettime() to CLOCK_MONOTONIC? Why restrict time() to NULL?
  • Some of the restrictions in sandbox.c don't make sense given how the syscalls work. (e.g., surely we're sometimes calling epoll_ctl with EPOLL_CTL_DEL!)
  • We probably want to restrict execve arguments in the same way we restrict open, openat, etc.
  • The structures and functions all need documentation.

Not necessarily this time:

  • It's confusing to me how filter_dynamic is a linked list of so many different things. Maybe once it's documented, it will make sense?
  • It seems excessive to use a separate mmap() for every prot_strdup() call. Perhaps instead we should have a big mmap that fills up with strings, and which we mprotect when we're done. This could wait until later though.

Other than that, it's looking good....

comment:4 Changed 6 years ago by nickm

Oh, one more issue:

+  if (filter_nopar_gen != NULL) {
+    filter_size = sizeof(filter_nopar_gen) / sizeof(filter_nopar_gen[0]);
+  } else {
+    filter_size = 0;
+  }
+

It's impossible for a static array to be NULL, and this code makes Coverity complain about deadcode issues.

comment:5 Changed 6 years ago by ctoader

Just so I can keep here a complete list, there are also the memory leaks in sandbox_init_filter();

  • LENGHT is mispelled. (Probably it should be ARRAY_LENGTH, and it should be in util.h).

(FIXED)

  • filter_dynamic should probably be static; nothing needs to use it outside of sandbox.c, right?

(FIXED)

  • Exposing sandbox_static_cfg_t, pfd_elem, and sandbox_filter_func_t in the header seems wrong: those are not part of the public sandbox interface, right? They only make sense for seccomp2.

I added support for a sandbox_t struct which will have an array of function pointers associated with the filter as well as a list of parameters for the filters; this should be public as a configuration context for a sandbox; different definitions of the struct may exist based on the implementation so it's not necessarily seccomp specific..

  • Some of the restrictions in sandbox.c don't make sense for security; we should at least document why we're doing them. For example, why restrict clock_gettime() to CLOCK_MONOTONIC? Why restrict time() to NULL?

I agree that some restrictions may be dropped as they don't bring security value.. need to discuss which..

  • Some of the restrictions in sandbox.c don't make sense given how the syscalls work. (e.g., surely we're sometimes calling epoll_ctl with EPOLL_CTL_DEL!)

(FIXED)

  • We probably want to restrict execve arguments in the same way we restrict open, openat, etc.

(FIXED)

  • It's confusing to me how filter_dynamic is a linked list of so many different things. Maybe once it's documented, it will make sense?

it is a linked list of parameters, their index, type, and associated syscall; if prot is set to 0, then the pointer was not protected.. i kept changing it so it's not documented yet..

  • It seems excessive to use a separate mmap() for every prot_strdup() call. Perhaps instead we should have a big mmap that fills up with strings, and which we mprotect when we're done. This could wait until later though.

I don't really see the upside to that other than reducing the number of calls from N to 1; the downside however would be having to manage that memory similar to malloc/calloc but having only one allocated segment (or maybe have another block allocated when a limit is reached..?); do you consider it is worth the added code complexity?

  • It's impossible for a static array to be NULL, and this code makes Coverity complain about deadcode issues.

(FIXED)

  • The structures and functions all need documentation.

comment:6 Changed 6 years ago by ctoader

32-bit only ready to review.. I'll try to have the 64 bit ready tomorrow as well

Remote: https://github.com/cristiantoader/tor-gsoc-capabilities
Branch: gsoc-cap-stage2

comment:7 Changed 6 years ago by ctoader

Status: newneeds_review

64-bit version is also ready for review

comment:8 Changed 6 years ago by nickm

Notes:

  • Does MAX_PARAM_LEN really mean that we can't have a filename longer than 64 characters? That sounds poor. Those happen all the time in real life.
  • Using strncmp instead of strcmp (in various places in sandbox.c) seems likely to give a wrong result if one value is a prefix of the other. strcmp() should be safe instead.
  • Do we truly only use getaddrinfo in the code to look up our own address?
  • Use doxygen style for documenting structure elments too.
  • I'd still be more comfortable if the data structure for pfd_elem weren't so closely tied to the sandbox implementation. At the very least, it shouldn't be exposed in the header if it's implementation-specific.
  • Prefer TOR_SLIST or TOR_SINGLEQ to manually managing a linked list. (They're defined in src/ext/tor_queue.h)
  • All functions that take no arguments need to be declared as foo(void), not as foo(). ex: sandbox_cfg_new() in sandbox.h, sandbox_init_filter() in main.c
  • Use "int" for a boolean, not "char". (There's almost never a reason to use char this way)
  • We should be supporting /dev/urandom, not /dev/random
  • When we call openssl's RAND_poll periodically, does that call still work?
  • In the functions that take an variable of filenames, it seems fragile to have to include the count of the filenames. What if instead we make them end with NULL.?

Also:

*It seems excessive to use a separate mmap() for every prot_strdup() call. Perhaps instead we should have a big mmap that fills up with strings, and which we mprotect when we're done. This could wait until later though.

I don't really see the upside to that other than reducing the number of calls from N to 1; the downside however would be having to manage that memory similar to malloc/calloc but having only one allocated segment (or maybe have another block allocated when a limit is reached..?); do you consider it is worth the added code complexity?

Well, it does mean that we use an entire memory page for every interned string. That seems inefficient.

You don't need malloc/calloc here, since these strings are never freed. Instead, you could probably use something closer to what we have in memarea.c. (For example, you could make an alternative constructor for memarea_t that takes a function which it uses to get a new chunk.) This isn't a change you need to make yourself if you don't want to, though; i'm happy to hack it in later.

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

I added comments at the end of each item describing their state. I need a bit of feedback, wasn't able to fully complete 2 items. Please let me know as soon as you can what I should do. I have also fixed a few bugs, might be worth looking over the commit history.

Looking forward to a response!

Replying to nickm:

Notes:

  • Does MAX_PARAM_LEN really mean that we can't have a filename longer than 64 characters? That sounds poor. Those happen all the time in real life. -- DONE
  • Using strncmp instead of strcmp (in various places in sandbox.c) seems likely to give a wrong result if one value is a prefix of the other. strcmp() should be safe instead. --DONE
  • Do we truly only use getaddrinfo in the code to look up our own address? --Probably not, but the calls are many and complicated, do you have a suggestion as how to approach this? So far I have added extra logging for whenever a sandbox_getaddrinfo() is not successful and also I have implemented a way to allow storing more than one result.
  • Use doxygen style for documenting structure elments too. -- DONE
  • I'd still be more comfortable if the data structure for pfd_elem weren't so closely tied to the sandbox implementation. At the very least, it shouldn't be exposed in the header if it's implementation-specific. --I have made everything more dynamic in order to allow for different implementations. It is basically a linked list of void* data, and the pointer is interpreted in different ways depending on the implementation.
  • Prefer TOR_SLIST or TOR_SINGLEQ to manually managing a linked list. (They're defined in src/ext/tor_queue.h) --I found it very difficult to use the macros in tor_queue.h; also they modify the data structures a bit, is this a requirement or is it possible for the code to be accepted as is from this point of view. Everything seems much clearer to me without the macros but I would guess that someone familiar with them would think otherwise. Please let me know..
  • All functions that take no arguments need to be declared as foo(void), not as foo(). ex: sandbox_cfg_new() in sandbox.h, sandbox_init_filter() in main.c -- DONE..
  • Use "int" for a boolean, not "char". (There's almost never a reason to use char this way) -- DONE
  • We should be supporting /dev/urandom, not /dev/random --DONE
  • When we call openssl's RAND_poll periodically, does that call still work? --DONE (the function that was failing before the fix was RSAPrivateKey_dup in crypto_pk_copy_full, and it seems to be working fine, ran the code with the debugger)
  • In the functions that take an variable of filenames, it seems fragile to have to include the count of the filenames. What if instead we make them end with NULL.? -- DONE

Also:

*It seems excessive to use a separate mmap() for every prot_strdup() call. Perhaps instead we should have a big mmap that fills up with strings, and which we mprotect when we're done. This could wait until later though.

I don't really see the upside to that other than reducing the number of calls from N to 1; the downside however would be having to manage that memory similar to malloc/calloc but having only one allocated segment (or maybe have another block allocated when a limit is reached..?); do you consider it is worth the added code complexity?

Well, it does mean that we use an entire memory page for every interned string. That seems inefficient.

You don't need malloc/calloc here, since these strings are never freed. Instead, you could probably use something closer to what we have in memarea.c. (For example, you could make an alternative constructor for memarea_t that takes a function which it uses to get a new chunk.) This isn't a change you need to make yourself if you don't want to, though; i'm happy to hack it in later. -- DONE (the implementation is a bit different: due to the fact that we know the full memory size of all strings before the filter is applied, so I made it that the memory is allocated all at once and protected right before applying the filter, so memarea_t was not needed..)

comment:10 Changed 6 years ago by nickm

QUick review:

  • In prot_strings, you say: "strlen((char*) el->param)". Why the cast? If we don't know it's a char*, we shouldn't be taking strlen() of it. (Are some of these an intptr? The type seems to be an intptr_t ... how do we know which ones are strings?)
  • In prot_strings, string lengths should really be size_t.
  • Use tor_malloc() and tor_free() instead of malloc and free.
  • Use tor_strdup(), not strdup().
  • Every function should have documentation.
  • The hints argument to getaddrinfo is "const struct addrinfo hints *", not "struct addrinfo hints". Shouldn't sandbox_getaddrinfo look the same way?

comment:11 Changed 6 years ago by nickm

I also get some warnings compiling on systems without updated libevent versions, and systems without sandboxing supported. I'll be happy to clean those up myself once I merge, since conditional compilation fu is sometimes my forte.

comment:12 in reply to:  9 Changed 6 years ago by nickm

Replying to ctoader:

I added comments at the end of each item describing their state. I need a bit of feedback, wasn't able to fully complete 2 items. Please let me know as soon as you can what I should do. I have also fixed a few bugs, might be worth looking over the commit history.

Looking forward to a response!

Replying to nickm:

  • Do we truly only use getaddrinfo in the code to look up our own address?

Probably not, but the calls are many and complicated, do you have a suggestion as how to approach this? So far I have added extra logging for whenever a sandbox_getaddrinfo() is not successful and also I have implemented a way to allow storing more than one result.

Looks good. We can clean these up as we go.

  • Prefer TOR_SLIST or TOR_SINGLEQ to manually managing a linked list. (They're defined in src/ext/tor_queue.h)

I found it very difficult to use the macros in tor_queue.h; also they modify the data structures a bit, is this a requirement or is it possible for the code to be accepted as is from this point of view. Everything seems much clearer to me without the macros but I would guess that someone familiar with them would think otherwise. Please let me know..

Okay; I'm happy to clean this up later.

comment:13 in reply to:  10 Changed 6 years ago by ctoader

Replying to nickm:
I have added the changes and I hope everything is ok now, and we can merge. Thank you for helping with the items I've mentioned before, I would really struggle doing those myself. Please let me know if there is anything else.

QUick review:

  • In prot_strings, you say: "strlen((char*) el->param)". Why the cast? If we don't know it's a char*, we shouldn't be taking strlen() of it. (Are some of these an intptr? The type seems to be an intptr_t ... how do we know which ones are strings?)

I left the cast for 2 reasons: originally parameter filtering had multiple types which included strings and integer values, and each filter item had a field saying how the initptr_t should be interpreted, but this was removed and now all values are interpreted as the address which holds the string; and second, the seccomp filter takes the address of the string pointer as a parameter so it would be a choice between casting it when the parameter is added to the filter or casting it when creating the parameter filter element.

  • In prot_strings, string lengths should really be size_t.

done

  • Use tor_malloc() and tor_free() instead of malloc and free.

done, except for [1] due to a compilation error introduced when using the macro; also the macro is not needed in that case because the value of the pointer is reassigned on the next line and value cannot be NULL.

  • Use tor_strdup(), not strdup().

done

  • Every function should have documentation.

done

  • The hints argument to getaddrinfo is "const struct addrinfo hints *", not "struct addrinfo hints". Shouldn't sandbox_getaddrinfo look the same way?

done

[1] https://github.com/cristiantoader/tor-gsoc-capabilities/blob/gsoc-cap-stage2/src/common/sandbox.c#L825

comment:14 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

I've pushed a "gsoc-cap-stage2" branch to my public repository. It fixes stuff up on the computers I usually build on.

If our "interned string" magic is going to be any good, we need to disallow any attempt to munmap or mremap the interned string mapping. We also need to stop any attempt to use mprotect to make it unprotected by setting PROT_READ|PROT_WRITE on any part of it.

comment:15 Changed 6 years ago by ctoader

Did the latest commit fix this? I also had 2 more commits, some changes didn't work on my configuration (linux 32 bit).

Small note: unfortunately by stepping through libseccomp with the debugger, I realised it doesn't support intervals on the same parameter because each filter entry applies to only one syscall parameter; so you couldn't have entry 1 say 'addr > x' and entry 2 say 'addr < x', unless they're done in separate calls; I did manage do make something work though.

Please let me know if anything else needs to be done.

comment:16 in reply to:  15 ; Changed 6 years ago by nickm

Replying to ctoader:

Did the latest commit fix this? I also had 2 more commits, some changes didn't work on my configuration (linux 32 bit).

Your change in 8e003b1c69152ba6e5c3a09db11472eef5db14da re-broke my 64-bit linux box with libseccomp version 1.0.1. I get:

Sep 11 13:38:48.000 [err] add_noparam_filter(): Bug: (Sandbox) failed to add syscall index 34 (NR=-109), received libseccomp error -33
Sep 11 13:38:48.000 [err] install_syscall_filter(): Bug: (Sandbox) failed to add param filters!
Sep 11 13:38:48.000 [err] tor_main(): Bug: Failed to create syscall sandbox filter

This is for recv, which is apparently implemented via the recvfrom syscall on 64-bit linux.

Also, your change in 3802cae9597fa417ceec42 breaks compilation on OSX, where I get:

src/common/sandbox.h:166: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘*’ token
src/common/sandbox.h:174: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:185: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:193: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:204: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:212: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:222: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:230: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:241: error: expected ‘)’ before ‘*’ token
src/common/sandbox.h:244: error: expected ‘)’ before ‘*’ token
make[1]: *** [src/common/address.o] Error 1

I tried to fix the second one in my gsoc-cap-stage2 branch, but I don't know what to do for the first.

Small note: unfortunately by stepping through libseccomp with the debugger, I realised it doesn't support intervals on the same parameter because each filter entry applies to only one syscall parameter; so you couldn't have entry 1 say 'addr > x' and entry 2 say 'addr < x', unless they're done in separate calls; I did manage do make something work though.

I think this is almost okay.... It would be cool to test it out, though, by adding a spurious call to mprotect different points inside the mapping (including the beginning or end) or remap the mapping right after the sandbox filters are installed, and to verify that that does the right thing.

But hm. What happens if somebody tries to mprotect the page right before immediately before the mapping, and they give a bunch of pages that includes the mapping, as in "mprotect(pr_mem_base - 4096, 8192, PROT_READ|PROT_WRITE)" ?

(What exactly is it that needs to do mprotect(PROT_READ|PROT_WRITE)? I think it is malloc/arena.c in glibc.)

comment:17 in reply to:  16 Changed 6 years ago by nickm

Replying to nickm:

But hm. What happens if somebody tries to mprotect the page right before immediately before the mapping, and they give a bunch of pages that includes the mapping, as in "mprotect(pr_mem_base - 4096, 8192, PROT_READ|PROT_WRITE)" ?

(What exactly is it that needs to do mprotect(PROT_READ|PROT_WRITE)? I think it is malloc/arena.c in glibc.)

It appears that the largest arena that glibc will allocate now is 1 MB long. So I believe that we could kludge our way around this by mmaping a region that is 1MB larger than we need, putting our constant data at the end of it, and forbidding any attempt to mprotect(PROT_READ|PROT_WRITE) more than 1MB of data.

Is there a better kludge?

comment:18 Changed 6 years ago by nickm

(FWIW, I only looked at the source for glibc-2.17; I don't know if older/newer/other mallocs use mprotect(PROT_READ|PROT_WRITE) differently.)

comment:19 Changed 6 years ago by ctoader

Fixed linux x86_64 bug, libseccomp 1.0.1, where loading a filter was failing with -33. Notes:

  • send, recv are not defined on linux x86_64, libseccomp defines them as negative values (-108, -109).
  • they can be multiplexed using socketcall (they are on newer versions of libseccomp), but libseccomp 1.0.1 considers this to be wrong and returns an error.
  • fixed by ifdef-ing them out from the syscall filter for linux x86_64, as they are not required for this configuration (tested and works).

Fixed mprotect exploit as discussed on irc. Notes:

  • I have added the 1MB extra buffer before the protected strings buffer
  • mprotect may not be used with lengths higher than 1MB

I hope this is what you had in mind. Please let me know if there is anything else, or if the merge is done.

comment:20 Changed 6 years ago by nickm

This is looking better. I'll have another look in the morning once I'm fresh, and probably merge then. Thanks!

comment:21 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed
M
 E
  R
   G
    E
     D

Thanks!

Note: See TracTickets for help on using tickets.