Try to preserve current working directory #48
Conversation
| @@ -104,6 +111,13 @@ int main(int argc, char **argv) | |||
| snappy_udev_cleanup(&udev_s); | |||
| #endif // ifdef STRICT_CONFINEMENT | |||
|
|
|||
| // Try to re-locate back to vanilla working directory. This can fail | |||
| // because that directory is no longer present. In that case print a | |||
| // diagnostic message and carry on without failing. | |||
There was a problem hiding this comment.
If we want snap commands to be able to operate on files in the current working directory, wouldn't we want to refuse to launch the command if we couldn't preserve the current working directory? It seems dangerous to allow chdir() to fail if you're passing something like "." or "../" to a snap command.
There was a problem hiding this comment.
This is a little problematic. I think that it depends on the snap and what it means to launch it.
There was a problem hiding this comment.
NOTE: if the chdir fails then we are in /
There was a problem hiding this comment.
Another utility that faces this same problem is schroot. It refuses to execute a command in the chroot if the cwd is not present in the chroot. It prints a helpful error message:
$ schroot -c xenial-amd64 -- ls
E: Failed to change to directory ‘/lib32’: No such file or directory
I: The directory does not exist inside the chroot. Use the --directory option to run the command in a different directory.
I'm not suggesting that schroot is a user experience that we necessarily want to recreate in snap-confine but, IMO, it seems like a sane thing to do.
There was a problem hiding this comment.
This is tricky. I agree with @tyhicks and don't like just ignoring the error, however I acknowledge @zyga's point that the command may not even need the current directory. snappy isn't a program for managing chroots either.
I think failing outright is quite unfriendly and I'm confident we will get a bug and be asked to fix it rather quickly. We should not silently fail though. I think it would be ok if the error message were changed to "cannot change directory to /foo/bar/baz; changing to '/'" then doing an explicit chdir("/") then carrying on (I know we are supposed to be there already, but code changes... like the code that introduced this bug :).
@tyhicks - do you see a problem with this approach?
There was a problem hiding this comment.
Imagine that you're in ~/project/src and you want to do some recursive operation on ~/project/tmp, such as removing all files and directories. There's a nice, snapped up command called recursive-remove that does exactly what you want. If the chdir() back to cwd fails, recursive-remove will delete your /tmp instead of ~/project/tmp.
To be clear, I'm not blocking this PR. As long as this behavior is documented clearly, this wouldn't be a security issue. I just think it could be a potential usability issue and wanted to make sure everyone signing off on the PR was aware of what could happen.
I guess I am slightly blocking this PR because I'd like to see the decided on behavior documented at least in README.md.
There was a problem hiding this comment.
Bah, my last comment left out an important detail. You'd run recursive-remove ../ from ~/project/tmp/ and, if the chdir() failed, you'd delete /tmp because /../tmp is the same thing as /tmp.
There was a problem hiding this comment.
@tyhicks may have accidentally convinced me to die() on that chdir :\ I can imagine a lot of snaps not checking their current directory properly and accidentally doing things. Granted, on a system with security confinement in place they'd 'only' be able to harm their own data, but still.
I'm starting to wonder why we are allowing this behavior at all now since the systemd file is meant to set the working directory and cli commands are meant to start in $HOME. Both of these directories are under the control of snapd and should be guaranteed to exist, so why not die()? Plus, there is no guarantee that the snap will have access to the current working directory if we set it to the one the user is in at command invocation.
This PR seems problematic after some thought-- is there some other bug that crept in that prompted this PR?
There was a problem hiding this comment.
I want to fix this because without it spread (as packaged in a snap) is useless.
This patch makes snap-confine save and then try to restore the current working directory. In the past this was not done and the working directory was always reset to the root directory, making snaps of command like tools like git or spread useless. Fixes: https://bugs.launchpad.net/snap-confine/+bug/1595444 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
zyga
force-pushed
the
fix-working-directory
branch
from
731db4e
to
93c9ff2
Compare
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This patch evolves the approach to current directory treatment. After some discussion with the security team I'm now convinced that it should always refuse to run if the current directory cannot be retained. It is better to fail early and loudly in those few cases where this will happen. Note that this is separate from being able to read or write to the current directory, this is simply related to the fact that the crafted chroot that snap execute in cannot contain all the locations that are present on the outside. This patch changes the old behavior of relocating to the root directory to an explicit failure message followed by exit from snap-confine. The regression test was updated to match this. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
| // Try to re-locate back to vanilla working directory. This can fail | ||
| // because that directory is no longer present. | ||
| if (chdir(vanilla_cwd) != 0) { | ||
| // NOTE: Use exit rather than die as this produces a more refined error message |
There was a problem hiding this comment.
Hm, I wonder if the perror() from die() might be interessting here for the user. E.g. to see that its a permsision denied error vs a not-exit error and the like.
There was a problem hiding this comment.
Hmm, I can switch back to die, given that we run as root the only cause for this to fail might be apparmor. In any case, I'll switch to die and we'll see.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
| char vanilla_cwd[512]; | ||
| if (getcwd(vanilla_cwd, sizeof vanilla_cwd) == NULL) { | ||
| die("cannot get the current working directory"); | ||
| } |
There was a problem hiding this comment.
I suggest using get_current_dir_name() instead of getcwd(), like we do in mount-support.c. This has the advantage of not having to pick an arbitrary size for vanilla_cwd but does mean you need to free() the result.
This patch changes getcwd to get_current_dir_name and does a tiny refactor to move cleanup functions to a dedicated module so that they can be shared. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
|
I'll merge this shortly. Tests on debian will fail but that's expected now. We should either move to debian-sid or use dpkg-vendor patches to build a different package on debian. |
This patch makes snap-confine save and then try to restore the current
working directory. In the past this was not done and the working
directory was always reset to the root directory, making snaps of
command like tools like git or spread useless.
Fixes: https://bugs.launchpad.net/snap-confine/+bug/1595444
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com