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

DRAFT: Add landlock support #575

Merged
merged 4 commits into from
Jun 8, 2024
Merged

DRAFT: Add landlock support #575

merged 4 commits into from
Jun 8, 2024

Conversation

valoq
Copy link
Contributor

@valoq valoq commented May 25, 2024

This adds landlock support to the sandbox feature.

While the seccomp filter should already prevent writing to files, this adds another safeguard to prevent file modifications.

Todo:

  • fix dependency check in meson (check for kernel version)
  • cleanup landlock.c (remove unused functions)

@sebastinas
Copy link
Member

This looks much more maintainable then the seccomp filters. So could we just drop the seecomp filter and move to landlock? With the ability to restrict on a path-basis, this would also enable the use of dbus and stop crippling the user experience in strict sandbox mode.

@valoq
Copy link
Contributor Author

valoq commented May 26, 2024

This looks much more maintainable then the seccomp filters. So could we just drop the seecomp filter and move to landlock? With the ability to restrict on a path-basis, this would also enable the use of dbus and stop crippling the user experience in strict sandbox mode.

While this certainly provides a simple way to isolate the filesystem, it does not address the kernel attack surface at all. For that we need to filter system calls as well.
That said, it may be a good idea to reintroduce the normal sandbox mode (and make it default again) which could rely only on landlock for filesystem isolation, since that would address most low hanging fruits that an adversary would target.

In order to make a complete sandbox that does not block any features, we would need to split the zathura process and have one sandboxed renderer process that is managed through IPC by a primary zathura process (like openssh and modern browsers do it).
I have been looking into this for a while, but it does require quite a bit of work since splitting the memory management is not easy and requires changes to the plugin API.
The most reasonable approach to this would be something like https://github.com/google/sandboxed-api which could be implemented in libraries like poppler and mupdf. That would also minimized the required changes to zathura and benefit more project that use those libs.

@valoq
Copy link
Contributor Author

valoq commented May 29, 2024

For now I kept some of the currently unused code in case we do use it for an alternative sandbox version.

Also credit for most of the code goes to @l0kod and his documentation.

meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
zathura/landlock.c Show resolved Hide resolved
zathura/landlock.c Show resolved Hide resolved
zathura/landlock.c Outdated Show resolved Hide resolved
zathura/landlock.c Outdated Show resolved Hide resolved
zathura/landlock.c Show resolved Hide resolved
zathura/landlock.h Show resolved Hide resolved
@sebastinas
Copy link
Member

This looks much more maintainable then the seccomp filters. So could we just drop the seecomp filter and move to landlock? With the ability to restrict on a path-basis, this would also enable the use of dbus and stop crippling the user experience in strict sandbox mode.

While this certainly provides a simple way to isolate the filesystem, it does not address the kernel attack surface at all. For that we need to filter system calls as well. That said, it may be a good idea to reintroduce the normal sandbox mode (and make it default again) which could rely only on landlock for filesystem isolation, since that would address most low hanging fruits that an adversary would target.

I am not sure if seccomp is actually a maintainable approach in the long run. The last years have shown that it requires whitelisting of platform specific syscalls and they change without notice depending on the dependencies. It would potentially make more sense to provide flatpak images (or comparable) for some sandboxing without breaking features and a separate zathura-seccomp binary for people that want it.

In order to make a complete sandbox that does not block any features, we would need to split the zathura process and have one sandboxed renderer process that is managed through IPC by a primary zathura process (like openssh and modern browsers do it). I have been looking into this for a while, but it does require quite a bit of work since splitting the memory management is not easy and requires changes to the plugin API. The most reasonable approach to this would be something like https://github.com/google/sandboxed-api which could be implemented in libraries like poppler and mupdf. That would also minimized the required changes to zathura and benefit more project that use those libs.

The likely hood of that being merged is close to 0.

@valoq
Copy link
Contributor Author

valoq commented Jun 2, 2024

Thanks for the review

This looks much more maintainable then the seccomp filters. So could we just drop the seecomp filter and move to landlock? With the ability to restrict on a path-basis, this would also enable the use of dbus and stop crippling the user experience in strict sandbox mode.

While this certainly provides a simple way to isolate the filesystem, it does not address the kernel attack surface at all. For that we need to filter system calls as well. That said, it may be a good idea to reintroduce the normal sandbox mode (and make it default again) which could rely only on landlock for filesystem isolation, since that would address most low hanging fruits that an adversary would target.

I am not sure if seccomp is actually a maintainable approach in the long run. The last years have shown that it requires whitelisting of platform specific syscalls and they change without notice depending on the dependencies.

I see what you mean and frankly I did not expect this feature to require this many adjustments when I first implemented it. Though to me it seems manageable to address upcoming syscall changes introduced by dependencies by addressing new kernel syscalls with the ERRNO_RULE like this since those are usually the syscalls that are used by newer version of dependent libraries and they always have fallbacks for older kernels included.

It would potentially make more sense to provide flatpak images (or comparable) for some sandboxing without breaking features and a separate zathura-seccomp binary for people that want it.

A separate binary would address issues such as the default gtk dbus socket, so maybe this is a more reasonable approach.
As for general sandboxing, there are multiple options available already. Either through mandatory access control like apparmor (here) or by using the same isolation technology as flatpak like this.

In order to make a complete sandbox that does not block any features, we would need to split the zathura process and have one sandboxed renderer process that is managed through IPC by a primary zathura process (like openssh and modern browsers do it). I have been looking into this for a while, but it does require quite a bit of work since splitting the memory management is not easy and requires changes to the plugin API. The most reasonable approach to this would be something like https://github.com/google/sandboxed-api which could be implemented in libraries like poppler and mupdf. That would also minimized the required changes to zathura and benefit more project that use those libs.

The likely hood of that being merged is close to 0.

Can you please elaborate on this? Do you mean you don't see a feature like sandboxed-api to make it into projects like poppler or are you opposed to having this implemented as part of zathura?
As I understand it, with sandboxed-api there would be only small changes required to zathura plugins that want to use that api.
Not to mention it would address the maintability of seccomp since the filter would be applied to the renderer process that should only require very limited and static syscalls.

edit:
Actually, using sandboxed-api would mean that zathura would not need to manage seccomp filter lists at all since that would be done in the library projects where proper testing would address any new syscalls. Therefore, assuming any of the libraries implement this, it would make the use of a separate sandboxed renderer process in zathura very easy and require no maintenance at all.

@valoq
Copy link
Contributor Author

valoq commented Jun 5, 2024

Github still shows "Changes Requested" but I believe all issues were addressed.
@sebastinas if there is anything missing please let me know.

@sebastinas
Copy link
Member

Haven't had the the time to test the changes yet.

@sebastinas sebastinas merged commit 76f3ad5 into pwmt:develop Jun 8, 2024
5 checks passed
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