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

Experimental Landlock based sandboxing #597

Merged
merged 3 commits into from
Dec 4, 2024
Merged

Experimental Landlock based sandboxing #597

merged 3 commits into from
Dec 4, 2024

Conversation

vlaci
Copy link
Contributor

@vlaci vlaci commented Jun 6, 2023

Implementation of #594 together with onekey-sec/unblob-native#11

Having to first create the extraction directory complicates things a lot.
I am unsure what approach we should take here but it can be seen, that the first directory needs somewhat special treatment.

Alternative integration approaches:

  • set LANDLOCK_ACCESS_FS_MAKE_DIR on the parent of the extraction root as an escape hatch
  • introduce a workdir where extraction takes place one level deeper to achieve simpler code.

@qkaiser
Copy link
Contributor

qkaiser commented Jun 6, 2023

I would be in favor of option 1 since it's how unblob works. The extraction path provided with -e is where we create the extraction directory so we do need LANDLOCK_ACCESS_FS_MAKE_DIR.

As long as we're clear about the fact that unblob limits itself to the path provided with -e and not the directory within, it's acceptable to me.

unblob/models.py Outdated Show resolved Hide resolved
unblob/processing.py Outdated Show resolved Hide resolved
unblob/processing.py Outdated Show resolved Hide resolved
@vlaci
Copy link
Contributor Author

vlaci commented Jun 6, 2023

I would be in favor of option 1 since it's how unblob works. The extraction path provided with -e is where we create the extraction directory so we do need LANDLOCK_ACCESS_FS_MAKE_DIR.

As long as we're clear about the fact that unblob limits itself to the path provided with -e and not the directory within, it's acceptable to me.

We need this for the parent of extraction directory:

LANDLOCK_ACCESS_FS_MAKE_DIR: Create (or rename) a directory.

And this for the parent directory of report file:

LANDLOCK_ACCESS_FS_MAKE_REG: Create (or rename or link) a regular file.

The latter seems a bit much to me

@vlaci vlaci force-pushed the landlock branch 4 times, most recently from d2138c8 to 3c51313 Compare June 7, 2023 21:01
@vlaci
Copy link
Contributor Author

vlaci commented Jun 7, 2023

Hehe, this change is incompatible with code coverage measurement :D

 INTERNALERROR>   File "/home/runner/.cache/pypoetry/virtualenvs/unblob-PkeEArhf-py3.8/lib/python3.8/site-packages/coverage/sqldata.py", line 1064, in _connect
INTERNALERROR>     raise DataError(f"Couldn't use data file {self.filename!r}: {exc}") from exc
INTERNALERROR> coverage.exceptions.DataError: Couldn't use data file '/home/runner/work/unblob/unblob/.coverage.fv-az626-878.3677.365646': unable to open database file

@qkaiser
Copy link
Contributor

qkaiser commented Jan 8, 2024

Two things to do before tagging as ready for review:

  • solve the ModuleNotFoundError: No module named 'unblob_native.sandbox' error by properly exposing it
  • add unit testing (unsupported platforms, different access checks)

@qkaiser qkaiser assigned qkaiser and unassigned qkaiser Jan 9, 2024
@qkaiser qkaiser added enhancement New feature or request dependencies Pull requests that update a dependency file labels Jan 9, 2024
@qkaiser qkaiser force-pushed the landlock branch 3 times, most recently from 59659a3 to c71c40a Compare February 4, 2024 10:47
@qkaiser qkaiser marked this pull request as ready for review February 4, 2024 10:49
@qkaiser
Copy link
Contributor

qkaiser commented Feb 21, 2024

@qkaiser
Copy link
Contributor

qkaiser commented Feb 21, 2024

Will rebase once version 0.1.2 of unblob-native is in.

@qkaiser
Copy link
Contributor

qkaiser commented Oct 11, 2024

Even though it seems to be supported by QEMU on aarch64 since qemu/qemu@3a2f19b, landlock syscalls are actually not supported in user mode emulation.

For demo purposes, let's write this quick-and-dirty PoC:

#include <stdio.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <errno.h>

int main() {
    int result = syscall(SYS_landlock_create_ruleset, NULL, 0, 0);

    if (result == -1) {
        printf("System call failed: %d (errno: %d)\n", result, errno);
    } else {
        printf("System call succeeded.\n");
    }

    return 0;
}

Compile it, make sure it's aarch64, run it in user mode emulation:

$ aarch64-linux-gnu-gcc -static poc.c
$ file a.out 
a.out: ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), statically linked, BuildID[sha1]=c42a43dc682c2dc504699d7b816e99a2e32c6bf0, for GNU/Linux 3.7.0, not stripped
$ qemu-aarch64-static ./a.out        
System call failed: -1 (errno: 38)

errno 38 is -ENOSYS or "function not implemented".

So problem is on QEMU user mode emulation not doing the translation for landlock syscalls. However, it should be handled gracefully by unblob-native I think.

Note: tested with qemu version 6.2.0 and 8.2.94

@qkaiser
Copy link
Contributor

qkaiser commented Oct 11, 2024

