Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap: make users Xauthority file available in snap environment #3177
Conversation
mvo5
added this to the 2.25 milestone
Apr 11, 2017
| + | ||
| + // If everything is ok, we can now point the snap to the new | ||
| + // location of the Xauthority file. | ||
| + setEnv("XAUTHORITY", targetPath) |
zyga
Apr 11, 2017
Contributor
This is a nitpick but I'm not a fan of setenv, it's not safe in multi threaded programs and we can avoid it by remembering the new XAUTHORITY value and setting it at exec time.
| @@ -33,6 +33,10 @@ const x11ConnectedPlugAppArmor = ` | ||
| /var/cache/fontconfig/ r, | ||
| /var/cache/fontconfig/** mr, | ||
| + | ||
| +# Allow access to the user specific copy of the xauth file specified | ||
| +# in the XAUTHORITY environment variable snap run creates on startup. |
zyga
Apr 11, 2017
Contributor
# Allow access to the user specific copy of the xauth file specified
# in the XAUTHORITY environment variable, that "snap run" creates on startup.
| + "os" | ||
| +) | ||
| + | ||
| +type xauth struct { |
zyga
Apr 11, 2017
Contributor
Where is this documented (as a file format) can you please link to such documentation.
| +} | ||
| + | ||
| +func readChunk(f *os.File) ([]byte, error) { | ||
| + b := make([]byte, 2) |
morphis
Apr 11, 2017
Contributor
A static [2]byte didn't work with the f.Read call below if I remember well. Let me try that again.
zyga
Apr 11, 2017
Contributor
You need to get the slice for f.Read but the make makes no sense IMO (pun intended)
| + return nil | ||
| +} | ||
| + | ||
| +func ValidateXauthority(path string) (int, error) { |
zyga
Apr 11, 2017
Contributor
This seems misnamed, it's not really validating and it returns an int. How about countAuthCookies(path string) (int, error)
zyga
Apr 12, 2017
Contributor
See below, I changed my mind later during the review. this should just return error.
| + return cookies, nil | ||
| +} | ||
| + | ||
| +func MockXauthority(cookies int) string { |
zyga
Apr 11, 2017
Contributor
You should put test code for foo.go in foo_test.go. Alternatively if the test needs access to private function you can use the export_test.go trick.
| +} | ||
| + | ||
| +func MockXauthority(cookies int) string { | ||
| + f, _ := ioutil.TempFile("", "xauth") |
zyga
Apr 11, 2017
Contributor
Typically you'd pass an instance of c.C in and then c.Assert(err, IsNil).
You should also do defer f.Close() after checking that err is nil.
| +// Hook up check.v1 into the "go test" runner | ||
| +func Test(t *testing.T) { TestingT(t) } | ||
| + | ||
| +type XauthTestSuite struct {} |
| +func (s *XauthTestSuite) TestXauthFileNotAvailable(c *C) { | ||
| + n, err := x11.ValidateXauthority("/does/not/exist") | ||
| + c.Assert(n, Equals, 0) | ||
| + c.Assert(err, Not(IsNil)) |
| + c.Assert(n, Equals, len(data)) | ||
| + | ||
| + n, err = x11.ValidateXauthority(f.Name()) | ||
| + c.Assert(err, DeepEquals, fmt.Errorf("Could not read enough bytes")) |
| +} | ||
| + | ||
| +func (s *XauthTestSuite) TestXauthFileExistsButHasInvalidContent(c *C) { | ||
| + f, err := ioutil.TempFile("", "xauth") |
zyga
Apr 11, 2017
Contributor
If you structure your APIs so that you can read from io.Reader and then have a wrapper that does the open/defer close/read dance then you can just write all those tests with string readers instead.
| + | ||
| +func (s *XauthTestSuite) TestValidXauthFile(c *C) { | ||
| + path := x11.MockXauthority(1) | ||
| + n, err := x11.ValidateXauthority(path) |
zyga
Apr 11, 2017
Contributor
I changed my mind, this should just return an error and not any numbers.
| ) | ||
| var ( | ||
| syscallExec = syscall.Exec | ||
| userCurrent = user.Current | ||
| + getEnv = os.Getenv |
mvo5
Apr 12, 2017
Collaborator
(Super nicktpick) most code uses the convention osGetenv = os.Getenv (same in the line below).
mvo5
reviewed
Apr 12, 2017
This looks very nice, thank you! Don't be discouraged from the amount of comments, its all nitpicks and comments about the conventions.
| + cookies, err := x11.ValidateXauthority(tmpXauthPath) | ||
| + if err != nil { | ||
| + return err | ||
| + } else if cookies == 0 { |
mvo5
Apr 12, 2017
Collaborator
(nitpick) I don't think we need the else. If err != nil it already returns.
| + } | ||
| + | ||
| + if !osutil.FileExists(baseTargetDir) { | ||
| + os.MkdirAll(baseTargetDir, 0700) |
mvo5
Apr 12, 2017
Collaborator
We probably want an error check here, something like: if err := os.MkdirAll(...); err != nil {return err}
| + os.MkdirAll(baseTargetDir, 0700) | ||
| + } | ||
| + | ||
| + err = osutil.CopyFile(tmpXauthPath, targetPath, flags) |
mvo5
Apr 12, 2017
Collaborator
This can be done in a single line: if err := osutil.CopyFile(...); err != nil {.
| @@ -230,6 +302,10 @@ func runSnapConfine(info *snap.Info, securityTag, snapApp, command, hook string, | ||
| logger.Noticef("WARNING: cannot create user data directory: %s", err) | ||
| } | ||
| + if err := migrateXauthority(info); err != nil { | ||
| + logger.Noticef("WARNING: cannot copy user Xauthority file: %s", err) |
| + n, err := f.Read(b) | ||
| + if err != nil { | ||
| + return nil, err | ||
| + } else if n != 2 { |
mvo5
Apr 12, 2017
Collaborator
The else is not really needed here, is it? If err != nil it returns anyway.
morphis
Apr 12, 2017
Contributor
If I read https://golang.org/pkg/os/#File.Read correctly then there is the case that you supply an array of a specifc length but don't get all bytes filled back without any error. We need to ensure here that we get 2 bytes. Otherwise the file is invalid.
mvo5
Apr 18, 2017
Collaborator
Sorry, I did not express myself very well. I was thinking of just a tiny style change:
n, err := f.Read(b)
+ if err != nil {
+ return nil, err
+ }
+ if n != 2 {
(i.e. drop the "else" but keep the check for n!=2)
| + n, err = f.Read(chunk) | ||
| + if err != nil { | ||
| + return nil, err | ||
| + } else if n != size { |
| + return chunk, nil | ||
| +} | ||
| + | ||
| +func (xa *xauth) ReadFromFile(f *os.File) error { |
mvo5
Apr 12, 2017
Collaborator
It looks like this is used only internally so I think we can make it private.
| +} | ||
| + | ||
| +func ValidateXauthority(path string) (int, error) { | ||
| + f, err := os.OpenFile(path, os.O_RDONLY, 0600) |
mvo5
Apr 12, 2017
Collaborator
I think you can just use os.Open(path) here, internally it will open the file readonly and its slightly shorter.
| +func (s *XauthTestSuite) TestXauthFileNotAvailable(c *C) { | ||
| + n, err := x11.ValidateXauthority("/does/not/exist") | ||
| + c.Assert(n, Equals, 0) | ||
| + c.Assert(err, Not(IsNil)) |
| +} | ||
| + | ||
| +func (s *XauthTestSuite) TestXauthFileExistsButIsEmpty(c *C) { | ||
| + f, err := ioutil.TempFile("", "xauth") |
mvo5
Apr 12, 2017
Collaborator
(nitpick^99) - you could use tempdir := c.MkDir(); ioutil.WriteFile(filepath.Join(tempfile, "xauth"), nil, 0600) here which is a whopping one line shorter ;) so really not worth it. Mostly wanted to show c.MkDir() as its nice and the gocheck code will do the cleanup for us of that temp dir (you could create a tempdir in in SetupTest() and use use that for the tempfiles without having to worry about cleanup. But I'm really into nitpick land now :)
| +func migrateXauthority(info *snap.Info) error { | ||
| + u, err := userCurrent() | ||
| + if err != nil { | ||
| + return fmt.Errorf(i18n.G("cannot get the current user: %v"), err) |
zyga
reviewed
Apr 13, 2017
Looks much nicer, thank you for iterating on this. I left a few more comments but I think this is getting closer to being merged. I'm still a bit convinced we should avoid setenv but I would like to discuss this with @mvo5. I would like to see some spread tests as well, maybe a simple test where we have a canned correct Xauth file that we see validated and copied to the execution environment (no need to run anything graphical).
| + return fmt.Errorf(i18n.G("cannot get the current user: %v"), err) | ||
| + } | ||
| + | ||
| + // If we're running as root then we don't do anything |
morphis
Apr 13, 2017
Contributor
Idea was that it doesn't make sense to allow root using X11 but there could be edge cases where people do this. Regular case will be always that a user != root has an XAUTHORITY set. Will drop this. @jdstrand please correct me if I miss anything.
| + | ||
| + // Only validate Xauthority file again when both files don't match | ||
| + // ohterwise we can continue using the existing Xauthority file | ||
| + if osutil.FileExists(targetPath) && osutil.FilesAreEqual(targetPath, tmpXauthPath) { |
| + // either the data can't be parsed or there are no cookies in | ||
| + // the file is invalid. | ||
| + if err := x11.ValidateXauthority(tmpXauthPath); err != nil { | ||
| + return nil |
| + | ||
| + // If everything is ok, we can now point the snap to the new | ||
| + // location of the Xauthority file. | ||
| + osSetenv("XAUTHORITY", targetPath) |
zyga
Apr 13, 2017
Contributor
Just re-stating the setenv-is-dangerous comment I made earlier. I'll let @mvo5 comment on this, if we want to re-work it to avoid setenv entirely.
| + "os" | ||
| +) | ||
| + | ||
| +// See https://cgit.freedesktop.org/xorg/lib/libXau/tree/AuRead.c and |
| + | ||
| +func (xa *xauth) readFromFile(f *os.File) error { | ||
| + b := [2]byte{} | ||
| + _, err := f.Read(b[:]) |
|
@zyga Adding a spread test for this. |
jdstrand
requested changes
Apr 13, 2017
I'm not done with the PR review but have enough for an initial comment.
| @@ -216,6 +220,68 @@ func snapRunHook(snapName, snapRevision, hookName string) error { | ||
| return runSnapConfine(info, hook.SecurityTag(), snapName, "", hook.Name, nil) | ||
| } | ||
| +func migrateXauthority(info *snap.Info) error { | ||
| + u, err := userCurrent() |
jdstrand
Apr 13, 2017
Contributor
Nit-pick: it might make the code easier to read if this was closer to where it is first used.
| + // Nothing to do for us. Most likely running outside of any | ||
| + // graphical X11 session. | ||
| + return nil | ||
| + } |
jdstrand
Apr 13, 2017
•
Edited 1 time
-
jdstrand
Apr 13, 2017
Contributor
-
Apr 13, 2017
In thinking about this more, there is an attack I'd like to add a little more protection against. Consider:
- sysadmin updates sudoers to allow running 'sudo /snap/bin/foo' by some non-admin user
- that user does
XAUTHORITY=/etc/shadow sudo /snap/bin/foo - /etc/shadow would be copied to /run/user/0/Xauthority
- foo is somehow controllable by the non-admin user (eg, flaw in foo, developer of foo, etc) such that /run/user/0/Xauthority (ie, /etc/shadow) can be shipped off, copied to SNAP_DATA, etc
This sort of attack is precisely why I recommended validating the xauth file (ie, ValidateXauthority, below) but I think a nice hardening/defensive-coding measure would be to check if XAUTHORITY is in /tmp to help guard against if our validation code has a bug. The whole reason we are doing this is because /tmp is in a different mount namespace and some distros put it in there, setting XAUTHORITY. Let's just fix that rather than trying to be overly generic. As such, please, after you've stored off xauthPath (ie, right where this comment is), please add something to the effect of:
// Abs() also calls Clean()
// https://golang.org/pkg/path/filepath/#Abs
xauthPathAbs, err := filepath.Abs(xauthPath)
if err != nil {
return nil
}
// remove all symlinks from path
xauthPathCan, err := filepath.EvalSymlinks(xauthPathAbs)
if err != nil {
return nil
}
// verify XAUTHORITY matches the cleaned path
if xauthPath != xauthPathCan {
logger.Noticef("WARNING: %s != %s\n", xauthPath, xauthPathCan)
return nil
}
// Only do the migration from /tmp since /tmp is what is in a different
// mount namespace. There is a TOCTOU between this and the
// copy below, but this is just a quick check before validating the file
if !strings.HasPrefix(xauthPath, "/tmp") {
logger.Noticef("WARNING: %s is not in /tmp", xauthPath)
return nil
}
jdstrand
Apr 13, 2017
Contributor
I don't like the TOCTOU here and will think about improving this bit.
jdstrand
Apr 13, 2017
•
Edited 1 time
-
jdstrand
Apr 13, 2017
Contributor
-
Apr 13, 2017
The way to do this is change the algorithm a bit. This also means we don't need a tempfile which is something Gustavo was looking for in the forum
- if defined and XAUTHORITY not zero length, open XAUTHORITY to get a file object and return nil if it doesn't exist
- perform all the above recommended checks on file.Name() (the name that was used to open the file) and return nil in case of shenanigans or not in /tmp
- read the contents from the file object
- if the file contents are the same as what is already in XDG_RUNTIME_DIR/.Xauthority, set XAUTHORITY (via snapenv?) and return
- otherwise, validate the content of the file object. if invalid, return nil with warning
- since valid, securely write it out to XDG_RUNTIME_DIR/.Xauthority, 0600 and owned by userCurrent (let's just set up the permissions securely and not worry about copying the perms). set XAUTHORITY (via snapenv?) and return
morphis
Apr 18, 2017
Contributor
@jdstrand I've implemented this now. Can you check if it matches now your expectations? The code is still a bit rough and tests are not adjusted but wanted to have these changes up as quick as possible to not loose time on your general feedback. Will work on optimizations and test cases next.
| + } | ||
| + | ||
| + // Copy Xauthority file into a temporary place so we can safely | ||
| + // process it further there. |
| + } | ||
| + | ||
| + baseTargetDir := filepath.Join(dirs.XdgRuntimeDirBase, u.Uid) | ||
| + targetPath := filepath.Join(baseTargetDir, "Xauthority") |
jdstrand
Apr 13, 2017
Contributor
I'm leaning towards this being '.Xauthority' instead. If we are going to pick a name, let's pick one that doesn't clutter up the user's XDG_RUNTIME_DIR.
| + targetPath := filepath.Join(baseTargetDir, "Xauthority") | ||
| + | ||
| + // Only validate Xauthority file again when both files don't match | ||
| + // ohterwise we can continue using the existing Xauthority file |
jdstrand
Apr 13, 2017
Contributor
Can you append to this comment: "This is ok to do here because we aren't trying to protect against the user changing the Xauthority file in XDG_RUNTIME_DIR outside of snapd."
| + // Only validate Xauthority file again when both files don't match | ||
| + // ohterwise we can continue using the existing Xauthority file | ||
| + if osutil.FileExists(targetPath) && osutil.FilesAreEqual(targetPath, tmpXauthPath) { | ||
| + osSetenv("XAUTHORITY", targetPath) |
jdstrand
Apr 13, 2017
Contributor
Wondering if this should be in snap/snapenv/snapenv.go as it handles setting various other environment variables.
| + | ||
| + // Ensure that we have a valid Xauthority. It is invalid when | ||
| + // either the data can't be parsed or there are no cookies in | ||
| + // the file is invalid. |
jdstrand
Apr 13, 2017
Contributor
Please adjust to be:
// To guard against setting XAUTHORITY to non-xauth files, check
// that we have a valid Xauthority. Specifically, the file must be
// parseable as an Xauthority file and not be empty.
| + if err := os.MkdirAll(baseTargetDir, 0700); err != nil { | ||
| + return err | ||
| + } | ||
| + } |
jdstrand
Apr 13, 2017
Contributor
This is interesting because it is getting into the bug where snapd is not creating XDG_RUNTIME_DIR for the snap itself. I think we absolutely should set /run/user/<uid>/snap.$SNAP_NAME, but conditionally creating only /run/user/<uid> here is not correct because we are creating it only sometimes when we should be creating it all the time. Also, the permissions should be:
- /run - 0755
- /run/user - 0755
- /run/user/<uid> - 0700
- /run/user/<uid>/snap.$SNAP_NAME - 0700 (not implemented)
/run will (in practice) always exist, but it's conceivable /run/user may not on core if we ever don't use systemd (looks like src/login/logind-user.c in systemd creates this for us). Since we can assume /run and /run/user are created, I think that this should be changed to os.Mkdir() from os.MkdirAll() and moved somewhere outside of this function and before this function is called (then we can just error out/return nil if /run/user/<uid> doesn't exist). Where you move should also be where we create /run/user/<uid>/snap.$SNAP_NAME.
morphis
Apr 18, 2017
Contributor
I think the creation of a correct XDG_RUNTIME_DIR and where it is shouldn't be part of this PR and is a thing we need to debate separately. I will change the code here for now to not attempt to create XDG_RUNTIME_DIR. @jdstrand Is that ok for you?
| +# Allow access to the user specific copy of the xauth file specified | ||
| +# in the XAUTHORITY environment variable, that "snap run" creates on | ||
| +# startup. | ||
| +owner /run/user/[0-9]*/Xauthority r, |
| + | ||
| +// ValidateXauthority validates a given Xauthority file. The file is valid | ||
| +// if it can be parsed and contains at least one cookie. | ||
| +func ValidateXauthority(path string) error { |
morphis
Apr 18, 2017
Contributor
Added in migrateXauthority as this is just about validation of any xauth file regardless who owns it.
| + } | ||
| + // FIXME we can do further validation of the cookies like | ||
| + // checking for valid families etc. | ||
| + cookies += 1 |
jdstrand
requested changes
Apr 18, 2017
I still want to take a closer look at the validate code, but here is the feedback you requested.
| +func migrateXauthority(info *snap.Info) error { | ||
| + u, err := userCurrent() | ||
| + if err != nil { | ||
| + return fmt.Errorf(i18n.G("cannot get the current user: %s"), err) |
| + baseTargetDir := filepath.Join(dirs.XdgRuntimeDirBase, u.Uid) | ||
| + if !osutil.FileExists(baseTargetDir) { | ||
| + return fmt.Errorf("Target directory %s does not exist", baseTargetDir) | ||
| + } |
jdstrand
Apr 18, 2017
Contributor
I like the intent here, but migrateXauthority is currently unconditionally called for every snap-confine invocation (regardless of if it is needed) and while systemd seems to create /run/user/<uid> for user sessions, it does not for root so this will break daemons and invocations using sudo.
| + fin, err := os.Open(xauthPath) | ||
| + if err != nil { | ||
| + return err | ||
| + } |
| + | ||
| + // Verify XAUTHORITY matches the cleaned path | ||
| + if fin.Name() != xauthPathCan { | ||
| + logger.Noticef("WARNING: %s != %s\n", xauthPath, xauthPathCan) |
| + // Only do the migration from /tmp since /tmp is what is in a different | ||
| + // mount namespace. There is a TOCTOU between this and the | ||
| + // copy below, but this is just a quick check before validating the file | ||
| + if !strings.HasPrefix(fin.Name(), "/tmp") { |
jdstrand
Apr 18, 2017
Contributor
Can you use '/tmp/' here? You can remove the TOCTOU comment since you are working on an open file now.
| + } | ||
| + | ||
| + // Ensure that the file is owned by the current user | ||
| + fi, err := fin.Stat() |
jdstrand
Apr 18, 2017
Contributor
You are using fin.Stat() here and it is using fin.Name() but note the actual fstat() system call is done here, not at the time of the open so any chown()s done to the file after the open but before this fstat() will apply (ie, if chown to root between the open and the fstat, the fstat will show as root or if chown to non-root between the open and the fstat, the fstat will show non-root). I think the check is fine where it is though since we aren't trying to protect the user from herself. If the user has rights to chown the file, then we'll continue to honor that, but I think there needs to be a comment. Please use:
// We are performing a Stat() here to make sure that the user can't
// steal another user's Xauthority file. Note that while Stat() uses
// fstat() on the file descriptor created during Open(), the file might
// have changed ownership between the Open() and the Stat(). That's ok
// because we aren't trying to block access that the user already has:
// if the user has the privileges to chown another user's Xauthority
// file, we won't block that since the user can just steal it without
// having to use snap run. This code is just to ensure that a user who
// doesn't have those privileges can't steal the file via snap run
// (also note that the (potentially untrusted) snap isn't running yet).
| + fi, err := fin.Stat() | ||
| + if err != nil { | ||
| + return err | ||
| + } else { |
jdstrand
Apr 18, 2017
Contributor
You can drop this else and move everything over since the above error returns.
| + return fmt.Errorf("Can't validate file owner") | ||
| + } | ||
| + // cheap comparison but better to convert uid to a string than | ||
| + // a string into a number. |
jdstrand
Apr 18, 2017
Contributor
Can you clarify this comment by saying that u.Uid is a string which is why you are converting the stat to a uid?
| + // cheap comparison but better to convert uid to a string than | ||
| + // a string into a number. | ||
| + if fmt.Sprintf("%d", sys.(*syscall.Stat_t).Uid) != u.Uid { | ||
| + return fmt.Errorf("Xauthority file isn't owned by the current user") |
jdstrand
Apr 18, 2017
Contributor
Also, while I definitely like this check, I'm not 100% convinced we should error here. Should we warn and return nil (ie, skip migration) instead?
| + if fout, err = os.Open(targetPath); err != nil { | ||
| + return err | ||
| + } | ||
| + if osutil.FileStreamsEqual(fin, fout) { |
| + if osutil.FileStreamsEqual(fin, fout) { | ||
| + osSetenv("XAUTHORITY", targetPath) | ||
| + return nil | ||
| + } |
jdstrand
Apr 18, 2017
Contributor
fout leaked here. I would say to defer the close, but you reuse fout below by reopening the file. You either need to rework the open logic, use two different vars or explicitly close here.
| + if err := x11.ValidateXauthority(fin.Name()); err != nil { | ||
| + logger.Noticef("WARNING: invalid Xauthority file: %s", err) | ||
| + return nil | ||
| + } |
jdstrand
Apr 18, 2017
•
Edited 1 time
-
jdstrand
Apr 18, 2017
Contributor
-
Apr 18, 2017
You need to pass fin here, not fin.Name(), to avoid the TOCTOU issues.
| + return nil | ||
| + } | ||
| + | ||
| + fout, err = os.OpenFile(targetPath, os.O_WRONLY|os.O_CREATE, 0600) |
jdstrand
Apr 18, 2017
Contributor
This file needs to be securely created so it really needs os.O_EXCL, however the program logic atm means this file might exist (with different contents from XAUTHORITY). I suggest deleting the file and then using O_EXCL. Otherwise, create a temporary file somewhere else and move it into place (eg, via osutil.AtomicWriteFile).
mvo5
reviewed
Apr 21, 2017
This looks very good, very careful and thoughtful code! Still some comments, mostly nitpick and questions (sorry for that!), but this super close to be ready.
| + return "", nil | ||
| + } | ||
| + | ||
| + // Verify XAUTHORITY matches the cleaned path |
mvo5
Apr 21, 2017
Collaborator
I had a bit of trouble understanding what is going on here, i.e. why this is done etc. A small comment would be helpful I think. Maybe something like: Ensure the XAUTHORITY env is not abused by checking that it point to exactly the file we just opened (no symlinks, no funny "../.." etc). Or a better command, maybe @jdstrand has some input.
| + } | ||
| + | ||
| + // Only do the migration from /tmp since /tmp is what is in a different | ||
| + // mount namespace. |
mvo5
Apr 21, 2017
Collaborator
Maybe: Only do the migration from /tmp since the real /tmp is not visible for snaps ?
| + return "", nil | ||
| + } | ||
| + | ||
| + os.Remove(targetPath) |
| + return "", err | ||
| + } | ||
| + | ||
| + // Read data from the beginning of the file |
mvo5
Apr 21, 2017
Collaborator
(nitpick): I would move this up before the OpenFile() that openfile,copy,close are next to each other.
| + } | ||
| + | ||
| + // Read and write validated Xauthority file to its right location | ||
| + buf := make([]byte, 1024) |
mvo5
Apr 21, 2017
Collaborator
Could this be written as:
if _, err = io.Copy(fout, fin); err != nil {
fout.Close()
// FIXME: err handling
os.Remove(targetPath)
return "", fmt.Errorf(i18n.G("Failed to write new Xauthority file at %s"), targetPath)
}
or is there a specific reason for doing it "manually"?
| + c.Assert(err, check.IsNil) | ||
| + | ||
| + // Ensure XDG_RUNTIME_DIR exists for the user we're testing with | ||
| + os.MkdirAll(filepath.Join(dirs.XdgRuntimeDirBase, u.Uid), 0700) |
mvo5
Apr 21, 2017
Collaborator
We probably want err = os.MkdirAll(...); c.Assert(err,IsNil) here. Just for good measure :)
| + defer restorer() | ||
| + | ||
| + // Ensure we have XDG_RUNTIME_DIR for the current user | ||
| + os.MkdirAll(filepath.Join(dirs.XdgRuntimeDirBase, u.Uid), 0700) |
mvo5
Apr 21, 2017
Collaborator
Is that the same os.MkdirAll() as a few lines above? Or a different one? If a different one, maybe a small comment whats different about it?
| + snap install hello-world | ||
| + | ||
| + ensure_xauth_path() { | ||
| + export XAUTHORITY=$1 |
mvo5
Apr 21, 2017
Collaborator
(nitpick, sorry, shell makes my grumpy ;) - generally I think export XAUTHORITY="$1" (i.e. quotes) is better. However given that its a test etc this is nitpick^99 territory.
| + } | ||
| + | ||
| + fout, err = os.OpenFile(targetPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) | ||
| + defer fout.Close() |
mvo5
Apr 21, 2017
Collaborator
This needs to be after the "if err != nil"check, if err != nil then *os.File is nil and this would crash.
| + | ||
| + // Read and write validated Xauthority file to its right location | ||
| + if _, err = io.Copy(fout, fin); err != nil { | ||
| + err := os.Remove(targetPath) |
mvo5
Apr 21, 2017
Collaborator
This could be a single line if err := os.Remove(...); err != nill. Also maybe slightly confusing, if the copy fails and the remove also fails we swallow one of the errors. Maybe logger.Notice about the remove failure? Or going back to your original code and simply ignoring the error. Sorry for complicating things.
jdstrand
requested changes
Apr 21, 2017
I've reviewed the parsing algorithm as well as all the requested file operations and the overall approach and implementation look really good. Thanks! From my point of view, just a few small things and testsuite additions.
| + // string than a string into a number. | ||
| + if fmt.Sprintf("%d", sys.(*syscall.Stat_t).Uid) != u.Uid { | ||
| + return "", fmt.Errorf(i18n.G("Xauthority file isn't owned by the current user %s"), u.Uid) | ||
| + } |
jdstrand
Apr 21, 2017
•
Edited 1 time
-
jdstrand
Apr 24, 2017
Contributor
-
Apr 24, 2017
Didn't get an answer to my previous question, so asking again: "Also, while I definitely like this check, I'm not 100% convinced we should error here. Should we warn and return nil (ie, skip migration) instead?"
morphis
Apr 24, 2017
Contributor
That is what we do already. This part returns and error which is logged by the caller (see runSnapConfine) as a warning but the app is still being executed. In terms of Xauthority migration this is an error but we leave it up the caller here to decide what should happen when the migration fails.
jdstrand
Apr 24, 2017
•
Edited 1 time
-
jdstrand
Apr 24, 2017
Contributor
-
Apr 24, 2017
Oh, duh-- not sure why I added that comment-- maybe I was looking at an out of date tab... Anyway, yes, you addressed it.
| + return "", nil | ||
| + } | ||
| + | ||
| + if osutil.FileExists(targetPath) { |
jdstrand
Apr 21, 2017
Contributor
A comment here would be nice. Perhaps "Replace the file securely by removing and creating with O_CREATE|O_EXCL"
| + if err != nil { | ||
| + return "", err | ||
| + } | ||
| + defer fout.Close() |
jdstrand
Apr 21, 2017
Contributor
It's a bit weird that you os.Open()d above using fout, deferred the Close() then os.OpenFile()d here using fout and defer this Close(). Perhaps with os.Open(), above, just explicitly Close() so there is no confusion?
morphis
Apr 24, 2017
Contributor
Thanks for the comment. The code was indeed a bit confusing. Reworked it now.
| + # Data | ||
| + echo -n -e \\x00\\x01\\xff >> $1 | ||
| + done | ||
| + } |
jdstrand
Apr 21, 2017
Contributor
For the spread tests, I suggest using a real Xauthority file generated with xauth otherwise you are mocking data for precisely the way the code is written (mocking in this way in the unit tests is fine imo). Eg:
rm -f "$1"
touch "$1"
chmod 600 "$1"
xauth -f "$1" add localhost:0 . 00112233445566778899aabbccddeeff
xauth -f "$1" list
Using xauth add you don't need a running X server.
morphis
Apr 24, 2017
Contributor
We can add this as additional test but we don't have xauth available on core type systems. I will add this as additional test case for non core systems.
| + # Xauthority should be correctly migrated | ||
| + ensure_xauth_path /tmp/valid-xauthority /run/user/0/.Xauthority | ||
| + test -e /run/user/0/.Xauthority | ||
| + |
jdstrand
Apr 21, 2017
Contributor
Adding a test for an xauth file that has 2 cookies would be good to, so make sure they all got copied and not just one.
morphis
Apr 24, 2017
Contributor
We generate a xauth file with four cookies by default. See https://github.com/snapcore/snapd/pull/3177/files/c97fce06b70a54d089def2677c96aa85b4f880aa#diff-2a1421d1bbec61fd7f67fe8b28c0f138R64
| +// https://cgit.freedesktop.org/xorg/lib/libXau/tree/include/X11/Xauth.h | ||
| +// for details about the actual file format. | ||
| +type xauth struct { | ||
| + Family uint16 |
jdstrand
Apr 21, 2017
•
Edited 1 time
-
jdstrand
Apr 24, 2017
Contributor
-
Apr 24, 2017
I guess you chose this to make sure the C 'unsigned short' would always fit? I'm not terribly excited about this assumption since I believe some standards state unsigned short is a minimum of 16 bits. However, C99 and C11 both state the size of an unsigned int as precisely 16 bits. Anecdotally, I double checked the sizeof(unsigned short) on s390x, powerpc, ppc64el, i386, amd64, armhf, and arm64 and it as indeed 16 bits. As such, I think this is fine as is.
| + return err | ||
| + } else if n != 2 { | ||
| + return fmt.Errorf("Could not read enough bytes") | ||
| + } |
jdstrand
Apr 21, 2017
Contributor
These 7 lines of code are duplicated here at the top of readChunk and extremely similar in the next part in readChunk. I wonder if it would make sense to abstract this out into readBytes(n int), then you call readBytes(2) to get the Family, readBytes(2) to get the size then readBytes(size) to get the chunk.
Also, you reference the X sources (which is great!) but it would be good to explicitly comment that Family is two bytes, and len for the strings is two bytes.
morphis
Apr 24, 2017
Contributor
Added a function readBytes and a comment to explain the lengths used.
| + c.Assert(err, IsNil) | ||
| + err = x11.ValidateXauthority(path) | ||
| + c.Assert(err, IsNil) | ||
| +} |
jdstrand
Apr 21, 2017
Contributor
Adding a unit test with two cookies in the xauth file would be good too.
| + | ||
| + # Generate valid Xauthority file which `snap run` will accept | ||
| + mock_xauthority /tmp/valid-xauthority 4 | ||
| + chmod 600 /tmp/valid-xauthority |
jdstrand
Apr 21, 2017
•
Edited 1 time
-
jdstrand
Apr 21, 2017
Contributor
-
Apr 21, 2017
One more thing, doing a sha256sum on the xauth in /tmp against the one in XDG_RUNTIME_DIR would be great here. I anecdotally checked exactly this on s390x (big endien), ppc64el, i386, amd64, armhf, and arm64 (all little endien) to blackbox test it, so it is good, but a spread test would ensure it stays good. :)
|
@jdstrand Thanks for all the comments. Will rework necessary portions today. |
jdstrand
approved these changes
Apr 24, 2017
I'm going to mark this as 'Approved' since all that is needed are a couple of testsuite additions and removing a bit of redundant code.
| + if err != nil { | ||
| + logger.Noticef("WARNING: failed to remove existing file %s", targetPath) | ||
| + } | ||
| + } |
jdstrand
Apr 24, 2017
Contributor
I'm not sure if the os.Remove() at line 327 was added later or I just didn't notice it, but with it and the O_CREAT|O_EXCL, you don't need this stanza (it is unreachable; though you probably want the error checking from here moved up to line 327 since it doesn't have it).
| + if err := readBytes(f, b[:]); err != nil { | ||
| + return err | ||
| + } | ||
| + xa.Family = binary.BigEndian.Uint16(b[:]) |
jdstrand
Apr 24, 2017
Contributor
A comment here on the family length like you did for readChunk would be nice.
added some commits
Apr 3, 2017
All checks have passed
mvo5
merged commit 2f300e5
into
snapcore:master
Apr 25, 2017
6 checks passed
niemeyer
reviewed
Apr 25, 2017
@morphis Thank you very much for implementing this.
A few simple late notes, mostly about error handling but a few about other details. Can we please make sure these are handled soon so the next release takes them in?
Thanks!
| + | ||
| + fin, err := os.Open(xauthPath) | ||
| + if err != nil { | ||
| + return "", err |
niemeyer
Apr 25, 2017
Contributor
Message needs to point out that this is about the Xauthority file, as the filename might not give a hint. So user ends up with a "cannot open" message without knowing why it's even trying to open.
morphis
Apr 26, 2017
Contributor
Not really. If you look into runSnapConfine from there is already a logger.Noticef call which puts a message around this error.
| + // it point to exactly the file we just opened (no symlinks, | ||
| + // no funny "../.." etc) | ||
| + if fin.Name() != xauthPathCan { | ||
| + logger.Noticef("WARNING: %s != %s\n", fin.Name(), xauthPathCan) |
niemeyer
Apr 25, 2017
Contributor
This is too vague. Something along those lines would be better:
("XAUTHORITY environment value is not a clean path: %q", xuathPathCan)
| + } | ||
| + sys := fi.Sys() | ||
| + if sys == nil { | ||
| + return "", fmt.Errorf(i18n.G("Can't validate file owner")) |
niemeyer
Apr 25, 2017
Contributor
"Can't" => "cannot", and needs to include more details about which file that is.
| + | ||
| + fout.Close() | ||
| + if err := os.Remove(targetPath); err != nil { | ||
| + logger.Noticef("WARNING: failed to remove existing file %s", targetPath) |
niemeyer
Apr 25, 2017
Contributor
Here we apparently warn and then return the same error, which is then warned again from the call site.
| + // that we have a valid Xauthority. Specifically, the file must be | ||
| + // parseable as an Xauthority file and not be empty. | ||
| + if err := x11.ValidateXauthorityFromFile(fin); err != nil { | ||
| + logger.Noticef("WARNING: invalid Xauthority file: %s", err) |
niemeyer
Apr 25, 2017
Contributor
It's not clear why/when do we sometimes return the error and warn above, and sometimes warn and return nil. It'd be good to make this more consistent. If we're warning from the call site, let's return the error here as well, since this is an error similar to the other cases here, and let the call site warn too.
morphis
Apr 26, 2017
Contributor
I am return in most cases now an error and only print a warning when we can't return an error. Please check if the implementation now fits what you were thinking about.
| + // Read and write validated Xauthority file to its right location | ||
| + if _, err = io.Copy(fout, fin); err != nil { | ||
| + if err := os.Remove(targetPath); err != nil { | ||
| + logger.Noticef("WARNING: failed to remove file at %s", targetPath) |
niemeyer
Apr 25, 2017
Contributor
"failed to" => "cannot", and the original error is useful to include after a colon at the end, so we know why we could not remove it.
| + if err := os.Remove(targetPath); err != nil { | ||
| + logger.Noticef("WARNING: failed to remove file at %s", targetPath) | ||
| + } | ||
| + return "", fmt.Errorf(i18n.G("Failed to write new Xauthority file at %s"), targetPath) |
niemeyer
Apr 25, 2017
Contributor
"Failed to" => "cannot", and the original error is useful to include after a colon at the end, so we know why we could not write it.
| -func streamsEqual(fa, fb io.Reader) bool { | ||
| +// FileStreamsEqual compares two file streams and returns true if both | ||
| +// have the same content. | ||
| +func FileStreamsEqual(fa, fb io.Reader) bool { |
niemeyer
Apr 25, 2017
Contributor
Those are really streams rather than files, or Readers if we want to be more precise. Files are *os.File.
This also seems somewhat unrelated to the operating system, but we can easily reorganize later. Let's please just fix the name since we changed it in this PR.
| @@ -34,14 +34,17 @@ import ( | ||
| // | ||
| // It merges it with the existing os.Environ() and ensures the SNAP_* | ||
| // overrides the any pre-existing environment variables. | ||
| -func ExecEnv(info *snap.Info) []string { | ||
| +func ExecEnv(info *snap.Info, extra map[string]string) []string { |
| + Data []byte | ||
| +} | ||
| + | ||
| +func readBytes(f *os.File, data []byte) error { |
niemeyer
Apr 25, 2017
Contributor
This looks like a reimplementation of io.ReadFull, except it lacks the handling of retries (EAGAIN, etc).
| + } | ||
| + | ||
| + if n != len(data) { | ||
| + return fmt.Errorf("Could not read enough bytes") |
niemeyer
Apr 25, 2017
Contributor
failed to / could not / can't / unable to => cannot
Our errors are always lowercased as well unless there's a good reason not to be.
| + } | ||
| + | ||
| + size := int(binary.BigEndian.Uint16(b[:])) | ||
| + chunk := make([]byte, size) |
niemeyer
Apr 25, 2017
Contributor
This should do some trivial validation of that size before attempting to allocate memory after it.
| + | ||
| +// ValidateXauthority validates a given Xauthority file. The file is valid | ||
| +// if it can be parsed and contains at least one cookie. | ||
| +func ValidateXauthorityFromFile(f *os.File) error { |
niemeyer
Apr 25, 2017
Contributor
Do we need to take a file, or is an io.Reader enough?
If so, I suggest naming this one as ValidateXauthority taking an io.Reader, and the other one as ValidateXauthorityFile taking a path.
|
@niemeyer Will do! Thanks for those comments! |
morphis commentedApr 11, 2017
Not all desktop environments set XAUTHORITY to something which is available
inside the environment we create for snaps. Some place the authority file
inside /tmp which a snap doesn't share with the host system. To workaround
this we copy the file XAUTHORITY points to when the snap is started into a
snap specific location in XDG_RUNTIME_DIR of the current user and allow
access via the x11 interface. This will work across all available desktop
environments.