flathub / org.keepassxc.KeePassXC

https://flathub.org/apps/details/org.keepassxc.KeePassXC
26 stars 15 forks source link

Safe and secure default permissions #101

Open daniel-j-h opened 1 year ago

daniel-j-h commented 1 year ago

Hey folks, awesome work here packaging KeePassXC as a flatpak :raised_hands:

I've been looking at the permissions seeing we are handing out a good amount of permissions by default e.g.

  1. Network access only to download favicons
  2. Access to the full host filesystem (instead of e.g. just the home dir)
  3. Access to all devices like webcams and all that, in case of Yubikey support

Because KeePassXC is a password manager and my tinfoil hat is comfy, I'm proposing the following:

  1. By default, let's remove network permissions, the upside of having fancy favicons does not outweigh the downside of a password manager having network access just for this one convenience feature
  2. By default, let's remove access to the full file system, and instead only allow access to the user's home dir; that sounds like a good compromise unless I'm missing why KeePassXC would need access to the full file system
  3. By default, let's remove access to all devices; folks with Yubikeys are tech-savy enough to enable it

And then have a few words in the docs how users can enable these features e.g. with flatseal or the equivalent commands

flatseal-keepassxc

This way the KeePassXC flatpak will be more safe and secure by default, and users can still have their functionality by toggling a switch if they want to. Because at the moment 99% of users will run KeePassXC with the default permissions even though they probably don't even have a Yubikey, and don't care about favicons downloaded via the network.

What do you think? (cc @crepererum h/t for the flatseal shout)

o-kotb commented 1 year ago

I believe --filesystem=host is used for ssh-agent integration.

johannesrld commented 1 year ago

I think it's probably best to wait until proper portals are added before removing said features, there are things that should be disabled eventually like X11/--share=ipc

droidmonkey commented 1 year ago

I won't be removing any permissions. Those that want more protection can use flatseal to REMOVE them. We handle an ungodly amount of dumb issues and complaints with snaps. Your recommendations will cause us to have those same things happen with flatpak.

Contrary to what us security minded folks think, 99% of folks just want their password manager to work for them. Arbitrary barriers to doing simple tasks like downloading a favicon will literally cause people to use LastPass. I wish I was kidding, but that's reality.

StayPirate commented 1 year ago

@daniel-j-h highlighted an interesting point. Here are the permissions I'm currently providing to KPXC:

[Context]
shared=network;ipc;
sockets=x11;wayland;ssh-auth;
devices=all;
filesystems=~/Nextcloud/Backups/Keepass;xdg-config/kdeglobals:ro;xdg-config/keepassxc;xdg-run/gvfs;

[Session Bus Policy]
org.freedesktop.secrets=own
org.freedesktop.Notifications=talk
com.canonical.Unity.Session=talk
org.kde.StatusNotifierWatcher=talk
org.kde.KGlobalSettings=talk
org.freedesktop.ScreenSaver=talk
com.canonical.AppMenu.Registrar=talk
org.gnome.SessionManager.Presence=talk
org.gnome.ScreenSaver=talk
org.kde.kconfig.notify=talk
org.gnome.SessionManager=talk

[System Bus Policy]
org.freedesktop.login1=talk

I'm using them since months without any drawback. The override file can be found in my dotfiles.

Arbitrary barriers to doing simple tasks like downloading a favicon will literally cause people to use LastPass. I wish I was kidding, but that's reality.

That sounds very pity, but I agree with you to some extent. For instance, filesystems=host could be avoided, contrary to what was mentioned by @o-kotb:

I believe --filesystem=host is used for ssh-agent integration.

sockets=ssh-auth; can be used instead.

Maybe applying for strict permissions by default would lead to unsatisfied users (which might end up moving to LastPass 🤦🏼‍♂️ ), but limiting filesystem to ~ would already be a good win from the security perspective.

droidmonkey commented 1 year ago

Can you tell me what the threat vector is for an app to have access to the file system? You aren't running it as root, are you?

The downsides to removing file system access are everything we hate about snaps. Inability to access mounted drives, network shares, files in awkward locations the user chose to put them in.

johannesrld commented 1 year ago

For KeePassXC? Nothing really (unless there is some kind of critical bug with favicon downloading, which I doubt there is)

edit 6hrs later: I do agree that keepass (and really any) application should try to reduce its permissions as much as possible.

johannesrld commented 1 year ago

Looking at flatpak documentation, --device=all might be able to be replaced with --socket=pcsc if all it's being used for is security keys (see here at flatpak documentation and Debian's article on smart cards)

--socket=fallback-x11 might be able to replace --socket=x11, although --socket=ipc will still need to exist, however keepassxc wayland support still isnt complete

droidmonkey commented 1 year ago

PCSC is a specific interface. Yubikeys are not PCSC when plugged into USB. They are when using NFC readers though.

johannesrld commented 1 year ago

shame

FOSSProponent9436 commented 1 year ago

I won't be removing any permissions. Those that want more protection can use flatseal to REMOVE them. We handle an ungodly amount of dumb issues and complaints with snaps. Your recommendations will cause us to have those same things happen with flatpak.

Contrary to what us security minded folks think, 99% of folks just want their password manager to work for them. Arbitrary barriers to doing simple tasks like downloading a favicon will literally cause people to use LastPass. I wish I was kidding, but that's reality.

Would it at least be possible to add documentation in a README or something about possible permissions that may not be necessary and you could revoke (maybe listing minor issues caused)? Although this issue page already sort of serves as that documentation, I would argue that people might use this flatpak thinking it is more secure/sandboxed than alternatives like the appimage when really out of the box it isn't.

o-kotb commented 1 year ago

I would argue that people might use the flatpak thinking it is more secure/sandboxed than alternatives when really out of the box it isn't.

Maybe in the early days of Flatpak. But it has become more mainstream since then, and nowadays it's seen as the simple and easy installation method that just works on any distro.

-- Omar Kotb

Hasshu commented 9 months ago

--socket=fallback-x11 might be able to replace --socket=x11, although --socket=ipc will still need to exist, however keepassxc wayland support still isnt complete

Incidentally, are there any blockers for replacing --socket=x11 with --socket=fallback-x11? If I'm not mistaken, the latter should make KeePassXC try to use Wayland by default (and no longer be called unsafe by Flathub and GNOME Software for using "a legacy windowing system").

droidmonkey commented 9 months ago

We don't necessarily want to use Wayland by default as it breaks Auto-Type and clipboard clearing.

Hasshu commented 9 months ago

Does it mean that KeePassXC running under a Wayland session via XWayland retains the said functionality somehow? (Am not particularly knowledgeable in this area.)

droidmonkey commented 9 months ago

It does because it is actually running on an X11 server (as a client), XWayland is an interface between the X11 server and Wayland compositor. Any other app running on that x11 server will be capable of receiving auto type.

boredsquirrel commented 2 months ago

Note also that any app with home access can elevate its privileges by placing a local override file in ~/.local/share/flatpak/overrides/. This allows it to enable flatpak-spawn which can do basically anything.

Waiting for portals is a good way.

For filesystem permission: KeepassXC can use the filepicker portal for opening files. But to automatically open them in the future, it (at least currently?) needs static filesystem permissions.

This is an issue flatpak needs to solve too

droidmonkey commented 2 months ago

That elevation of privilege sounds like a security problem that flatpak needs to solve. Has that been reported to them?