Use PCRE to speed up file system jail #424

quantum5 · 2019-04-10T23:44:10Z

To do:

add basic PCRE support for open-like system calls
properly handle relative paths (relative to /proc/$pid/cwd)
add support for openat-like system calls with AT_FDCWD
add support for openat-like system calls with arbitrary fds

codecov-io · 2019-04-10T23:49:26Z

Codecov Report

Merging #424 into master will decrease coverage by 19.79%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
- Coverage   75.78%   55.98%   -19.8%     
==========================================
  Files         130      130              
  Lines        4790     4796       +6     
==========================================
- Hits         3630     2685     -945     
- Misses       1160     2111     +951

Impacted Files	Coverage Δ
dmoj/cptbox/handlers.py	`100% <100%> (ø)`	⬆️
dmoj/cptbox/chroot.py	`86.56% <100%> (+0.41%)`	⬆️
dmoj/cptbox/sandbox.py	`73.73% <100%> (+0.16%)`	⬆️
dmoj/executors/VB.py	`0% <0%> (-100%)`	⬇️
dmoj/executors/RUBY19.py	`0% <0%> (-100%)`	⬇️
dmoj/executors/CS.py	`0% <0%> (-100%)`	⬇️
dmoj/executors/C11.py	`0% <0%> (-100%)`	⬇️
dmoj/wbox/__init__.py	`0% <0%> (-100%)`	⬇️
dmoj/executors/RUBY18.py	`0% <0%> (-100%)`	⬇️
dmoj/executors/CPP17.py	`0% <0%> (-100%)`	⬇️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af50b9f...5fdc8d6. Read the comment docs.

Also test judge on python 3.7 and drop 3.4. Groovy and Scala temporarily dropped because support is difficult.

Xyene · 2019-04-11T13:28:06Z

dmoj/cptbox/_cptbox.pyx

@@ -341,13 +344,15 @@ cdef class Process:
    cdef public unsigned int _cpu_time
    cdef public int _nproc
    cdef unsigned long _max_memory
+    cdef bytes _re_fs_read


I don't really like this naming scheme, took several attempts before I parsed it correctly. _re_readable_fs?

Xyene · 2019-04-11T13:29:22Z

dmoj/cptbox/posixpath.cpp

