[ML] Add system call restrictions to the ML processes #98

davidkyle · 2018-05-21T16:14:05Z

The autodetect, autoconfig, categorize and normalize programs all require the ability to create a named pipe (mkfifo/CreateNamedPipe) open a connection to a named pipe and read & write to those pipes. All other system calls which are not required such as fork/exec should be restricted using platform specific APIs.

The code here is similar to SystemCallFilter.java.

LibSeccomp

This PR adds a new static library libseccomp containing a single class CSystemCallFilter the library must be linked in the 4 main programs listed above.

Due to static linkage remember to make relink the unit test and binaries after making a change.

macOS

macOS has the most configurable sandboxing functionality the approach here is to blacklist everything by default but allow read* and write* for open, mkfifo, read and write. This imposes harder limits than the SystemCallFilter.java which only denies fork and exec.

Linux

Seccomp BPF was added to prctl() in kernal 3.5, no filters will be installed in older kernals. The dedicated seccomp() function was added in kernal 3.17 and provides the same the functionality as prctl() but with the SECCOMP_FILTER_FLAG_TSYNC option that is not required here if the filters are installed by the main thread before any other threads are spawned. Because of this we use the older more compatible prctl() function.

SystemCallFilter.java tries to use seccomp() and falls back to prctl()

Windows

The Windows implementation uses Job Objects to prevent the process spawning another.

Release note: Secures the machine learning processes by preventing system calls such as fork
and exec. The Linux implementation uses Seccomp BPF (secure computing with
Berkeley Packet Filters) to intercept system calls and is available in kernels
since 3.5. On Windows, Job Objects prevent new processes being created and macOS
uses the sandbox functionality

droberts195 · 2018-05-21T16:34:53Z

execve, fork and vfork are disallowed should any other system calls be? fexecve? execveat?

Yes, execveat should definitely be disallowed (if it exists). I guess the complication here is that it only exists on kernel 3.19 and above. (And I think fexecve is the libc function that uses execveat rather than a system call itself?)

I think also socket, socketcall and socketpair should be disallowed, because we wanted to prevent network access. If we stop people getting hold of a socket then hopefully that will avoid the need to list every single other network function.

hendrikmuhs · 2018-05-22T07:52:13Z

lib/seccomp/CSystemCallFilter_Linux.cc

