Thanks for the report.

While access to arbitrary chronos-owned files is certainly not intentional, it doesn't cross a security boundary. We still tried to fix these cases in the past, since this is a useful mechanism to access otherwise inaccessible state on disk when constructing exploit chains. See  issue 677817  for a recent similar example.

I think the immediate action here should be to make File Manager be more careful when handling paths such that it doesn't traverse symlinks accidentally.

Assigning to satorux@ to route to the file manager folks.

As a more fundamental measure, we should investigate what we can do to lock down dangerous file system features on user-supplied file systems. I've filed issue 867807 to track that.

mnissler@, Files app is now led by slangley@. IIRC, Linux Files support was done by joelhockey@

I'll look at this early next week with the aim to disallow traversing symlinks.

dats@, I think that this is fixed if sshfs uses follow_symlinks option.  It looks like you might have already meant to include that option at https://chromium.googlesource.com/chromiumos/platform2/+/master/cros-disks/sshfs_helper.cc#37

@joelhockey: I don't think that's going to work. Here's what sshfs actually looks like: https://github.com/libfuse/sshfs/blob/master/sshfs.c

Look at sshfs_getattr():

```
static int sshfs_getattr(const char *path, struct stat *stbuf,
			 struct fuse_file_info *fi)
{
	int err;
	struct buffer buf;
	struct buffer outbuf;
	struct sshfs_file *sf = NULL;

	if (fi != NULL && !sshfs.fstat_workaround) {
		sf = get_sshfs_file(fi);
		if (!sshfs_file_is_conn(sf))
			return -EIO;
	}
	

	buf_init(&buf, 0);
	if(sf == NULL) {
		buf_add_path(&buf, path);
		err = sftp_request(sshfs.follow_symlinks ? SSH_FXP_STAT : SSH_FXP_LSTAT,
				   &buf, SSH_FXP_ATTRS, &outbuf);
	}
	else {
		buf_add_buf(&buf, &sf->handle);
		err = sftp_request(SSH_FXP_FSTAT, &buf, SSH_FXP_ATTRS, &outbuf);
	}		
	if (!err) {
		err = buf_get_attrs(&outbuf, stbuf, NULL);
#ifdef __APPLE__
		stbuf->st_blksize = 0;
#endif
		buf_free(&outbuf);
	}
	buf_free(&buf);
	return err;
}
```

As far as I can tell, the follow_symlinks option only influences what commands are sent to sftp-server, not how the data coming back is interpreted; therefore, a malicious guest should still be able to present symlinks to you, even if you can't see them when the guest is playing nice.

Adding follow_symlinks did fix this particular issue and I think is worth setting since it is likely what users would expect for how FilesApp handles symlinks in the container.

But I agree that it is still worth guarding FilesApp to not traverse symlinks.  I've created https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1152344 for follow_symlinks, and I'll look at FilesApp more.

As Jann mentions, setting follow_symlinks will only mitigate if the sshfs implementation does indeed reject symlinks received from the remote in case follow_symlinks is set. However, the sshfs implementation allows the remote side to set any file type it likes (buf_get_attrs), so follow_symlinks makes symlinks disappear with well-behaved servers, but doesn't fix the security issue.

At the very least, you'd have to patch sshfs to never report symlinks to FUSE if follow_symlinks is set. And this is still suboptimal in the light that an attacker who manages to exploit the sshfs process from a malicious ssh server,  can still inject symlinks into FUSE.

Thus, a robust fix would be to either have the kernel block symlink traversal or prevent symlinks from causing harm by isolating sshfs in a mount namespace that doesn't contain anything sensitive. Issue 867807 is meant to track these improvements.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1a00fefd737bcfca71e5a19a00da6fb1fd8eb9fc

commit 1a00fefd737bcfca71e5a19a00da6fb1fd8eb9fc
Author: Joel Hockey <joelhockey@chromium.org>
Date: Wed Aug 01 03:35:35 2018

cros-disks: Include follow_symlinks option for sshfs

Any symlinks in the container must be followed as
paths in the container rather than interpreted as
paths in the host.

BUG= chromium:867356 
TEST=Manually verified this fixes bug, updated unittest.

Change-Id: Id7aeae6f449573443f9029bce5d7163b726b84d1
Reviewed-on: https://chromium-review.googlesource.com/1152344
Commit-Ready: Joel Hockey <joelhockey@chromium.org>
Tested-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/1a00fefd737bcfca71e5a19a00da6fb1fd8eb9fc/cros-disks/sshfs_helper.cc
[modify] https://crrev.com/1a00fefd737bcfca71e5a19a00da6fb1fd8eb9fc/cros-disks/sshfs_helper_unittest.cc

Quick check - Should this fix be merged to M69? Would like to reflect the proper milestone target for tracking. Thanks!

yes, lets get this in to 69. Thanks!

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Merge approved, M69.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8b494b4248db30002f4a3877cb7b818bf4b8a0de

commit 8b494b4248db30002f4a3877cb7b818bf4b8a0de
Author: Joel Hockey <joelhockey@chromium.org>
Date: Thu Aug 09 21:53:23 2018

cros-disks: Include follow_symlinks option for sshfs

Any symlinks in the container must be followed as
paths in the container rather than interpreted as
paths in the host.

BUG= chromium:867356 
TEST=Manually verified this fixes bug, updated unittest.

Change-Id: Id7aeae6f449573443f9029bce5d7163b726b84d1
Reviewed-on: https://chromium-review.googlesource.com/1152344
Commit-Ready: Joel Hockey <joelhockey@chromium.org>
Tested-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
(cherry picked from commit 1a00fefd737bcfca71e5a19a00da6fb1fd8eb9fc)
Reviewed-on: https://chromium-review.googlesource.com/1170062
Commit-Queue: Joel Hockey <joelhockey@chromium.org>

[modify] https://crrev.com/8b494b4248db30002f4a3877cb7b818bf4b8a0de/cros-disks/sshfs_helper.cc
[modify] https://crrev.com/8b494b4248db30002f4a3877cb7b818bf4b8a0de/cros-disks/sshfs_helper_unittest.cc

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Should this be marked fixed?

Closed as fixed.

As per #c10, my understanding is that other required fixes are being tracked in separate bugs.

mnissler@, others,
Reopen if you feel like this bug still needs to track further changes in FilesApp code, or perhaps reassign if work is required elsewhere.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot