userd: add an OpenFile method for launching local files with xdg-open #4766

jhenstridge · 2018-02-28T12:21:24Z

This is the snapd side of work to make the xdg-open proxy handle local files. Similar to the xdg-settings D-Bus interface, it prompts the user before opening the file.

Similar to xdg-desktop-portal's OpenFile interface, this takes a file descriptor as proof of access. While the x-d-p implementation expects an O_PATH file descriptor, I'm explicitly checking that it isn't O_PATH: I believe that logic doesn't make sense because it is possible to create O_PATH file descriptors for files a process can't access (flatpak/xdg-desktop-portal#167). I also check to make sure the it is a regular file or directory.

The file name to open is determined by looking in /proc/self/fd/$fd, and checking that it refers to the same file. We never try to perform reads or writes on the file descriptor because the confined app shares control of it.

This work will need some matching changes to the xdg-open proxy in the core snap, but this side should be landable on its own. This Python script can be used for ad-hoc testing: https://paste.ubuntu.com/p/r8KGvHqrVF/

zyga

+1 though I left some comments about wording below.

zyga · 2018-02-28T12:41:37Z

userd/launcher.go

+	// File descriptors opened with O_PATH do not imply access to
+	// the file in question.
+	if flags&sys.O_PATH != 0 {
+		return "", fmt.Errorf("bad file descriptor")


The error message could be confusing, how about cannot use file descriptors opened using O_PATH

zyga · 2018-02-28T12:42:48Z

userd/launcher.go

+
+	// Sanity check to ensure we've got the right file
+	if fdStat.Dev != fileStat.Dev || fdStat.Ino != fileStat.Ino {
+		return "", fmt.Errorf("could not determine file name")


How about cannot determine file name?

I'd go farther: cannot determine file name (maj:min and/or inode do not match)

zyga · 2018-02-28T12:43:23Z

userd/launcher.go

+
+	fileType := fileStat.Mode & syscall.S_IFMT
+	if fileType != syscall.S_IFREG && fileType != syscall.S_IFDIR {
+		return "", fmt.Errorf("can only open regular files and directories")


Here I'd say cannot open anything other than regular files or directories

A comment as to why only file or directories would be nice. If you take @zyga's recommendation for the comment before fdToFilename(), that would be sufficient.

zyga · 2018-02-28T12:47:41Z

userd/launcher.go

@@ -93,3 +102,78 @@ func (s *Launcher) OpenURL(addr string) *dbus.Error {

 	return nil
 }
+
+func fdToFilename(fd int) (string, error) {


I'd document how this works. How about something like this:

fdToFilename determines the path associated with an open file descriptor. The file descriptor cannot be opened using O_PATH and must refer to a regular file or to a directory. The symlink at /proc/self/fd/<fd> is read to determine the filename. The descriptor is also fstat'ed and the resulting device number and inode number are compared to stat on the path determined earlier. The numbers must match.

zyga · 2018-02-28T12:51:13Z

userd/launcher.go

+		return dbus.MakeFailedError(err)
+	}
+	answeredYes := dialog.YesNo(
+		i18n.G("Allow open file?"),


This is nitpick territory but perhaps Allow opening file?

zyga · 2018-02-28T12:52:02Z

userd/launcher.go

+	}
+	answeredYes := dialog.YesNo(
+		i18n.G("Allow open file?"),
+		fmt.Sprintf(i18n.G("Allow snap %q to open file %q ?"), snap, filename),


Please remove the space between %q and the question mark.

mvo5

\o/

mvo5 · 2018-02-28T13:46:58Z

userd/launcher.go

@@ -40,6 +46,9 @@ const launcherIntrospectionXML = `
 	<method name='OpenURL'>
 		<arg type='s' name='url' direction='in'/>
 	</method>
+	<method name="OpenFile">
+		<arg type="h" name="fd" direction="in"/>


It would be great if we could prepare for the windowID here so that we can set the UI transient for the calling thing. But afaict only our xdg-open inside the core snap can figure out the window-id (xdg-open can get the pid of the other side of the dbus connection and then try to find that pid in the active X windows).

@mvo5 - be careful with PID checks since they are often racy due to PID wrapping, see below for some discussion.

codecov-io · 2018-02-28T20:49:48Z

Codecov Report

Merging #4766 into release/2.31 will decrease coverage by 0.03%.
The diff coverage is 53.7%.

@@               Coverage Diff                @@
##           release/2.31    #4766      +/-   ##
================================================
- Coverage         78.18%   78.14%   -0.04%     
================================================
  Files               459      459              
  Lines             32718    32774      +56     
================================================
+ Hits              25579    25610      +31     
- Misses             5016     5033      +17     
- Partials           2123     2131       +8

Impacted Files	Coverage Δ
interfaces/builtin/desktop.go	`87.5% <ø> (ø)`	⬆️
interfaces/builtin/unity7.go	`68.18% <ø> (ø)`	⬆️
userd/helpers.go	`34.88% <0%> (-9.24%)`	⬇️
userd/launcher.go	`65.62% <64.44%> (-2.8%)`	⬇️
overlord/configstate/configcore/services.go	`54.83% <0%> (+3.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e69dce...f5e9099. Read the comment docs.

jdstrand

Overall it looks really good to me. Just a couple of comment changes for your code.

I'd like to see the change made to snapFromSenderImpl fixed in this PR since those changes are small and this PR would benefit from them.

jdstrand · 2018-02-28T14:00:54Z

userd/launcher.go

+		return "", err
+	}
+	// File descriptors opened with O_PATH do not imply access to
+	// the file in question.


jdstrand · 2018-02-28T14:06:09Z

userd/launcher.go

+		return "", fmt.Errorf("bad file descriptor")
+	}
+
+	// Determine the file name associated with the passed file descriptor.


Can you update this slightly:

// Determine the file name associated with the passed file descriptor. Since // this is racy, we'll verify this next.

jdstrand · 2018-02-28T14:07:43Z

userd/launcher.go

+	}
+	if err := syscall.Stat(filename, &fileStat); err != nil {
+		return "", err
+	}


Nit-pick, reordering the Stat and the Fstat so that Stat comes first is more readable for dealing with the race to me.

jdstrand · 2018-02-28T14:25:23Z

userd/launcher.go

+
+	// Sanity check to ensure we've got the right file
+	if fdStat.Dev != fileStat.Dev || fdStat.Ino != fileStat.Ino {
+		return "", fmt.Errorf("could not determine file name")


I'd go farther: cannot determine file name (maj:min and/or inode do not match)

jdstrand · 2018-02-28T14:29:34Z

userd/launcher.go

+
+	fileType := fileStat.Mode & syscall.S_IFMT
+	if fileType != syscall.S_IFREG && fileType != syscall.S_IFDIR {
+		return "", fmt.Errorf("can only open regular files and directories")


A comment as to why only file or directories would be nice. If you take @zyga's recommendation for the comment before fdToFilename(), that would be sufficient.

jdstrand · 2018-02-28T14:45:53Z

userd/launcher.go

@@ -40,6 +46,9 @@ const launcherIntrospectionXML = `
 	<method name='OpenURL'>
 		<arg type='s' name='url' direction='in'/>
 	</method>
+	<method name="OpenFile">
+		<arg type="h" name="fd" direction="in"/>


@mvo5 - be careful with PID checks since they are often racy due to PID wrapping, see below for some discussion.

jdstrand · 2018-02-28T15:10:31Z

userd/launcher.go

+		return dbus.MakeFailedError(err)
+	}
+
+	snap, err := snapFromSender(s.conn, sender)


snapFromSender() is racy since it gets the PID from org.freedesktop.DBus.GetConnectionUnixProcessID, then looks up that PID to see what cgroup it is it. It is possible for this to happen:

process A with pid 2 opens a file, and passes it to OpenFile, calls snapFromSender which gets the pid of the connecting process

process A dies. process B starts, pid wraps and is now pid 2

snapFromSender looks up pid 2 and sees it is for process B

snapFromSender will error if the pid doesn't match any snap, but it will happily report when process B is in a snap. In the context of this PR, the snap name will be presented as process B, but process B never proved it could open the file. This means that if process A is unconfined, is allowed to open the file (eg, ~/.ssh/id_rsa) and uses this API, then process B could perform this attack to be allowed access to this file. If process A is a snap, then process B could try to grab data from process A's data dirs.

Unfortunately, I did not point out this race condition in #4073. Note that while this attack is possible, process B isn't going to be able to interfere with process A on systems with Full confinement unless process B is a classic snap (in which case it already has access to the file) or if process B has process-control. It is also unlikely that unconfined, non-snap applications will use this API. On systems without full confinement, the prompt is not a security barrier since only DAC checks and mount namespace are applied to the snap.

To handle snaps with process-control on systems with full confinement, we need to fix the race. I investigated this a bit today and think that if we adjust snapFromSenderImpl in userd/helpers.go to be:

func snapFromSenderImpl(conn *dbus.Conn, sender dbus.Sender) (string, error) { pid, err := connectionPid(conn, sender) if err != nil { return "", fmt.Errorf("cannot get connection pid: %v", err) } snap, err := snapFromPid(pid) if err != nil { return "", fmt.Errorf("cannot find snap for connection: %v", err) } // Since the above is a TOCTOU race, try to get the pid again to see if the // connection is still alive (dbus-daemon will disconnect the connection if // either side dies). If not, tear down the connection (to be sure) and error. _, err = connectionPid(conn, sender) if err != nil { conn.Close() return "", fmt.Errorf("client connection died: %v", err) } return snap, nil }

I tested this locally and found that connectionPid() would fail if the sender's pid was killed. I also simulated a successful pid race by unconditionally performing a conn.Close() and sending the error and found that the client sees "org.freedesktop.DBus.Error.NoReply: Message recipient disconnected from message bus without replying", ie, exactly what we want.

@mvo5 - you implemented #4073 so I'd like your opinion on this.

UPDATE CLARIFICATION: because xdg-open is being used to open the file or directory under the default unconfined handler, process B can't typically use this to give files to itself. It can bypass the proven access check though, and if the snap is registered as the default handler that xdg-open would use, then it would be able to grab external data.

I think a better fix would be to change the code to call GetConnectionCredentials instead of GetConnectionUnixProcessID, and look at LINUX_SECURITY_LABEL` in the returned dictionary. This avoids the PID reuse problem, since the bus unique names aren't reused.

Closing the dbus.Conn seems incorrect though: it represents connection to the bus, rather than a connection to another peer on the bus.

I considered solely using GetConnectionCredentials() instead of GetConnectionUnixProcessID() with a cgroup lookup, but we would need the cgroup lookup for cross-distro since there won't be a label on systems where AppArmor isn't enabled (that's why we have the cgroup check in the first place). Sure, we could check the confinement level and use GetConnectionCredentials if we expect to have a label, otherwise GetConnectionUnixProcessID, but I liked how calling GetConnectionUnixProcessID and detecting if the connection closed fixed both and can therefore ensure the proven check regardless.

Really though, I wanted to call something along the lines of "is connection still connected" to see if the client died, but I couldn't find in godbus a call to do that, so just reused what snapd already implemented. I also realized that the unique name isn't reused, but 'sender' isn't updated in what I outlined because process B didn't reconnect to the bus, it just reused the PID, so we couldn't check the sender's connection name (indeed, within the call to snapFromSender it wouldn't ever change). IIUC dbus-daemon is supposed to send a signal when the client disconnects, so what is probably most correct would be to detect this and terminate OpenFile if it did, but I didn't readily see how to do that in godbus. I'm certainly open to other ideas, but one more call to detect for disconnection was cheap.

As for closing dbus.Conn, I was thinking that it represented the connection between the server and the client for this unique connection name. Perhaps that is wrong, but I can say that when I tested this, the server didn't stop listening for new connections, which seemed to reaffirm my thinking. The call isn't strictly needed though since we return a DBus error instead of opening the file.

@mvo5 - this is the bit that needs to be addressed before committing (please see whole comment)

I've updated snapFromSender to perform a NameHasOwner call on the sender unique bus name after we've looked up the snap from the process ID. This will fail if the sender has disconnected from the bus, which would happen if the process A exits.

As mentioned earlier, we're not going to close the connection to the bus, because it will leave a zombie userd around disconnected from the session bus. I suspect that things appeared to work for @jdstrand because a new instance of userd was spawned by D-Bus service activation.

jdstrand · 2018-02-28T22:01:03Z

userd/launcher.go

+		fmt.Sprintf(i18n.G("Allow snap %q to open file %q ?"), snap, filename),
+		&ui.DialogOptions{
+			Timeout: 5 * 60 * time.Second,
+			Footer:  i18n.G("This dialog will close automatically after 5 minutes of inactivity."),


That's a long time! I'm not sure what would be better though.... Perhaps add a comment on why you chose 5 minutes in case we want to revisit this again?

The 5min is what we use in xdg-settings. It is a bit of an arbitrary choice, long enough to be not inconvenient (e.g. when a download takes long and the user does not look right away) but not so long that the dbus connection stays up forever (and auto-closes with awkward failures).

jdstrand · 2018-02-28T22:06:57Z

userd/launcher.go

+		return dbus.MakeFailedError(fmt.Errorf("permission denied"))
+	}
+
+	if err = exec.Command("xdg-open", filename).Run(); err != nil {


It's nice for us that exec.Command doesn't execute the shell, so don't have to worry about shell metacharacters here.

chipaca · 2018-02-28T22:29:40Z

userd/launcher_test.go

+	c.Assert(err, IsNil)
+	defer file.Close()
+
+	dupFd, err := syscall.Dup(int(file.Fd()))


don't forget to close these fds

OpenFile closes the file descriptor passed to it. It seems godbus simply transfers ownership of the file descriptor to the method implementation, so I'm providing OpenFile with its own fd in the tests.

jhenstridge · 2018-03-01T12:12:34Z

The core snap side of this can be found here: snapcore/core#82 -- at the moment it just passes an empty string for the parent window argument @mvo5 requested.

…=jlorenzo This fix does several things: * Removes the mime cache generated by the desktop-gtk3 remote part * Installs a stub shared-mime-info database * Set default association for all types to use xdg-open Note: There is still work[1] to be completed in snapd, adding OpenFile support to xdg-open. Landing this is harmless though, it will fail silently just as it does today but will start working when the snapd feature lands. 1. snapcore/snapd#4766 opening requested files, MozReview-Commit-ID: 1eeOLeVN8xQ --HG-- extra : rebase_source : e948bafc583f91177530ce00ec26c69619a7f8f9

…=jlorenzo a=lizzard This fix does several things: * Removes the mime cache generated by the desktop-gtk3 remote part * Installs a stub shared-mime-info database * Set default association for all types to use xdg-open Note: There is still work[1] to be completed in snapd, adding OpenFile support to xdg-open. Landing this is harmless though, it will fail silently just as it does today but will start working when the snapd feature lands. 1. snapcore/snapd#4766 opening requested files, MozReview-Commit-ID: 1eeOLeVN8xQ --HG-- extra : source : ca2b32cb6509a887c35db3293bd56f23fd0db131 extra : histedit_source : d438663ea755117e631f7c5a72f3a153774c2753%2Cba6b086260bb5f3c79076f589a10a717d53b3927

up process information

mvo5 · 2018-03-05T11:24:54Z

Thanks for adding the extra check to avoid the TOCTOU.

mvo5 · 2018-03-05T11:26:23Z

When this gets merged, please lets make sure we squash it during the merge for easier cherry-picking.

jdstrand · 2018-03-05T12:58:35Z

userd/helpers.go

+	// that the process ID was reused.
+	if !nameHasOwner(conn, sender) {
+		return "", fmt.Errorf("sender is no longer connected to the bus")
+	}


Thank you for adding this.

…#4766) * userd: add OpenFile D-Bus method for opening local files * interfaces: allow OpenFile method in cases where OpenURL was allowed * userd: make changes requested by zyga * userd: add a parentWindow argument, as requested by mvo * userd: stat the filename first, as requested by jdstrand * userd: check that the sender is still connected to the bus after looking up process information

mvo5 · 2018-03-05T13:07:29Z

Thanks! Merged and cherry-picked to 2.32 and master.

…=jlorenzo a=lizzard This fix does several things: * Removes the mime cache generated by the desktop-gtk3 remote part * Installs a stub shared-mime-info database * Set default association for all types to use xdg-open Note: There is still work[1] to be completed in snapd, adding OpenFile support to xdg-open. Landing this is harmless though, it will fail silently just as it does today but will start working when the snapd feature lands. 1. snapcore/snapd#4766 opening requested files, MozReview-Commit-ID: 1eeOLeVN8xQ

…=jlorenzo This fix does several things: * Removes the mime cache generated by the desktop-gtk3 remote part * Installs a stub shared-mime-info database * Set default association for all types to use xdg-open Note: There is still work[1] to be completed in snapd, adding OpenFile support to xdg-open. Landing this is harmless though, it will fail silently just as it does today but will start working when the snapd feature lands. 1. snapcore/snapd#4766 opening requested files, MozReview-Commit-ID: 1eeOLeVN8xQ UltraBlame original commit: 3800cbeee1e3d01cb5c7688fb8f50bca72ad7442

jhenstridge added 2 commits February 28, 2018 20:20

userd: add OpenFile D-Bus method for opening local files

81da2ea

interfaces: allow OpenFile method in cases where OpenURL was allowed

b094367

zyga approved these changes Feb 28, 2018

View reviewed changes

zyga requested a review from jdstrand February 28, 2018 12:53

mvo5 modified the milestones: 2.32, 2.31 Feb 28, 2018

mvo5 approved these changes Feb 28, 2018

View reviewed changes

jdstrand suggested changes Feb 28, 2018

View reviewed changes

chipaca reviewed Feb 28, 2018

View reviewed changes

jhenstridge added 2 commits March 1, 2018 19:28

userd: make changes requested by zyga

19a6e56

userd: add a parentWindow argument, as requested by mvo

3322908

jhenstridge mentioned this pull request Mar 1, 2018

xdg-open: add implementation capable of opening local files snapcore/core#82

Merged

userd: stat the filename first, as requested by jdstrand

169b9ec

mvo5 added ⚠ Critical High-priority stuff (e.g. to fix master) Sprint labels Mar 2, 2018

userd: check that the sender is still connected to the bus after looking

f5e9099

up process information

jdstrand approved these changes Mar 5, 2018

View reviewed changes

mvo5 merged commit aa4ad07 into snapcore:release/2.31 Mar 5, 2018

userd: add an OpenFile method for launching local files with xdg-open #4766

userd: add an OpenFile method for launching local files with xdg-open #4766

Conversation

jhenstridge commented Feb 28, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 28, 2018 • edited

Codecov Report

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhenstridge commented Mar 1, 2018

mvo5 commented Mar 5, 2018

mvo5 commented Mar 5, 2018

Choose a reason for hiding this comment

mvo5 commented Mar 5, 2018

codecov-io commented Feb 28, 2018 •

edited

jdstrand Feb 28, 2018 •

edited

jdstrand Mar 1, 2018 •

edited