Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed syscalls open and openat from policy defined in addExecutionControlRules #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mikimasn
Copy link
Contributor

Syscalls open and openat were handled two times.

Once without any restrictions in

"open",
"epoll_create1",
"openat"});

And the second time with a check to enforce read-only mode on the file system

rules_.emplace_back(SeccompRule(
"open",
action::ActionAllow(),
(filter::SyscallArg(1) & (O_RDWR | O_WRONLY)) == 0));
rules_.emplace_back(SeccompRule(
"openat",
action::ActionAllow(),
(filter::SyscallArg(2) & (O_RDWR | O_WRONLY)) == 0));

The first policy was more permissive and made the second one useless(it always allowed syscall open without checking access mode) so I removed it.

…ontrolRules due to this syscalls being handled by policy defined in addFileSystemAccessRules
@Wolf480pl
Copy link
Member

Wolf480pl commented Feb 11, 2024

Looks like the duplicate open/openat were added by https://github.com/sio2project/sio2jail/pull/27/files?diff=unified&w=1

I looked through OI admins' internal chat logs and it looks like we allowed those syscalls in response to python3.9+numpy having an issue:

intercepted forbidden syscall openat(257) (4294967196, 47589183160736, 524481, 420, 0, 3346281667394566245)

which means

openat(-100, somepointer, 0o2000301, 0o644)

a.k.a.

openat(AT_FDCWD, somepointer, O_CLOEXEC | O_EXCL | O_CREAT | O_WRONLY, 0644 /* rw-r--r-- */)

but that error was difficult to reproduce (it only happened on old kernels)

In any case, I think the right thing to do would be to return either EPERM or EROFS when someone's trying to open a file for writing - the libraries that do it implicitly hopefully handle that error gracefully.

Unfortunately this means when a contestant's program explicitly tries to create a temporary file, we can't explicitly report that as a Rule Violation, instead the program will probably fail to handle the error and the contestant will see a generic Runtime Error. But that was already an issue with the changes introduced in #27 so I guess we'll have to live with it.

When I have time, I'll try to reproduce the error with python3.9+numpy.

Meanwhile, you can change this part

rules_.emplace_back(SeccompRule(
"open",
action::ActionAllow(),
(filter::SyscallArg(1) & (O_RDWR | O_WRONLY)) == 0));
rules_.emplace_back(SeccompRule(
"openat",
action::ActionAllow(),
(filter::SyscallArg(2) & (O_RDWR | O_WRONLY)) == 0));

to return EROFS or EPERM when open is not read-only, maybe similar to this
syscallRules_.emplace_back(seccomp::SeccompRule(
"clone",
seccomp::action::ActionAllow{},
(Arg(2) & CLONE_VM) == CLONE_VM));
syscallRules_.emplace_back(seccomp::SeccompRule(
"clone",
seccomp::action::ActionKill(),
(Arg(2) & CLONE_VM) == 0));

so that we don't have to worry which rules apply first.

@mikimasn
Copy link
Contributor Author

mikimasn commented Feb 11, 2024

Now attempts to open a file in write mode when read-only mode is enforced fail with EROFS error.

src/seccomp/policy/DefaultPolicy.cc Outdated Show resolved Hide resolved
src/seccomp/policy/DefaultPolicy.cc Outdated Show resolved Hide resolved
@mikimasn
Copy link
Contributor Author

Sorry for taking so long to make changes but I was participating in the competition and didn't have time to do it.

@Wolf480pl
Copy link
Member

no worries, it'll take me long to test it anyway :P

@mikimasn
Copy link
Contributor Author

mikimasn commented Apr 9, 2024

How is the testing going?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants