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

Add sandboxing with Landlock #184

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

l0kod
Copy link

@l0kod l0kod commented Jul 1, 2022

p7zip handles a lot of archive formats, each bringing complexity and extending
its attack surface. Landlock is a Linux security sandboxing mechanism which
helps limit the impact of bug exploitation. These modifications enable to use
Landlock following a best-effort appoach: if Landlock is available on the
running system, then it will be used to sandbox p7zip, otherwise nothing will
change.

By default, all console commands (7z, 7za and 7zr) reading archives will create
a sandbox which denies basic file-system accesses (e.g. read, create or write
to files). All other commands (b, h, i, t) are untouched.

The sandbox is enforced when calling COpenCallbackConsole::Open_setTotal(),
which seems to be the more generic place for this purpose.

When extracting an archive, a new helper called GetOutputBaseDir() is used to
add security exception to the sandbox so that it will be allowed to create
files in the output directory.

See https://docs.kernel.org/userspace-api/landlock.html for the kernel UAPI
documentation.

p7zip handles a lot of archive formats, each bringing complexity and extending
its attack surface.  Landlock is a Linux security sandboxing mechanism which
helps limit the impact of bug exploitation.  These modifications enable to use
Landlock following a best-effort appoach: if Landlock is available on the
running system, then it will be used to sandbox p7zip, otherwise nothing will
change.

By default, all console commands (7z, 7za and 7zr) reading archives will create
a sandbox which denies basic file-system accesses (e.g. read, create or write
to files).  All other commands (b, h, i, t) are untouched.

FIXME: For now, only basic use of list (l) and extracts (e, x) commands is
working and tested.

The sandbox is enforced when calling COpenCallbackConsole::Open_setTotal(),
which seems to be the more generic place for this purpose.

When extracting an archive, a new helper called GetOutputBaseDir() is used to
add security exception to the sandbox so that it will be allowed to create
files in the output directory.

Signed-off-by: Mickaël Salaün <[email protected]>
@l0kod
Copy link
Author

l0kod commented Jul 1, 2022

This is a work-in-progress, but I'd like some feedback before continuing and rebasing on the p7zip21.07 branch. For now, only basic use of list (l) and extracts (e, x) commands is working and tested.

@l0kod
Copy link
Author

l0kod commented Jul 1, 2022

I'd also like to create a more consistent Ruleset class, but it is enough to test for now.

@l0kod
Copy link
Author

l0kod commented Jul 1, 2022

In case a draft PR doesn't sent notifications, ping @jinfeihan57.

@jinfeihan57
Copy link
Contributor

I don't konw much about Landlock.How much will this affect efficiency?

@l0kod
Copy link
Author

l0kod commented Nov 15, 2022

I don't konw much about Landlock.How much will this affect efficiency?

The performance impact will only be for the open* syscall (which may already vary according to the filesystem type), and such syscall impact should be negligible for archives with big files, but a bit more visible for archives with a lot of small files. Anyway, the performance impact should be small for most use cases. Landlock is part of the kernel and there is ongoing work to improve performance even if it is already decent.

I can do some benchmark but I encourage you to do so and share your methodology.

Sandboxing could be optional (e.g. depending on a command argument or a build option), but I would suggest to create a new command argument to (optionally) disable sandboxing, making p7zip protects its users by default.

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