+        if (!child.empty() && child[0] == '/') {
+            return child;
+        }
+        if (parent.empty() || parent.back() == '/') {


Xyene · 2019-04-11T13:29:29Z

dmoj/cptbox/posixpath.cpp

+        while (std::getline(stream, token, '/')) {
+            if (token.empty() || token == ".")
+                continue;
+            if (token != ".." || (!absolute && comps.empty()) ||


Xyene · 2019-04-11T13:29:41Z

dmoj/cptbox/posixpath.cpp

+            return ".";
+        }
+
+        bool absolute = path[0] == '/';


is_absolute

Xyene · 2019-04-11T13:33:59Z

dmoj/cptbox/ptproc.cpp

@@ -67,6 +83,66 @@ int pt_process::set_handler(int syscall, int handler) {
    return 0;
 }

+bool pt_process::set_re_fs_read(const char *pattern, int length, char *error,


Probably want to make this more generic, since we'll likely have the write equivalent of this in the future. Maybe have some struct wrapping all the pointers needed for a single regex?

Xyene · 2019-04-11T13:35:57Z

dmoj/cptbox/ptproc.cpp

@@ -12,9 +13,11 @@
 #include <unistd.h>
 #include <sys/ptrace.h>

+#include <string>


Include order.

Xyene · 2019-04-11T13:36:01Z

dmoj/cptbox/ptproc.cpp

 #include <set>

 #include "ptbox.h"
+#include "posixpath.h"


Include order.

Xyene · 2019-04-11T13:38:10Z

dmoj/cptbox/ptdebug.cpp

+    } else {
+        path += "/fd/" + std::to_string(fd);
+    }
+    int size = 1024;


PATH_MAX? pathconf?

Xyene · 2019-04-11T13:38:29Z

dmoj/cptbox/ptdebug.cpp

+    } else {
+        path += "/fd/" + std::to_string(fd);
+    }
+    int size = 1024;


Xyene · 2019-04-11T13:38:56Z

dmoj/cptbox/ptdebug.cpp

@@ -219,4 +220,42 @@ void pt_debugger::freestr(char *buf) {
    free(buf);
 }

+std::string pt_debugger::get_fd(int fd) {
+    std::string path = "/proc/" + std::to_string(getpid());


Is this the correct PID?

Xyene · 2019-04-11T13:47:27Z

dmoj/cptbox/ptproc.cpp

+
+    int result;
+    unsigned long file_ptr = (unsigned long) debugger->arg0();
+    char *file = debugger->readstr(file_ptr, 4096);


Unrelated to this patchset, but this is a function that has failure conditions and should be able to return nullptr. But it doesn't; the underlying code will just segfault.

Xyene · 2019-04-11T13:55:09Z

dmoj/cptbox/ptproc.cpp

+    std::string path(file);
+
+    debugger->freestr(file);
+    if (path.empty() || path[0] != '/') {


I'm not convinced you should be able to open("") and have it succeed. It'll likely block the access later, but it should block it here, because that's not an alias for the current working directory as this code implies.

Xyene · 2019-04-11T13:57:46Z

dmoj/cptbox/ptproc.cpp

+        return false;
+    }
+
+    int result;


Unused variable.

Xyene · 2019-04-11T13:58:18Z

dmoj/cptbox/ptproc.cpp

+        return false;
+    }
+
+    int result;


Unused variable.

Xyene · 2019-04-11T13:59:42Z

dmoj/cptbox/ptdebug.cpp

+        }
+
+        if (r < size) {
+            buffer[r] = 0;


'\0' for semantics.

Xyene · 2019-04-11T14:11:51Z

dmoj/cptbox/ptproc.cpp

+    int result;
+    int dirfd = (int) debugger->arg0();
+    unsigned long file_ptr = (unsigned long) debugger->arg1();
+    char *file = debugger->readstr(file_ptr, 4096);


This doesn't seem safe to me. What if the system path max is greater than 4096? Then something like print open('./' * 2048 + '../../etc/secrets').read() will validate successfully. This should either fail loudly if the path is longer then 4096, or perform another read.

Xyene · 2019-04-11T14:46:58Z

setup.py

@@ -27,6 +27,7 @@

 # Allow manually disabling seccomp on old kernels. WSL doesn't have seccomp.
 has_seccomp = not is_wsl and os.environ.get('DMOJ_USE_SECCOMP') != 'no'
+has_pcre = os.environ.get('DMOJ_USE_PCRE') != 'no'


If this is made optional, it should be added to the appropriate README.md section.

Xyene · 2021-08-28T21:45:54Z

This has been superseded by the plans in #871.

quantum5 requested review from Xyene, WallE256 and kiritofeng April 10, 2019 23:44

quantum5 force-pushed the pcre branch from 07cea12 to 8394c7e Compare April 10, 2019 23:49

Using xenial image on Travis (#425)

af50b9f

Also test judge on python 3.7 and drop 3.4. Groovy and Scala temporarily dropped because support is difficult.

quantum5 force-pushed the pcre branch from 8394c7e to efad239 Compare April 11, 2019 04:04

Add basic PCRE matching for open-like system calls

70facd3

quantum5 force-pushed the pcre branch from efad239 to 70facd3 Compare April 11, 2019 04:23

quantum5 added 2 commits April 11, 2019 02:38

Add path normalization

315c1ec

Add relative path handling

8e31679

quantum5 force-pushed the pcre branch from a37e81b to 8e31679 Compare April 11, 2019 06:38

Handle openat-like calls

5fdc8d6

quantum5 marked this pull request as ready for review April 11, 2019 06:48

Xyene requested changes Apr 11, 2019

View reviewed changes

Xyene reviewed Apr 11, 2019

View reviewed changes

dmoj/cptbox/ptproc.cpp

return false;

}

int result;

Copy link

Member

Xyene Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable.

Xyene reviewed Apr 11, 2019

View reviewed changes

dmoj/cptbox/ptproc.cpp

return false;

}

int result;

Copy link

Member

Xyene Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable.

Xyene reviewed Apr 11, 2019

View reviewed changes

dmoj/cptbox/ptdebug.cpp

}

if (r < size) {

buffer[r] = 0;

Copy link

Member

Xyene Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'\0' for semantics.

Xyene reviewed Apr 11, 2019

View reviewed changes

Xyene force-pushed the master branch from af50b9f to 1f5324d Compare April 15, 2019 04:28

Xyene force-pushed the master branch from ad3eb78 to 5d7a928 Compare February 28, 2021 01:10

Xyene closed this Aug 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PCRE to speed up file system jail #424

Use PCRE to speed up file system jail #424

quantum5 commented Apr 10, 2019 •

edited

Loading

codecov-io commented Apr 10, 2019 •

edited

Loading

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019 •

edited

Loading

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019

Xyene Apr 11, 2019 •

edited

Loading

Xyene Apr 11, 2019

Xyene commented Aug 28, 2021

Use PCRE to speed up file system jail #424

Use PCRE to speed up file system jail #424

Conversation

quantum5 commented Apr 10, 2019 • edited Loading

codecov-io commented Apr 10, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xyene Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xyene Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xyene commented Aug 28, 2021

quantum5 commented Apr 10, 2019 •

edited

Loading

codecov-io commented Apr 10, 2019 •

edited

Loading

Xyene Apr 11, 2019 •

edited

Loading

Xyene Apr 11, 2019 •

edited

Loading