+        }
+
+    } else {
+        LOG_DEBUG(<< "Seccomp BPF not available");


could imagine this being info as it might be useful for customers to know

I'm not sure. Upgrading this would mean customers running on CentOS 6 would be spammed every time an ML process starts up. And normalize can run pretty frequently...

It started as info level but as Dave points out every time normalize starts the message is logged and this can spam the log files

Actually we could log this message in autodetect main only. It is a useful message

Remember ES itself will have logged this once at startup. Logging in autodetect will still log 100 times if 100 jobs are started.

hendrikmuhs · 2018-05-22T07:56:06Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+    // In production this needs to match the setting of java.io.tmpdir.  We rely
+    // on the JVM that spawns our controller daemon setting TMPDIR in the
+    // environment of the spawned process.
+    const char* tmpDir(::getenv("TMPDIR"));


consider using boost for that, it handles some more cases afaik and can be easily replaced with std once we switch to C++17

I'm not sure what you mean is there a boost version of getenv? I'm using the temp dir set by elasticsearch

I mean boost::filesystem::temp_directory_path()

hendrikmuhs · 2018-05-22T07:58:27Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+std::string getTempRulesFilename() {
+    std::string tempDir = getTempDir();
+    // randomise the sandbox rules filename
+    char * tempName = ::tempnam(tempDir.c_str(), "ml");


consider boost::filesystem::unique_path

mkstemps is the safest way to do this as it avoids a race condition between creating and opening the file. I pushed a change to do this, unfortunately it uses strlcpy and strlcat and file descriptors.

droberts195

I left quite a few comments about low level things but the fundamental structure of this is exactly right in my opinion.

One more thing is that some of the formatting looks a little different to what clang-format would choose, so please run dev-tools/clang-format.sh over the whole codebase.

droberts195 · 2018-05-22T07:59:20Z

include/seccomp/CSystemCallFilter.h

+//!
+class CSystemCallFilter {
+public:
+    CSystemCallFilter();


I think we should use a static method like installSystemCallFilter() rather than the constructor. Using the constructor implies an RAII idiom where the destructor reverses what the constructor has done. But that's impossible for system call filters.

Similar classes would be CCrashHandler and CProcessPriority and they use a static method rather than the constructor.

droberts195 · 2018-05-22T08:00:02Z

include/seccomp/CSystemCallFilter.h

+//! Installs Sandbox/sys call filters
+//!
+//! DESCRIPTION:\n
+//!


Could say the intention is to prevent starting new processes and, where possible, preventing network access.

droberts195 · 2018-05-22T08:00:50Z

lib/seccomp/CSystemCallFilter_Linux.cc

+
+namespace ml {
+namespace seccomp {
+


Everything from here down to the end of canUseSeccompBpf() should be in an unnamed namespace to enforce internal linkage.

droberts195 · 2018-05-22T08:01:08Z

lib/seccomp/CSystemCallFilter_Linux.cc

+unsigned int SECCOMP_DATA_ARCH_OFFSET = 0x04;
+
+// Copied from seccomp.h
+// seccomp.h cannot be included as it was added in Linux kernal 3.17


typo: kernal -> kernel

droberts195 · 2018-05-22T08:02:32Z

lib/seccomp/CSystemCallFilter_Linux.cc

+
+// The old x32 ABI always has bit 30 set in the sys call numbers.
+// The x64 architecture should fail these calls
+unsigned int UPPER_NR_LIMIT = 0x3FFFFFFF;


I think this and the two below should be const std::uint32_t (plus #include <cstdint>). The struct member they're used with is explicitly 32 bits, and whilst unsigned int is 32 bits on all the compilers we're currently using it's best practice to specify the size exactly where the consuming code does.

droberts195 · 2018-05-22T08:42:51Z

lib/seccomp/unittest/CSystemCallFilterTest.cc

+    CPPUNIT_ASSERT(semVersion.tokenise(release, tokens));
+    // Seccomp is available in kernals since 3.5
+    if (std::stoi(tokens[0]) < 3 || std::stoi(tokens[1]) < 5) {
+        LOG_INFO(<< "Cannot test seccomp on linux kernals before 3.5");


typo: kernals -> kernels

droberts195 · 2018-05-22T08:43:16Z

lib/seccomp/unittest/CSystemCallFilterTest.cc

+    // system call filters are applied
+    CPPUNIT_ASSERT(systemCall());
+
+    // // Install the filter


nit: double //

droberts195 · 2018-05-22T08:47:54Z

lib/seccomp/unittest/Makefile

+TARGET=ml_test$(EXE_EXT)
+
+USE_BOOST=1
+USE_BOOST_REGEX_LIBS=1


You shouldn't need this as lib/core has it and only lib/core is using the Boost regex lib directly. (If you do need it then it's a sign something isn't working as expected.)

droberts195 · 2018-05-22T08:50:43Z

lib/seccomp/unittest/Makefile

+USE_BOOST_REGEX_LIBS=1
+
+LIBS:=$(LIB_ML_SECCOMP)
+


For consistency the linker flags should be added here too, i.e. LDFLAGS:=$(ML_SECCOMP_LDFLAGS)

droberts195 · 2018-05-22T08:51:39Z

mk/stdcpptest.mk

@@ -15,7 +15,7 @@ REQUIRED_LIBS=$(LIB_ML_TEST) $(LIB_ML_CORE) $(LIB_ML_VER) $(CPPUNITLIBS)
 LIBS:=$(LOCALLIBS) $(filter-out $(REQUIRED_LIBS), $(LIBS)) $(REQUIRED_LIBS)

 CPPFLAGS+=$(INCLUDE_PATH)
-LDFLAGS:=$(UTLDFLAGS) $(LDFLAGS) $(LIB_PATH) $(ML_VER_LDFLAGS)
+LDFLAGS:=$(UTLDFLAGS) $(LDFLAGS) $(LIB_PATH) $(ML_VER_LDFLAGS) $(ML_SECCOMP_LDFLAGS)


Since we're not linking the seccomp library to every unit test program I think the linker flags should also be added just in the one unit test program that tests it.

tveasey · 2018-05-22T10:49:00Z

docs/CHANGELOG.asciidoc

@@ -35,6 +35,10 @@ Explicit change point detection and modelling ({pull}92[#92])
 Forecasting of Machine Learning job time series is now supported for large jobs by temporarily storing
 model state on disk ({pull}89[#89])

+Secure the ML processes by preventing system calls such as fork and exec. The Linux implemenation uses
+Seccomp BPF to intercept system calls and is available in kernels since 3.5. On Windows Job Objects prevent
+new proceses being created and macOs uses the sandbox functionality ({pull}98[#98])


proceses -> processes

tveasey · 2018-05-22T10:56:08Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+    char * tempName = ::tempnam(tempDir.c_str(), "ml");
+    if (tempName != nullptr) {
+        return std::string(tempName) + ".sb";
+        ::free(tempName);


free after return? But goes away with boost anyway.

droberts195

I left a few more minor comments/suggestions.

droberts195 · 2018-05-22T14:38:40Z

docs/CHANGELOG.asciidoc

@@ -35,6 +35,10 @@ Explicit change point detection and modelling ({pull}92[#92])
 Forecasting of Machine Learning job time series is now supported for large jobs by temporarily storing
 model state on disk ({pull}89[#89])

+Secure the ML processes by preventing system calls such as fork and exec. The Linux implemenation uses
+Seccomp BPF to intercept system calls and is available in kernels since 3.5. On Windows Job Objects prevent
+new processes being created and macOs uses the sandbox functionality ({pull}98[#98])


nit: I think the official name is "macOS" with a captial S at the end

droberts195 · 2018-05-22T14:41:13Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+    (allow file-write-data)");
+
+// mkstemps will replace the Xs with random characters
+static const char FILE_NAME_TEMPLATE[] = {"ml.XXXXXX.sb"};


It's not necessary to declare variables static within an unnamed namespace - simply being inside the unnamed namespace gives them internal linkage. (I can see a few files where I've unnecessarily added static in this situation in the past, so I'll remove those in a follow-up.)

droberts195 · 2018-05-22T14:42:45Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+
+    // Create and open a temporary file with a random name
+    // templateBuff is updated with the new filename.
+    // 3 is the size of the suffix ".sb"


I think it would be nicer if the 3 was a symbolic constant (e.g. FILE_NAME_TEMPLATE_SUFFIX_LEN) defined adjacent to FILE_NAME_TEMPLATE. Then anyone changing FILE_NAME_TEMPLATE would be more likely to see they also needed to change the 3.

droberts195 · 2018-05-22T14:44:52Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+    // Create and open a temporary file with a random name
+    // templateBuff is updated with the new filename.
+    // 3 is the size of the suffix ".sb"
+    int fd = mkstemps(templateBuff.get(), 3);


You could avoid the C-style string functions by doing something like this:

std::string profileFilename = getTempDir() + FILE_NAME_TEMPLATE; int fd = mkstemps(&profileFilename[0], FILE_NAME_TEMPLATE_SUFFIX_LEN);

Also, the documentation for mkstemps() states that it can fail and set errno, so maybe log the error and return an empty string in that case.

Is that safe? Is &profileFilename[0] null terminated?

It is in C++11 and above, but not previous standards.

https://stackoverflow.com/a/11752722 summarizes the situation.

droberts195 · 2018-05-22T14:49:10Z

lib/seccomp/CSystemCallFilter_Windows.cc

+
+namespace {
+
+struct CheckedHandle {


Should be SCheckedHandle as our structs begin with S

droberts195 · 2018-05-22T14:51:41Z

include/seccomp/CSystemCallFilter.h

+//!
+class CSystemCallFilter {
+public:
+    CSystemCallFilter();


I think we should use a static method like installSystemCallFilter() rather than the constructor. Using the constructor implies an RAII idiom where the destructor reverses what the constructor has done. But that's impossible for system call filters.

CCrashHandler and CProcessPriority are conceptually similar and they use a static method rather than the constructor.

droberts195

Thanks for making all those changes.

LGTM

droberts195 · 2018-05-23T13:23:27Z

lib/seccomp/unittest/CSystemCallFilterTest.cc

+const char* TEST_WRITE_PIPE_NAME = "\\\\.\\pipe\\testwritepipe";
+#else
+const char* TEST_READ_PIPE_NAME = "testreadpipe";
+const char* TEST_WRITE_PIPE_NAME = "testwritepipe";


To have internal linkage these 4 const char* constants should really be const char* const.

droberts195 · 2018-05-23T13:25:37Z

lib/seccomp/CSystemCallFilter_MacOSX.cc

+    (allow file-write-data)");
+
+// mkstemps will replace the Xs with random characters
+const char* FILE_NAME_TEMPLATE = "ml.XXXXXX.sb";


You could make this a const std::string, as that's what we usually use for string constants unless there's some horrible circular dependency in linkage or static initialization.

But if you stick with a const char* then it really should be const char* const to result in internal linkage.

droberts195 · 2018-05-23T14:31:37Z

lib/seccomp/CSystemCallFilter_Linux.cc

+    BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_exit, 4, 0),
+    BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_statfs, 3, 0),
+    BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_dup2, 2, 0),
+    BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_open, 1, 0),


We call dladdr() and raise() as part of the crash handler. I think it would be worth double checking that these don't require a system call that's not on the list if you haven't done so already.

davidkyle · 2018-05-23T14:36:44Z

I pushed a commit that inverts the BPF approach on linux so instead of listing banned syscalls I list all the ones that can be used. This has the advantage that we don't have to think of every syscall that should be blocked and covers new functions (e.g. execveat added in kernel 3.19). The downside is that a different distribution makes unusual syscalls then the processes won't function. I do not know how much of a risk that is.

strace is useful for finding the syscalls a program makes for example the output from running the seccomp unit tests:

$strace -c ML_CPP_HOME/lib/seccomp/unittest/ml_test
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 43.50    0.001027        1027         1           wait4
 15.67    0.000370           3       114        80 open
  8.68    0.000205          23         9           brk
  8.30    0.000196           2        86           mmap
  6.01    0.000142           4        34        23 stat
  3.64    0.000086           2        53           mprotect
  2.20    0.000052          26         2         2 connect
  2.12    0.000050           3        18           write
  2.03    0.000048           1        36           close
  1.95    0.000046          46         1           readlink
  1.74    0.000041           1        35           read
  1.57    0.000037           1        34           fstat
  0.59    0.000014           7         2         2 access
  0.55    0.000013          13         1           execve
  0.51    0.000012           6         2           lstat
  0.25    0.000006           3         2           socket
  0.21    0.000005           2         3           clone
  0.17    0.000004           1         3         1 prctl
  0.08    0.000002           0        14           rt_sigaction
  0.08    0.000002           2         1           getcwd
  0.04    0.000001           0         3           rt_sigprocmask
  0.04    0.000001           1         2           getuid
  0.04    0.000001           1         2           futex
  0.00    0.000000           0         1           lseek
  0.00    0.000000           0         6           munmap
  0.00    0.000000           0         1           uname
  0.00    0.000000           0         1           getrlimit
  0.00    0.000000           0         1           arch_prctl
  0.00    0.000000           0         1           set_tid_address
  0.00    0.000000           0         1           set_robust_list
------ ----------- ----------- --------- --------- ----------------

To get the full list I change to filter to return SECCOMP_RET_KILL so the error would be logged then ran the integration tests adding syscalls to the filter until the tests stopped failing.

andrewkroh

I looked over the Linux sections. 👍

Restrict the ability of the autodetect, autoconfig, categorize and normalize programs to make system calls. Implemented for Linux (Seccomp BPF), macOS (sandbox) and Windows (Job Groups)

Add system call restrictions to the ML processes (#98) Restrict the ability of the autodetect, autoconfig, categorize and normalize programs to make system calls. Implemented for Linux (Seccomp BPF), macOS (sandbox) and Windows (Job Groups) * [ML] Add comment explaining changes required if Linux BPF are modified.

davidkyle added v7.0.0 review :ml v6.4.0 labels May 21, 2018

hendrikmuhs reviewed May 22, 2018

View reviewed changes

droberts195 reviewed May 22, 2018

View reviewed changes

tveasey reviewed May 22, 2018

View reviewed changes

davidkyle force-pushed the seccomp-lib branch from edb69fa to 801cb91 Compare May 22, 2018 11:52

droberts195 added the >enhancement label May 22, 2018

droberts195 reviewed May 22, 2018

View reviewed changes

droberts195 approved these changes May 23, 2018

View reviewed changes

droberts195 reviewed May 23, 2018

View reviewed changes

davidkyle and others added 11 commits May 24, 2018 13:45

Seccomp library

726e900

OSX implementation

03d0643

Add system call filter to executables

e5b8c62

Randomise the sandbox rules filename

9ba3f98

Tidy up the tests

a0c769f

Linux implementation

e3ea1e4

Fix compilation and test on pre 3.5 kernals

af710ee

Tidy up

05585e2

Windows Implementation

9c76edb

Add change log description and document CSystemCallFilter class

225350d

Address review comments

a2b6cf4

David Kyle and others added 6 commits May 24, 2018 13:45

Fix platform compilation

0f27509

Clang format

8832d23

Change to static method installSystemCallFilter()

e9be419

use const char* const

7e31ed8

Use allowed list instead of banned list for linux syscalls

aa4236c

Add more sys calls to filter

25d85fb

davidkyle force-pushed the seccomp-lib branch from ec7cdb6 to 25d85fb Compare May 24, 2018 12:46

Remove socket from allowed syscalls

46d3100

andrewkroh approved these changes May 25, 2018

View reviewed changes

davidkyle merged commit c15c7aa into elastic:master May 28, 2018

davidkyle deleted the seccomp-lib branch May 28, 2018 21:07

davidkyle mentioned this pull request May 29, 2018

[6.x] [ML] Add system call restrictions to the ML processes #106

Merged

sophiec20 added :ml and removed :ml labels Jun 12, 2018

		USE_BOOST_REGEX_LIBS=1

		LIBS:=$(LIB_ML_SECCOMP)

[ML] Add system call restrictions to the ML processes #98

[ML] Add system call restrictions to the ML processes #98

Uh oh!

Conversation

davidkyle commented May 21, 2018 • edited by lcawl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LibSeccomp

macOS

Linux

Windows

Uh oh!

droberts195 commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tveasey May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

davidkyle commented May 21, 2018 •

edited by lcawl

Loading

tveasey May 22, 2018 •

edited

Loading