I built the sandboxer example from rust-landlock and it works on qemu-aarch64-static by returning an error:

$ LL_FS_RO="/bin:/lib:/usr:/proc:/etc:/dev/urandom" \
LL_FS_RW="/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp" \
LL_TCP_BIND="9418" LL_TCP_CONNECT="80:443" \
qemu-aarch64-static \
target/aarch64-unknown-linux-musl/debug/examples/sandboxer bash -i
Error: Landlock is not supported by the running kernel.

So the bug is probably in unblob-native or between unblob-native Rust and unblob-native Python. The exception is giving me the same feeling:

where False = isinstance(AttributeError("'int' object has no attribute 'results'"), PermissionError)

@vlaci
Copy link
Contributor Author

vlaci commented Oct 11, 2024

I'll add tests on the rust side as well then :)

@vlaci
Copy link
Contributor Author

vlaci commented Oct 11, 2024

I built the sandboxer example from rust-landlock and it works on qemu-aarch64-static by returning an error:

$ LL_FS_RO="/bin:/lib:/usr:/proc:/etc:/dev/urandom" \
LL_FS_RW="/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp" \
LL_TCP_BIND="9418" LL_TCP_CONNECT="80:443" \
qemu-aarch64-static \
target/aarch64-unknown-linux-musl/debug/examples/sandboxer bash -i
Error: Landlock is not supported by the running kernel.

So the bug is probably in unblob-native or between unblob-native Rust and unblob-native Python. The exception is giving me the same feeling:

where False = isinstance(AttributeError("'int' object has no attribute 'results'"), PermissionError)

Ahh, we need this error check at the end...
https://github.com/landlock-lsm/rust-landlock/blob/94721d26b2fd1151e71bd7a3aa5a43c463a22347/examples/sandboxer.rs#L133-L135

@l0kod
Copy link

l0kod commented Oct 11, 2024

Ahh, we need this error check at the end... https://github.com/landlock-lsm/rust-landlock/blob/94721d26b2fd1151e71bd7a3aa5a43c463a22347/examples/sandboxer.rs#L133-L135

Please note that kernels not supporting Landlock should not be an error for programs sandboxing themselves, only for sandboxers that must create sandboxes or error out (like this example). It can be a warning though.

@vlaci
Copy link
Contributor Author

vlaci commented Oct 11, 2024

Ahh, we need this error check at the end... https://github.com/landlock-lsm/rust-landlock/blob/94721d26b2fd1151e71bd7a3aa5a43c463a22347/examples/sandboxer.rs#L133-L135

Please note that kernels not supporting Landlock should not be an error for programs sandboxing themselves, only for sandboxers that must create sandboxes or error out (like this example). It can be a warning though.

Yep, unblob itself already just logs the problem, but I like clear failures in tests.
https://github.com/onekey-sec/unblob/pull/597/files#diff-2d4e67217491bddba5546f44b16c52ae6be29d568b52949950866ec5e4c24b0fR92-R95

@vlaci
Copy link
Contributor Author

vlaci commented Oct 11, 2024

Failing build resolved-by onekey-sec/unblob-native#65

unblob/pool.py Outdated Show resolved Hide resolved
unblob/sandbox.py Show resolved Hide resolved
@vlaci vlaci force-pushed the landlock branch 2 times, most recently from 01db1b2 to 71eac4c Compare October 16, 2024 16:52
@vlaci
Copy link
Contributor Author

vlaci commented Oct 16, 2024

The one thing requires thorough manual testing is keeping exit on CTRL-C and SIGTERM working. Many shenanigans are added for that purpose.

tests/test_cli.py Show resolved Hide resolved
tests/test_sandbox.py Show resolved Hide resolved
tests/test_sandbox.py Show resolved Hide resolved
unblob/processing.py Show resolved Hide resolved
unblob/sandbox.py Show resolved Hide resolved
unblob/pool.py Show resolved Hide resolved
unblob/pool.py Show resolved Hide resolved
unblob/pool.py Show resolved Hide resolved
unblob/pool.py Show resolved Hide resolved
unblob/sandbox.py Show resolved Hide resolved
unblob/sandbox.py Outdated Show resolved Hide resolved
@qkaiser
Copy link
Contributor

qkaiser commented Dec 4, 2024

@vlaci we're one conflict away from a merge I think :)

@qkaiser
Copy link
Contributor

qkaiser commented Dec 4, 2024

We need #1013 for tests to run :)

vlaci and others added 3 commits December 4, 2024 15:01
all other tests in this file assert on `process_file` being called
with correct arguments. We need specific tests which test that the
configuration is interpreted correctly
Instead of juggling with signal handlers and hoping that
`ShutDownRequired` will be fired in the appropriate place in
`multiprocessing.BasePool`, on exceptional termination, we signal
workers via `SIGTERM`.

As a side-effect this makes it possible to run `process_file` in
non-main thread.
@qkaiser qkaiser enabled auto-merge December 4, 2024 21:45
@qkaiser qkaiser merged commit c31b05e into main Dec 4, 2024
15 checks passed
@qkaiser qkaiser deleted the landlock branch December 4, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants