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

Move socket into separate directory for sandbox friendliness #8018

Closed
WhyNotHugo opened this issue May 6, 2022 · 16 comments · Fixed by #8030
Closed

Move socket into separate directory for sandbox friendliness #8018

WhyNotHugo opened this issue May 6, 2022 · 16 comments · Fixed by #8030

Comments

@WhyNotHugo
Copy link
Contributor

I'm trying to set up the browser plugin to talk to a locally running KeePassXC instance. I run the browser in a local sandbox so that it doesn't have unrestricted access to the entire system using Firejail (though the topic described here probably affects any sandboxing mechanism).

From what I understand, communication happens via a socket named org.keepassxc.KeePassXC.BrowserServer placed directly in $XDG_RUNTIME_DIR.

If I configure the browser's sandbox to have access to this file, it will only work if the file exists before the browser is started. If the file does not exist when the browser is started, there is nothing to mount inside its filesystem namespace. Exposing individual files to sandboxes is tricky, and especially sockets. Usually, sockets are placed inside a dedicated directory, since exposing that directory to the sandbox is less problematic.

A common workaround seems to have been to grant the browser access to the entirety of $XDG_RUNTIME_DIR, but this exposes a huge attack surface to the browser, granting it access to lot of very sensitive sockets.

Expected Behavior

I can configure a sandboxed browser to talk to KeePassXC.

Current Behavior

There's no obvious way to connect the browser plugin to keepass while keeping the browser sandboxed.

Possible Solution

Move the socket into $XDG_RUNTIME_DIR/org.keepassxc.KeePassXC.BrowserServer/sock. This means that the $XDG_RUNTIME_DIR/org.keepassxc.KeePassXC.BrowserServer/ directory can be exposed to browser sandboxes, and the plugin would be able to talk to keepass regardless of the order in which applications were started.

This directory can be created by either keepass-proxy itself, or by the sandbox running the browser (e.g.: Firejail, Flatpak); it doesn't matter which. That's what makes the change work; whichever of both processes run first can create the directory.

Steps to Reproduce (for bugs)

Run Firefox with Firejail and try to set up the extension.

Debug info

KeePassXC 2.71.
KeePassXC-Browser 1.7.12
Operating system: Linux
Browser: Firefox (but probably affects all browsers)

@WhyNotHugo
Copy link
Contributor Author

This is obviously a breaking change, so it's probably ideal for the plugin to fall-back to searching in the current location too, for backwards compatibility.

@varjolintu varjolintu transferred this issue from keepassxreboot/keepassxc-browser May 6, 2022
@varjolintu
Copy link
Member

Moved this to the KeePassXC side.

@droidmonkey
Copy link
Member

This was requested before, it might be a duplicate

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented May 6, 2022

Oh, i didn't check the issues here (only for the extension itself).

I don't see any issues mentioning this issue / change specifically.

There's a bunch of issues on the firejail issue tracker, but they generally converge on "loosen the sandbox and allow access to all of $XDG_RUNTIME_DIR", which really compromise a lot on security. Moving the socket into a dedicated directory allows keeping the sandbox nice and tight.

@droidmonkey
Copy link
Member

It's fine this is a well presented request so if I find the other I will close that one. Either way, I do like the idea of isolating the socket more. We could create our own symlink to maintain backwards compatibility.

@varjolintu
Copy link
Member

Also the socket location needs to be changed for the Safari extension because macOS sandbox requires it. So this feature would be very welcome.

@WhyNotHugo
Copy link
Contributor Author

Also the socket location needs to be changed for the Safari extension because macOS sandbox requires it. So this feature would be very welcome.

What are the requirements on macOS? Does it need a specific location, or just a dedicated directory?

We could create our own symlink to maintain backwards compatibility.

Sure; given that the old path is currently just used by unsandboxed applications, following the link should not be an issue.

@varjolintu
Copy link
Member

What are the requirements on macOS? Does it need a specific location, or just a dedicated directory?

It needs to placed inside a path that is defined in a Group Policy. It should remain the same because it exists under the current user's home.

@rusty-snake
Copy link

This was requested before

Here: #6741 (reply in thread)

@rusty-snake
Copy link

A common workaround seems to have been to grant the browser access to the entirety of $XDG_RUNTIME_DIR, but this exposes a huge attack surface to the browser, granting it access to lot of very sensitive sockets.

Secure workaround: #6741 (reply in thread)

@WhyNotHugo
Copy link
Contributor Author

Ah, great find! I will give it a shot, but sounds like it should work.

The workaround mostly configures everything to use the socket in a different location, so I think it kinda validates that this change to the default would be a sensible one.

@WhyNotHugo
Copy link
Contributor Author

See #8030

@WhyNotHugo
Copy link
Contributor Author

@varjolintu Where would the socket need to be moved for macOS?

@varjolintu
Copy link
Member

@WhyNotHugo The path needed for Safari support is ~/Library/Group Containers/KeePassXC/org.keepassxc.KeePassXC.BrowserServer.

droidmonkey pushed a commit that referenced this issue May 28, 2022
This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped.

The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`,
since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions.

Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same.

If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged.

Closes: #8018
References: #6741
pull bot pushed a commit to soitun/keepassxc that referenced this issue May 29, 2022
This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped.

The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`,
since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions.

Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same.

If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged.

Closes: keepassxreboot#8018
References: https://github.com/keepassxreboot/keepassxc/discussions/6741
pull bot pushed a commit to tigerwill90/keepassxc that referenced this issue May 30, 2022
This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped.

The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`,
since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions.

Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same.

If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged.

Closes: keepassxreboot#8018
References: https://github.com/keepassxreboot/keepassxc/discussions/6741
@WhyNotHugo
Copy link
Contributor Author

@varjolintu Do you want to open a separate ticket for the macOS/Safari change?

@varjolintu
Copy link
Member

@WhyNotHugo Not yet. All Safari-related changes will eventually go to the same PR. And it's still WIP.

WhyNotHugo pushed a commit to WhyNotHugo/keepassxc-proxy-rust that referenced this issue Jun 2, 2022
Rather than try a single path, try all supported paths, in the preferred
order. This avoids breaking compatibility with previous KeePassXC
versions.

See: keepassxreboot/keepassxc#8018
Fixes: varjolintu#8
Supercedes: varjolintu#7
droidmonkey pushed a commit that referenced this issue Jun 27, 2022
This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped.

The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`,
since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions.

Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same.

If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged.

Closes: #8018
References: #6741
t-h-e pushed a commit to t-h-e/keepassxc that referenced this issue Sep 8, 2022
This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped.

The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`,
since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions.

Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same.

If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged.

Closes: keepassxreboot#8018
References: keepassxreboot#6741
droidmonkey pushed a commit that referenced this issue Sep 22, 2022
This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped.

The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`,
since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions.

Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same.

If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged.

Closes: #8018
References: #6741
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants