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-update-ns: add secureMkfileAll #4169
Conversation
codecov-io
commented
Nov 7, 2017
•
Edited 8 times
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 15, 2017
-
codecov-io
Nov 15, 2017
-
codecov-io
Nov 8, 2017
-
codecov-io
Nov 8, 2017
-
Nov 16, 2017 -
Nov 16, 2017 -
Nov 16, 2017 -
Nov 16, 2017 -
Nov 15, 2017 -
Nov 15, 2017 -
Nov 8, 2017 -
Nov 8, 2017
Codecov Report
@@ Coverage Diff @@
## master #4169 +/- ##
==========================================
+ Coverage 75.8% 75.96% +0.15%
==========================================
Files 439 440 +1
Lines 37995 38427 +432
==========================================
+ Hits 28803 29190 +387
- Misses 7190 7223 +33
- Partials 2002 2014 +12
Continue to review full report at Codecov.
|
| +// 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
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")
}
Some checks were not successful
zyga
added some commits
Nov 8, 2017
Some checks were not successful
jdstrand
requested changes
Nov 14, 2017
Overall seems fine, but a few questions in line and one change re filepath.Clean.
| @@ -110,7 +110,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
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
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.
| @@ -144,6 +144,46 @@ func secureMkDir(fd int, segments []string, i int, perm os.FileMode, uid, gid in | ||
| } | ||
| // secureMkdirAll is the secure variant of os.MkdirAll. |
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.
| + var newFd int | ||
| + var err error | ||
| + | ||
| + const openFlags = syscall.O_NOFOLLOW | syscall.O_CLOEXEC | syscall.O_RDWR |
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
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
Nov 16, 2017
Contributor
Again, not that I am saying you have to, but you can use O_CREAT | O_RDONLY just fine.
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 :-)
| + if err != nil { | ||
| + switch err { | ||
| + case syscall.EEXIST: | ||
| + // If the file exists then just open it without O_EXCL |
| + 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
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
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
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
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.
| + } | ||
| + if strings.HasSuffix(name, "/") { | ||
| + return fmt.Errorf("cannot create non-file path: %q", name) | ||
| + } |
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
Nov 15, 2017
•
Edited 1 time
-
zyga
Nov 15, 2017
Contributor
-
Nov 15, 2017
I think filepath.Clean chops the trailing / but I'll double-check. EDIT: Done in 0aab1e7
zyga
Nov 15, 2017
Contributor
Also changed this code to reject unclean paths. I think it's good to merge now.
| + c.Assert(s.sys.Calls(), HasLen, 0) | ||
| +} | ||
| + | ||
| +// Ensure that we can "create" the root directory. |
zyga
added some commits
Nov 15, 2017
| + s.sys.InsertFault(`openat 4 "path" O_NOFOLLOW|O_CLOEXEC|O_RDWR|O_CREAT|O_EXCL 0755`, syscall.EROFS) | ||
| + err := update.SecureMkfileAll("/rofs/path", 0755, 123, 456) | ||
| + c.Check(err, ErrorMatches, `cannot operate on read-only filesystem at /rofs`) | ||
| + // c.Assert(err.(*update.ReadOnlyFsError).Path, Equals, "/rofs") |
zyga commentedNov 7, 2017
This branch is based on #4163
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