Open sebastinas opened 1 year ago
sebastinas
commented
1 year ago With the changes proposed here, in !70, and in pwmt/girara!8, support for the sandbox is leaking to other components. Could we alternatively have a separate binary that initializes the seccomp sandbox and then launches the zathura binary?
sebastinas
commented
1 year ago On GitLab by @valoq on Mar 12, 2023, 19:06
Yes, we could use a separate script to initialize a sandbox before starting zathura. This would prevent the dbus connection reliably as well. The actual seccomp filter would still need to be initialized within the zathura process to allow a small number of syscalls. Initiating seccomp before the actual zathura process would mean that all the dangerous syscalls that are needed for zathura initialization (socket, connect etc.) also need to be whitelisted, while setting the seccomp restriction during zathura execution right before parsing a target file allows for a much stricter filter.
A simple solution would be a script that only changes the dbus socket address like in !70 and then starts the zathura binary, though it would still need to read zathurarc to check for the sandbox option. Instead of reimplementing what girara already does, I would prefer adding the change in girara, which could theoretically be used by other features as well.
In regards to the sandbox feature in general, I have been thinking about two different options to move forward:
A) With the additional check to be introduced in !8 the strict sandbox will provide a strict runtime without any remaining ipc escape path and I am rather confident that it would be a reliable protection even against targeted malware. The normal sandbox is nearly useless right now, which is why I have been working on a patch to introduce landlock in order to make the normal mode a mostly read only runtime that still has most features, like history and bookmarks, while providing a degree of opportunistic security, though sandbox escape would still be trivial. The default sandbox mode would be changed to "none" in order to avoid any issue for users that have not set the sandbox feature explicitly.
B) Ideally, a sandbox would be build using a separate rendering process, like modern browsers use. Instead of parsing the file directly, a separate process would be spawned and share a socket connection for ipc (or an alternative ipc method). The separate process would then initiate seccomp with an even more strict filter list then we have now and return the interpreted data back to the main zathura process. This would be the ideal sandbox architecture that would allow the user to use all zathura features like normal, while the dangerous part is locked away. Zathuras plugin architecture may actually be a good target for this.
Method B is something I would like to explore and it may end up a research project eventually, but it would need some time and there might be other issues that are found along the way. The proposed merge requests would fix the immediate issue and get the feature to a more useful state.
sebastinas
commented
1 year ago On GitLab by @valoq on Mar 19, 2023, 19:40
One more alternative solution might be to call fork after reading the sandbox configuration and start a new zathura process if the setting is set to strict. Since the configuration is read before a gui appears the user would not notice this either.
Personally I would still argue that the added girara option feels like the cleanest solution but if you prefer any of the other options instead, let me know and I will rework the merge requests.
sebastinas
commented
1 year ago On GitLab by @valoq on May 18, 2023, 15:13
After revisiting this patch with the intention to avoid the sandbox feature leaking to other components, I noticed that while this is the case in !70 and pwmt/girara!8, this MR for a command line option does not really change anything outside the added configuration.
The only other component that is changed is this !67/diffs#5923ecf234ac14d9836eace4bc9c4867ca2878de_110_111 which only changes the order in which configuration and command line options are handled. This is not actually related to the sandbox itself, but this is required for setting any command line option that is also present as configuration. With this change to the init function, command line options in general can be prioritized against configuration options, which could be useful for other options as well.
Its also a fairly small change and loading the defaults and files before zathura_init should not cause any issues either.
sebastinas
commented
4 months ago On GitLab by @valoq on Mar 10, 2024, 18:31
added 131 commits
pwmt:developsebastinas
commented
4 months ago On GitLab by @valoq on Mar 10, 2024, 18:42
marked this merge request as draft
On GitLab by @valoq on Dec 22, 2022, 19:20
Merges master -> develop
This commit adds an alternative command line option to set the sandbox mode as an alternative to the configuration option.
One example use cases would be to have the sandbox set to strict in the configuration for general use and use the command line option --sandbox=normal (or none) when using zathura in something like vimtex where we want all features and the file is safe anyway.
The command line option is prioritized if there is a different setting in zathurarc