flathub / com.corsixth.corsixth

https://flathub.org/apps/details/com.corsixth.corsixth
0 stars 2 forks source link

Improve security of the sandbox settings #1

Open ssokolow opened 3 years ago

ssokolow commented 3 years ago

This line really isn't ideal:

  # Filesystem access - for game files
  --filesystem=home:ro

A Flatpak shouldn't be granting access to the user's entire home directory (even just read-only access) just to read application-specific data files. That's more intended for situations like "This application uses a custom Open/Save dialog that can't be reasonably replaced with the XDG Portal APIs."

If you're looking to load game files, the proper solution would be to ask users of the Flatpak to do one of the following:

  1. Put HOSP in a predefined location under ~/.var/app/com.corsixth.corsixth/
  2. Run flatpak override com.corsixth.corsixth --filesystem=/path/to/HOSP:ro to grant access to just the folder needed. (Or use com.github.tchx84.Flatseal to do it in a more GUI-oriented way.)
rmnvgr commented 3 years ago

I would also love to drop the home directory access permission, however Flatpaks usage have to be transparent to the user. Requiring them to put their data in a specific directory or using an external tool, without telling them to do so, goes against that.

The proper solution would rather be for upstream to use a portal for game files selection; I would advise you to contribute or open an issue on their repository.

ssokolow commented 3 years ago

Judging by how I've seen other apps on Flathub handling filesystem sandboxing, that seems a bit arbitrary... especially for a one-time "install the game resources" thing compared to various Flatpaks where it's visible to the user every time they choose a file to interact with.

rmnvgr commented 3 years ago

Can you provide examples of flatpaked games that require external data not asking for filesystem permission? From the few that I checked (Julius, OpenMW, OpenRCT2), they all ask for that permission. For OpenRCT2 we are considering dropping it because upstream uses portals for file access.

Again, the proper solution is for programs to use portals. We can't expect users to know out of the blue that they have to put their files in a specific directory when the same non-flatpaked program doesn't require it.

kparal commented 3 years ago

The set of users who can place the game data into some pre-defined location without any prompt (because they can RTFM) is probably equivalent to the set of users who can use flatpak override to strengthen the security of a flatpak app if they wish to do so :-) On the other hand, the set of users who can point to the game data location through a file open dialog is definitely much larger. So, for public consumption, I don't think the approach suggested by OP is viable. Even if you display an info box in the game telling people where to place the files exactly, it would still cut off some people lacking certain skills (e.g. navigating into ~/.var, which is a hidden directory).

However, because this use case might be pretty common for open-source reimplementations of popular games (and we have lots of them, many potentially flatpak-able), asking each upstream to implement a portal support might not be the right answer. Perhaps a better solution would be to implement a very minimal tool which could be used across most of these games, and which would simply display some basic configurable instructions and then display a file open portal. The file open action would then grant the flatpak permanent access (just) to this directory (is that how it works?), or it could possibly copy the whole contents into ~/.var/app/appid/. This tool would be a shared module that you can simply add in the manifest and provide the right config for it, and it would work universally for any game/engine with this use case. Unicorns and rainbows everywhere, somebody just needs to create it :-)

Btw, thanks a lot @rmnvgr for bringing CorsixTH onto Flathub. I just decided to play it, when I found this very fresh Flathub entry. Much appreciated.

rmnvgr commented 3 years ago

Perhaps a better solution would be to implement a very minimal tool which could be used across most of these games, and which would simply display some basic configurable instructions and then display a file open portal.

OpenRCT2 spawns zenity (included in the Freedesktop runtime) to display a file chooser, it uses a GtkFileChooserNative which itself uses a portal for file selection. I haven't looked at the details but the permissions for the selected files are remembered between restarts.

Btw, thanks a lot @rmnvgr for bringing CorsixTH onto Flathub. I just decided to play it, when I found this very fresh Flathub entry. Much appreciated.

You're welcome :)

ssokolow commented 3 years ago

