cmd/snap-update-ns: add secureMkfileAll #4169

Merged
merged 12 commits into from Nov 16, 2017
Next

cmd/snap-update-ns: add secureMkfileAll

This patch adds a function similar to secureMkdirAll that instead of
creating a number of directories instead creates a number of directories
and a final leaf file.

The purpose of this function is to create empty files as bind mount
targets for files present in a read-only location that needs to become
writable.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
  • Loading branch information...
commit 08d3ed6f54de26a7e671b58453044baebf591e1d @zyga zyga committed Nov 8, 2017
@@ -40,8 +40,9 @@ var (
FreezeSnapProcesses = freezeSnapProcesses
ThawSnapProcesses = thawSnapProcesses
// utils
- SecureMkdirAll = secureMkdirAll
EnsureMountPoint = ensureMountPoint
+ SecureMkdirAll = secureMkdirAll
+ SecureMkfileAll = secureMkfileAll
// main
ComputeAndSaveChanges = computeAndSaveChanges
@@ -82,6 +83,18 @@ func formatOpenFlags(flags int) string {
flags ^= syscall.O_DIRECTORY
fl = append(fl, "O_DIRECTORY")
}
+ if flags&syscall.O_RDWR != 0 {
+ flags ^= syscall.O_RDWR
+ fl = append(fl, "O_RDWR")
+ }
+ if flags&syscall.O_CREAT != 0 {
+ flags ^= syscall.O_CREAT
+ fl = append(fl, "O_CREAT")
+ }
+ if flags&syscall.O_EXCL != 0 {
+ flags ^= syscall.O_EXCL
+ fl = append(fl, "O_EXCL")
+ }
if flags != 0 {
panic(fmt.Errorf("unrecognized open flags %d", flags))
}
@@ -101,7 +101,7 @@ func secureMkDir(fd int, segments []string, i int, perm os.FileMode, uid, gid in
const openFlags = syscall.O_NOFOLLOW | syscall.O_CLOEXEC | syscall.O_DIRECTORY
- if err = sysMkdirat(fd, segment, uint32(perm)); err != nil {
+ if err = sysMkdirat(fd, segment, uint32(perm.Perm())); err != nil {
@jdstrand

jdstrand Nov 14, 2017

Contributor

This has the equivalent result for sysMkdirat and is consistent with perm used elsewhere, but seems superfluous. Not a blocker but curious if you made this change for something other than consistency?

@zyga

zyga Nov 15, 2017

Contributor

No, I just noticed after opening the other PR and I somehow landed the change here. Technically we want the Perm part of the mode so this is just for correctness in case of abuse.

switch err {
case syscall.EEXIST:
made = false
@@ -129,6 +129,46 @@ func secureMkDir(fd int, segments []string, i int, perm os.FileMode, uid, gid in
}
// secureMkdirAll is the secure variant of os.MkdirAll.
@jdstrand

jdstrand Nov 14, 2017

Contributor

This comment is wrong for this function. I think having a bigger comment ala secureMkdir is useful here, particularly that you are opening an existing file if it exists.

@zyga

zyga Nov 15, 2017

Contributor

Wrong merge. Updated.

+func secureMkFile(fd int, segments []string, i int, perm os.FileMode, uid, gid int) error {
+ logger.Debugf("secure-mk-file %d %q %d %v %d %d", fd, segments, i, perm, uid, gid)
+ segment := segments[i]
+ made := true
+ var newFd int
+ var err error
+
+ const openFlags = syscall.O_NOFOLLOW | syscall.O_CLOEXEC | syscall.O_RDWR
@jdstrand

jdstrand Nov 14, 2017

Contributor

Not a blocker, but curious why it is O_RDWR and O_RDONLY isn't supported? I'm not saying that O_RDONLY should be supported, but secureMkfile() doesn't scream 'create a read/write file'-- should the API be renamed secureMkfileReadWrite?

@zyga

zyga Nov 15, 2017

Contributor

Interesting point. Well, we don't really care (as the file is closed internally) and we check for EEXIST if the file was already there. I think all files must be writable to be made.

@jdstrand

jdstrand Nov 16, 2017

Contributor

Again, not that I am saying you have to, but you can use O_CREAT | O_RDONLY just fine.

@zyga

zyga Nov 16, 2017

Contributor

Man I only finally understand what's the point. Updated as we don't have to write to it.

I somehow instinctively assumed that O_CREAT needs O_RDWR :-)

+
+ // Open the final path segment as a file. Try to create the file (so that
+ // we know if we need to chown it) but fall back to just opening an
+ // existing one.
+
+ newFd, err = sysOpenat(fd, segment, openFlags|syscall.O_CREAT|syscall.O_EXCL, uint32(perm.Perm()))
+ if err != nil {
+ switch err {
+ case syscall.EEXIST:
+ // If the file exists then just open it without O_EXCL
@jdstrand

jdstrand Nov 14, 2017

Contributor

and O_CREAT

+ newFd, err = sysOpenat(fd, segment, openFlags, 0)
+ if err != nil {
+ return fmt.Errorf("cannot open file %q: %v", segment, err)
+ }
+ made = false
+ default:
+ return fmt.Errorf("cannot open file %q: %v", segment, err)
+ }
+ }
+ defer sysClose(newFd)
+
+ if made {
+ // Chown the file if we made it.
+ if err := sysFchown(newFd, uid, gid); err != nil {
+ return fmt.Errorf("cannot chown file %q to %d.%d: %v", segment, uid, gid, err)
@jdstrand

jdstrand Nov 14, 2017

Contributor

In secureMkdir you sysClose(newFd) with a comment. Shouldn't you be doing the same here? If not, perhaps add a comment why this is ok?

@zyga

zyga Nov 15, 2017

Contributor

No, because we don't return the fd here. It's defer closed already.

All the unit tests (with mocked syscalls) detect leaking descriptors.

+ }
+ }
+
+ return nil
@jdstrand

jdstrand Nov 14, 2017

Contributor

Why aren't you returning newFd here like with the secureMkdir? This is an API inconsistency that at the very least should be documented up above.

@zyga

zyga Nov 15, 2017

Contributor

Because the point of the API (at least for what I need it for) is to ensure the directory / file exists and there is no expected follow up. We don't write to those files, just use them as mount points.

+}
+
+// SecureMkdirAll is the secure variant of os.MkdirAll.
//
// Unlike the regular version this implementation does not follow any symbolic
// links. At all times the new directory segment is created using mkdirat(2)
@@ -169,6 +209,38 @@ func secureMkdirAll(name string, perm os.FileMode, uid, gid int) error {
return nil
}
+// secureMkfileAll is a secure implementation of "mkdir -p $(dirname $1) && touch $1".
+//
+// This function is like secureMkdirAll but it creates an empty file instead of
+// a directory for the final path component. Each created directory component
+// is chowned to the desired user and group.
+func secureMkfileAll(name string, perm os.FileMode, uid, gid int) error {
+ logger.Debugf("secure-mkfile-all %q %q %d %d", name, perm, uid, gid)
+
@bboozzoo

bboozzoo Nov 8, 2017

Contributor

This should probably bail out early if path is /foo/ with / at the end and as a side effect paths like "/" will be rejected too.

if strings.HasSuffix(name, "/") { 
    return errors.New("not a file path")
}
@zyga

zyga Nov 8, 2017

Contributor

Done just below.

+ // Only support absolute paths to avoid bugs in snap-confine when
+ // called from anywhere.
+ if !filepath.IsAbs(name) {
+ return fmt.Errorf("cannot create file with relative path: %q", name)
+ }
+ if strings.HasSuffix(name, "/") {
+ return fmt.Errorf("cannot create non-file path: %q", name)
+ }
@jdstrand

jdstrand Nov 14, 2017

Contributor

We should be checking the suffix after filepath.Clean() to avoid "/foo/..". Does it make sense to at the top of this function (and secureMkdirAll):

if filepath.Clean(name) != name {
   return fmt.Errorf("...")
}
@zyga

zyga Nov 15, 2017

Contributor

I think filepath.Clean chops the trailing / but I'll double-check. EDIT: Done in 0aab1e7

@zyga

zyga Nov 15, 2017

Contributor

Also changed this code to reject unclean paths. I think it's good to merge now.

+ segments := strings.FieldsFunc(filepath.Clean(name), func(c rune) bool { return c == '/' })
+
+ // Create the prefix.
+ fd, err := secureMkPrefix(segments, perm, uid, gid)
+ if err != nil {
+ return err
+ }
+ defer sysClose(fd)
+
+ if len(segments) > 0 {
+ // Create the final segment as a file.
+ err = secureMkFile(fd, segments, len(segments)-1, perm, uid, gid)
+ }
+ return err
+}
+
func ensureMountPoint(path string, mode os.FileMode, uid int, gid int) error {
// If the mount point is not present then create a directory in its
// place. This is very naive, doesn't handle read-only file systems
@@ -54,6 +54,8 @@ func (s *utilsSuite) TearDownTest(c *C) {
s.BaseTest.TearDownTest(c)
}
+// secure-mkdir-all
+
// Ensure that we refuse to create a directory with an relative path.
func (s *utilsSuite) TestSecureMkdirAllRelative(c *C) {
err := update.SecureMkdirAll("rel/path", 0755, 123, 456)
@@ -231,3 +233,173 @@ func (s *realSystemSuite) TestSecureMkdirAllForReal(c *C) {
c.Check(fi.IsDir(), Equals, true)
c.Check(fi.Mode().Perm(), Equals, os.FileMode(0750))
}
+
+// secure-mkfile-all
+
+// Ensure that we refuse to create a file with an relative path.
+func (s *utilsSuite) TestSecureMkfileAllRelative(c *C) {
+ err := update.SecureMkfileAll("rel/path", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, `cannot create file with relative path: "rel/path"`)
+ c.Assert(s.sys.Calls(), HasLen, 0)
+}
+
+// Ensure that we can "create" the root directory.
@jdstrand

jdstrand Nov 14, 2017

Contributor

This comment doesn't seem to go with the test.

@zyga

zyga Nov 15, 2017

Contributor

I meant that ironically but I re-worded this now

+func (s *utilsSuite) TestSecureMkfileAllLevel0(c *C) {
+ err := update.SecureMkfileAll("/", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, `cannot create non-file path: "/"`)
+ c.Assert(s.sys.Calls(), HasLen, 0)
+}
+
+// Ensure that we can create a file in the top-level directory.
+func (s *utilsSuite) TestSecureMkfileAllLevel1(c *C) {
+ c.Assert(update.SecureMkfileAll("/path", 0755, 123, 456), IsNil)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `openat 3 "path" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> 4
+ `fchown 4 123 456`,
+ `close 4`,
+ `close 3`,
+ })
+}
+
+// Ensure that we can create a file two levels from the top-level directory.
+func (s *utilsSuite) TestSecureMkfileAllLevel2(c *C) {
+ c.Assert(update.SecureMkfileAll("/path/to", 0755, 123, 456), IsNil)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `mkdirat 3 "path" 0755`,
+ `openat 3 "path" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 4
+ `fchown 4 123 456`,
+ `close 3`,
+ `openat 4 "to" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> 3
+ `fchown 3 123 456`,
+ `close 3`,
+ `close 4`,
+ })
+}
+
+// Ensure that we can create a file three levels from the top-level directory.
+func (s *utilsSuite) TestSecureMkfileAllLevel3(c *C) {
+ c.Assert(update.SecureMkfileAll("/path/to/something", 0755, 123, 456), IsNil)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `mkdirat 3 "path" 0755`,
+ `openat 3 "path" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 4
+ `fchown 4 123 456`,
+ `mkdirat 4 "to" 0755`,
+ `openat 4 "to" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 5
+ `fchown 5 123 456`,
+ `close 4`,
+ `close 3`,
+ `openat 5 "something" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> 3
+ `fchown 3 123 456`,
+ `close 3`,
+ `close 5`,
+ })
+}
+
+// Ensure that we don't chown existing files or directories.
+func (s *utilsSuite) TestSecureMkfileAllExistingDirsDontChown(c *C) {
+ s.sys.InsertFault(`mkdirat 3 "abs" 0755`, syscall.EEXIST)
+ s.sys.InsertFault(`openat 4 "path" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, syscall.EEXIST)
+ err := update.SecureMkfileAll("/abs/path", 0755, 123, 456)
+ c.Check(err, IsNil)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `mkdirat 3 "abs" 0755`,
+ `openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 4
+ `close 3`,
+ `openat 4 "path" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> EEXIST
+ `openat 4 "path" O_NOFOLLOW|O_CLOEXEC|O_RDWR 0`, // -> 3
+ `close 3`,
+ `close 4`,
+ })
+}
+
+// Ensure that we we close everything when openat fails.
+func (s *utilsSuite) TestSecureMkfileAllOpenat2ndError(c *C) {
+ s.sys.InsertFault(`openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, syscall.EEXIST)
+ s.sys.InsertFault(`openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_RDWR 0`, errTesting)
+ err := update.SecureMkfileAll("/abs", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, `cannot open file "abs": testing`)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> EEXIST
+ `openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_RDWR 0`, // -> err
+ `close 3`,
+ })
+}
+
+// Ensure that we we close everything when openat (non-exclusive) fails.
+func (s *utilsSuite) TestSecureMkfileAllOpenatError(c *C) {
+ s.sys.InsertFault(`openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, errTesting)
+ err := update.SecureMkfileAll("/abs", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, `cannot open file "abs": testing`)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> err
+ `close 3`,
+ })
+}
+
+// Ensure that we we close everything when fchown fails.
+func (s *utilsSuite) TestSecureMkfileAllFchownError(c *C) {
+ s.sys.InsertFault(`fchown 4 123 456`, errTesting)
+ err := update.SecureMkfileAll("/path", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, `cannot chown file "path" to 123.456: testing`)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `openat 3 "path" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, // -> 4
+ `fchown 4 123 456`,
+ `close 4`,
+ `close 3`,
+ })
+}
+
+// Check error path when we cannot open root directory.
+func (s *utilsSuite) TestSecureMkfileAllOpenRootError(c *C) {
+ s.sys.InsertFault(`open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, errTesting)
+ err := update.SecureMkfileAll("/abs/path", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, "cannot open root directory: testing")
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> err
+ })
+}
+
+// Check error path when we cannot open non-root directory.
+func (s *utilsSuite) TestSecureMkfileAllOpenError(c *C) {
+ s.sys.InsertFault(`openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, errTesting)
+ err := update.SecureMkfileAll("/abs/path", 0755, 123, 456)
+ c.Assert(err, ErrorMatches, `cannot open path segment "abs" \(got up to "/"\): testing`)
+ c.Assert(s.sys.Calls(), DeepEquals, []string{
+ `open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> 3
+ `mkdirat 3 "abs" 0755`,
+ `openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`, // -> err
+ `close 3`,
+ })
+}
+
+// Check that we can actually create files.
+// This doesn't test the chown logic as that requires root.
+func (s *realSystemSuite) TestSecureMkfileAllForReal(c *C) {
+ d := c.MkDir()
+
+ // Create f1, which is a simple subdirectory, with a distinct mode and
+ // check that it was applied. Note that default umask 022 is subtracted so
+ // effective directory has different permissions.
+ f1 := filepath.Join(d, "file")
+ c.Assert(update.SecureMkfileAll(f1, 0707, -1, -1), IsNil)
+ fi, err := os.Stat(f1)
+ c.Assert(err, IsNil)
+ c.Check(fi.Mode().IsRegular(), Equals, true)
+ c.Check(fi.Mode().Perm(), Equals, os.FileMode(0705))
+
+ // Create f2, which is a deeper subdirectory, with another distinct mode
+ // and check that it was applied.
+ f2 := filepath.Join(d, "subdir/subdir/file")
+ c.Assert(update.SecureMkfileAll(f2, 0750, -1, -1), IsNil)
+ fi, err = os.Stat(f2)
+ c.Assert(err, IsNil)
+ c.Check(fi.Mode().IsRegular(), Equals, true)
+ c.Check(fi.Mode().Perm(), Equals, os.FileMode(0750))
+}