Avoid bind-mounting temporary dirs within sandbox

[gollux]

I'm not sure if this is a bug report or a feature request :-)

Currently, CMS puts files in the sandbox by creating a temporary directory, initializing a sandbox, and bind-mounting the directory inside the sandbox as /tmp. This is not the approach recommended by Isolate's documentation and it leads to multiple problems with file owners and permissions, which might have security implications.

Isolate is started by cmsuser (the user running the worker services), but the process inside the sandbox runs on its own UID (each sandbox has its own). The CMS's temporary directory is owned by cmsuser and writable by everybody. It allows the sandbox user write in this directory. However, if the sandbox user creates a sub-directory not writable by everybody, CMS is then unable to remove its contents, leaving the temporary directory in /tmp forever. Also, the temporary directory is writable by all other users of the system.

The approach recommended by Isolate's documentation is to use the /box directory inside the sandbox. When the sandbox is created, this directory is owned by the caller. When isolate --run is called, the owner of all files inside /box is changed to the sandbox user, and once the sandboxed process finishes, the owner is changed back. This avoids all of the permission problems mentioned above.

I would very much recommend changing the way Isolate uses the sandbox to the recommended one. See also #1005.

Also, when we are at it, we should replace setting the file size limit by proper filesystem quotas (#916).

[prandla]

I tried implementing this in our fork here. However, I think this is not exactly ideal: as the box directory is now owned by the isolate user, they can always create new files in it or change the permissions on other files to override the allow_writing_only mechanism (currently, for batch tasks, the submission is only allowed to write to the stdout and stderr files). For now i just made all the allow_writing_* functions empty stubs. I think this means proper filesystem quotas are in fact necessary to prevent resource exhaustion. Also, this is more of a problem with my current implementation than with the concept, but cleanup fails when the submission chmods some of its files such that shutil.copytree() is unable to read them, I guess we would need to chmod them back before copying.

[gollux]

Thanks for your try!

I don't think that allow_writing_only makes any sense. Even in the current master, there exist places where the solution can write to. And running solutions without a proper filesystem quota was never secure.

Also, this is more of a problem with my current implementation than with the concept, but cleanup fails when the submission chmods some of its files such that shutil.copytree() is unable to read them, I guess we would need to chmod them back before copying.

I think that if the solutions explicitly made some files unreadable, we can safely assume they don't exist :)

[prandla]

Thinking about this again. How do we want to handle keep_sandbox after implementing this? We can't just leave the box dir in /var/lib/isolate, because then we can't reuse the box ID number. In the implementation I linked earlier, I copy the entire box directory to /tmp/cms-xxxxxx before doing isolate --cleanup. But I feel like this ruins most of the point of keep_sandbox: in order to experiment with the sandbox (i.e. rerun the isolate commands with different arguments), you would need to copy the directory back manually. And if you just wanted to inspect the sandbox, then you can do this from AWS directly now. (Well, almost directly, you still need to download and unpack it, but close enough.)

Also, implementing the copying in a robust manner is hard if we want to preserve permissions correctly. And it feels like debugging permission problems could be a great use for keep_sandbox.

[gollux]

I would drop keep_sandbox completely and just use the sandbox archival feature you added.

In the future, it would be nice to have a command-line tool that populates a given sandbox exactly as when evaluating a given submission, so that you could easily experiment on it. But I don't think it's easy to achieve though.