I'll need a little time before I can do a more thorough survey, but here are the from-memory inputs to what I wrote last night:

  1. Non-games on the list of top Flatpaks, like Discord, which don't use the Open/Save portal yet (eg. Electron just merged use of GtkFileChooserNative for v14 and there are no plans to backport it), often lock users into directories like xdg-documents or xdg-downloads, despite that meaning that users are forced to either reach for Flatseal or do a "file manager, then file chooser" dance whenever they want to send a file.

    Yes, that's an example of a high-priority Internet-connected attack target and, thus, an argument can be made that there's a bigger threat to be mitigated, but it's still a data point against "Flatpaks must be transparent to the user".

    In fact, for Discord specifically, there was recently a whole song-and-dance with the maintainer trying to resolve flathub/com.discordapp.Discord#99 by switching to a blacklist-based approach with a little referendum on what needed to be blacklisted and getting talked out of it in favour of waiting for the Electron update.

  2. Off the top of my head, one example of a game I can think of (because I just set it up yesterday) is StepMania, which grants no external filesystem permissions and requires you to copy your Dance Dance Revolution song data into ~/.var/app/com.stepmania.StepMania/.stepmania-5.1

  3. If file pickers for finding the assets are common now, that's a new thing. I remember the StepMania approach being the norm less than five years ago when I set up a huge pile of game engines without Flatpak.

    Heck, some of them, like GemRB (a reimplementation of the Infinity engine for games like Baldur's Gate) are so unhelpful about it that the setup instructions right now say GamePath in GemRB.cfg must always be edited, rather than giving it some default like ~/.local/share/gemrb/gamedata and giving users the option to edit it.

    As an example of a game not Flatpak'd which tries to be more helpful, Arx Libertatis looks in multiple locations, with ~/.local/share/arx being the most relevant to a theoretical Flatpak-ing, and comes with an arx-install-data /path/to/arx/fatalis/install/directory script to handle potential tripping points like normalizing filename case.

    (And that makes sense, given that SDL doesn't give you a file picker and most developers want a consistent cross-platform experience without having to source a file picker only used for one-time setup.)

While I don't intend it to be an argument for or against my point, I should note that I suspect there's a selective bias at play, where things requiring external resources are less likely to get flatpak'd to begin with.

OpenRCT2 spawns zenity (included in the Freedesktop runtime) to display a file chooser, it uses a GtkFileChooserNative which itself uses a portal for file selection. I haven't looked at the details but the permissions for the selected files are remembered between restarts.

I was about to propose that.

With Zenity, the source directory would be mounted into the sandbox for the session, and then the files could be copied into a persistent location.

Something like this:

zenity --info --width=600 --title="Could not find assets" --text="Corsix-TH requires game data from a retail copy of Theme Hospital, but could not find it.\n\nWhen you click OK, you will be asked to select the HOSP folder from your Theme Hospital CD or GOG.com install so it can be copied into Corsix-TH."
src_path="$(zenity --file-selection --directory --file-filter=HOSP)"

if [ -z "$src_path" ]; then
    exit # Cancelled
fi
cp -r "$src_path" ~/.var/app/com.corsixth.corsixth/WHEREVER

(If there's a terminal like xterm in the platform image, something like xterm -e cp -vr ... could be used to give a primitive form of status display. If rsync is in the platform image, that could be used in place of cp with the --progress option to get active status updates for the copying of large files.)

NOTE: If you do use a wrapper, please give it a name like corsix or corsix-tx or at least corsix-th.sh.

I use a homegrown update_flatpak_cli.sh that creates launcher wrappers from flatpak info -m "$X" | grep command= | cut -d= -f2 and it's annoying that I have ~/.local/bin/scummvm_wrapper and ~/.local/bin/flatpak/com.github.tchx84.Flatseal exposed to my gmrun rather than scummvm and flatseal.

(Unless I decide to rewrite it in Python and add some smarts to strip _wrapper and check for a match like ^(com|org|net)\..*\.([^.]+) followed by .group(2).lower()... which I'll probably do.)

That latter one is especially annoying because it's actively destroying my ability to Tab-complete.

ssokolow commented 3 years ago

Funny enough, if it weren't rude to have the game depend on the KDE platform for something so trivial, I think kioclient5 --interactive copy <source path> <absolute destination path> would get you a fully graphical progress indication.

(kdialog is KDE's equivalent to Zenity, while kioclient5 is an equivalent tool for using KDE's KIO counterpart/precursor to GIO/GVFS and the associated "copy/move/delete progress in desktop notifications" system from shell scripts.)

kparal commented 3 years ago

I'm not trying to argue with you, @ssokolow, but just a few notes:

  1. Non-games on the list of top Flatpaks, like Discord

I feel there is a difference between "the tool runs fine, but I can't send files to friends outside of ~/Documents" and "the game doesn't start at all".

  1. StepMania, which grants no external filesystem permissions and requires you to copy your Dance Dance Revolution song data

The difference is that StepMania runs just fine without the mentioned songs, the UI works and there is some content present out-of-the-box (I just tried very quickly). That's like running OpenTTD which uses its own sprites by default, but you can replace them with the original copyrighted ones, if you want. Again, different from "the game doesn't start at all" (the case of CorsixTH, if there is no home access permission to use the native file picker, nor there is portal integration).

  1. If file pickers for finding the assets are common now, that's a new thing.

I don't think they are that common. But this one does have it :-) Game reimplementations projects generally probably don't want to include the whole GTK or similar just to have a file picker. At the same time, people who install the project from source are definitely capable enough to handle this. But when the project gets packaged, my expectation is that it provides some reasonable integration into the target system (in this case, that could be a file picker, along with some config editing, etc). There is a distinct problem with Flatpak because it overrides some of the common paths (redirecting them into ~/.var), and making the rest of the filesystem inaccessible when sandboxed). That means even the project's set-up instructions are invalid. Which means the Flatpaked project must provide custom correct instructions, in a place where they can't be missed (after starting the app), or preferably some better integration.

With Zenity, the source directory would be mounted into the sandbox for the session, and then the files could be copied into a persistent location.

I agree that this is a viable approach. However, the corner cases need to be thought out:

I'm sure I forgot many more "corner cases" (which might not be so corner-y after all).

When I think about all of this, I'm not sure that the "copy files" approach is that great. Granting a permanent permission to a folder would work better (no copying, no leftover files), if the zenity approach can do that, but it still does have the same problem of "how do I change the folder when needed"? A possible approach is to a) detect the game exit code, if it has a specific exit code for "game data not found", and b) expose a new desktop file for force-changing the selected directory, so e.g. "CorsixTH" and "CorsixTH - set game data dir". Not completely pretty, but functional, I guess.

What are your thoughts about this?

ssokolow commented 3 years ago

I feel there is a difference between "the tool runs fine, but I can't send files to friends outside of ~/Documents" and "the game doesn't start at all".

But that's inherent in any engine which doesn't come with ready-to-use data files. Demo.zip is only 12MiB, so maybe some kind of system should be provided where the demo resources are bundled and users are offered the option to switch to full-version resources?

At the same time, people who install the project from source are definitely capable enough to handle this. But when the project gets packaged, my expectation is that it provides some reasonable integration into the target system (in this case, that could be a file picker, along with some config editing, etc).

Packaged on Flathub, maybe. All of the things that gave me an expectation of having to copy files into place before they'd do anything when you click the icon are things I installed from the Ubuntu and OpenPandora repositories.

What happens if the user selects the wrong directory? Will you be able to detect if it is the right dir or not? What if the user misclicks and selects his home directory? Will you start copying his whole home into ~/.var (that might fail at some point because of recursion, but he can select e.g. the whole ~/Downloads)?

I'd probably do a ScummVM-style thing and check that at least a few distinctive paths are present at the expected locations relative to the selected root folder.

If the game is not satisfied with those files, how do you detect it and make the dialog pop up again?

If you want to be perfect about it, you either bake portal support into the game or have some kind of --check-resources option which allows a wrapper to detect failure without duplicating things.

Otherwise, Theme Hospital is long past its support window, so you can approach it from two directions:

  1. Do as ScummVM does and make sure that's impossible by embedding a list of all potentially required paths and their SHA hashes, then eat the time needed to read the files a second time to fully validate them before copying anything.

    (If you want to validate them every time, store the mtimes of all the files and do what rsync does to skip having to read ostensibly unchanged files. At some point, you start shooting yourself in the foot if you try to protect the user from shooting themself in the foot without full cooperation from the application you're wrapping. I know ScummVM's checks can be outwitted and will result in confusing breakages and those are built in.)

  2. If Corsix-TH exits with a nonzero exit code, have the wrapper ask the user if they want the picker to appear again on next start.

If the user uninstalls the Flatpak, what will happen to the game files? For certain games, those can be many GBs. As a user, I'd expect it to be removed automatically. But how do we achieve that? Or how do we at least inform the user about the leftover files?

I'm not sure there's an ideal solution for this unless Flatpak has some kind of facility for displaying a "do you also want to delete your userdata for this application?" prompt.

As-is, unless you solve the "Gigabytes of mods were downloaded using the game's internal mod browser" problem, you can't properly solve the case you've described either.

XDG directory structuring and Flatpak aren't really designed for a distinction between "take good care of the irreplaceable userdata in my BasKet Note Pads (a OneNote-like tool)" and "this userdata is gigabytes of mods that shouldn't be flushed as ~/.cache and are too big to be appropriate for ~/.config"

When I think about all of this, I'm not sure that the "copy files" approach is that great. Granting a permanent permission to a folder would work better (no copying, no leftover files), if the zenity approach can do that, but it still does have the same problem of "how do I change the folder when needed"?

The fact that Flatseal is installed as a Flatpak proves it's possible... but I'd consider that to be a terrible solution since it disguises how unconstrained an application is to the eye of the less skilled Flatseal user.

...plus, I'm not familiar with Flathub's review process, but I could imagine you having trouble justifying giving a game xdg-data/flatpak/overrides:create permissions. It seems like the wrong side of the security vs. convience trade-off to set a precedent for.

If you must keep things outside of ~/.var, maybe something like ~/game_data/com.corsixth.corsixth:create where novice users can see it and users like me have less $HOME clutter while we look up how to move it inside ~/.var.

(My first use for sandboxing games was to use Firejail to overrule smartass games like The Escapists which use getpwuid/getpwnam to ignore my "fake $HOME" wrappers and put un-hidden folders in my actual $HOME anyway, but I'm planning a launcher which sandboxes every game by default to approximate Flatpak's sandboxing defaults on stuff I installed by pointing unzip or innoextract at GOG.com's MojoSetup or InnoSetup installers.)

rmnvgr commented 3 years ago

Please note that OpenRCT2 does not copy the data into its directories, zenity grants it a permanent access through the portal. This is the current flow for selecting the data:

  1. Show a dialog informing the user that they need to choose the game data
  2. Show a file chooser
  3. If the data are found in the chosen directory, launch the game. If the data are not found, show a dialog informing the user that the necessary files in that directory are not found, repeat 1-3.

A wrapper is not a solution, as there are other directories that can be set in-game, each opening a file chooser: CorsixTH folders selection

I really believe this is a discussion that should happen upstream, as the portal support should be implemented in the game.

ssokolow commented 3 years ago

Please note that OpenRCT2 does not copy the data into its directories, zenity grants it a permanent access through the portal.

Huh. I missed that it was possible to do that. Now you've got me worried that I'm going to have to come up with some kind of incrontab line to pitch a fit if a program sneaks one of those by me.

How is it accomplished and, if it doesn't require some specialized chooser with a big "this permissions grant will persist until explicitly cancelled" banner, how do I blacklist persistent grants Flatpak-wide?

(Heck, is it even intended to be possible? "Persistent grants could be used to exploit naive users" was the rationale that was given to refuse my request for a "Can monitor active application titles" portal so time-tracking apps could be cross-desktop compatible with Wayland.)

I really believe this is a discussion that should happen upstream, as the portal support should be implemented in the game.

Definitely.

rmnvgr commented 3 years ago

How is it accomplished and, if it doesn't require some specialized chooser with a big "this permissions grant will persist until explicitly cancelled" banner, how do I blacklist persistent grants Flatpak-wide?

Looking at the File chooser portal documentation, chosen files are added to the Documents portal.

I guess you can use the various DBus calls to list the documents of a specific app and remove them when it is closed. Or open a request for an "Allow access until closed" option in the File chooser portal, like the "Read-only" one in the GtkFileChooserNative.


I'll open an issue upstream to see if they are interested in supporting portals in CorsixTH.

kparal commented 3 years ago

I'll open an issue upstream to see if they are interested in supporting portals in CorsixTH.

I assume it might be enough for CorsixTH to detect if running in a Flatpak environment (some envvar should be available for that), and then call zenity instead of the integrated file browser (and you'll just make sure that zenity is installed in the Flatpak). That might be easier for them than a native Flatpak Portal integration (but I have never tried it, so just speculating and throwing ideas here).