ioi / isolate

Sandbox for securely executing untrusted programs
Other
1.1k stars 161 forks source link

CPU time (--time) consumed in subsequent runs in the same box with cg (cgroup2) enabled #158

Closed Brikaa closed 2 months ago

Brikaa commented 5 months ago

Since we get the cpu time from cpu.stat in the cgroup which does not get cleared between multiple isolate --cg --runs, the cpu time of a run is subtracted from the cpu time of the subsequent run. So if the first run passes the entire time limit, the second run will not get a chance.

Is this behavior intended? It does not seem to happen without --cg.

gollux commented 5 months ago

This is indeed wrong. It evaded our tests because we seldom use --run multiple times within the lifetime of a sandbox.

I will fix it soon.

Brikaa commented 3 months ago

My current workflow is to create a temporary directory (777 permissions) on the host for each submission, and to create a sandbox for the compile command and another one for the run command mounting the directory rw to both sandboxes. It seems to be a good workflow since sudo won't be needed to move the source files inside the sandbox (e.g, moving the source files to /var/lib/isolate/0/box).

gollux commented 3 months ago

Appealing as this workflow might be, I do not recommend it for security reasons. Sandboxed processes can create subdirectories within the temporary directory, which your program cannot remove.

Moving source files inside the sandbox should be easy: once you initialize the sandbox, the box directory is owned by the calling user, which can copy input files there. Before isolate --run executes the sandboxed process under a sandbox user, it changes ownership of all files in the box to the sandboxed process. And once the process finishes, owhership is changed back, so the calling user can read the output.

Brikaa commented 3 months ago

Ah, interesting. But there is another problem: I don't want compilation to take up some of the running time (as described in this issue). So compilation and running can't happen within the same sandbox.

Sandboxed processes can create subdirectories within the temporary directory, which your program cannot remove.

Hmm, why wouldn't the program be able to remove them if the parent directory has 777 permissions, is owned by the program's user and does not have the sticky bit?

gollux commented 3 months ago

Sorry, the problematic case is a little bit different: the sandboxed program creates a subdirectory with 0700 permissions and then creates a file within it. Other processes cannot unlink the file because of subdirectory permissions and they also cannot change the permissions, since it can be done only by the owner of the inode.

Brikaa commented 3 months ago

Oh, got it, thanks! So is there a solution to the following problem other than re-adjusting the limits based on the compilation metrics?:

But there is another problem: I don't want compilation to take up some of the running time (as described in this issue). So compilation and running can't happen within the same sandbox.

Thanks for your time

Brikaa commented 3 months ago

What I have in mind is either adjusting the running stage limits based on metrics from compiling stage with the normal workflow (moving stuff inside box), or a setuid script that cleans up the directory with the above workflow.

gollux commented 3 months ago

I hope to fix the problem with the limits soon. Sorry, I've been very busy with preparations for the Central-European olympiad in informatics which happens next week.

Brikaa commented 3 months ago

Thanks and good luck with the Central-European olympiad in informatics.

gollux commented 2 months ago

It should be fixed now, please check.

maxkt commented 2 months ago

@gollux Thanks for fixing this! It unblocks isolate with cgroupsv2 for us and we're going to migrate soon. Should we wait for a new release tag on the master branch or is it OK just to use the latest commit?

gollux commented 2 months ago

I am travelling now, so the release will probably take some time. In the meantime, please use the latest commit.

Brikaa commented 2 months ago

Thanks for the fix @gollux.

I am still not sure if we should switch to compiling and running in the same box. The reported memory usage describes the peak usage across all runs (both the compilation and the running stage). This can be misleading for contestants who do #include <bits/stdc++.h> in C++ since the compilation would use a lot of memory.

This was discussed in #136.

gollux commented 2 months ago

I am still not sure if we should switch to compiling and running in the same box.

I don't think you should. I always suggested using separate boxes.

Brikaa commented 2 months ago

Okay, I currently move the compilation result files from the compilation box to the new execution box.