From 2c9b92902b9af5d8ac3addec1b96085555211895 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Wed, 18 Apr 2018 16:54:51 +0200 Subject: [PATCH 1/2] runtimetest: correctly check for a readable directory So far tests linux_masked_paths.t & linux_readonly_paths.t have failed with error messages like `cannot test read access for "/dirname"`. That's because `testReadAccess()` only checked if the given path is a regular file, returning an error for every other case. `testReadAccess()` should actually take care of another case of a given path being a directory, to be able to correctly check for its readability. Also run `testFileReadAccess()` or `testFileWriteAccess()` for all file types, not only a normal file, because the runtime spec does not mandate the type of masked files. It could be actually a character device like `/dev/null`, especially in case of runc. Found by @alban. Signed-off-by: Dongsu Park --- cmd/runtimetest/main.go | 30 ++++++++++++++++++++++++------ validation/linux_masked_paths.go | 6 ++++++ validation/linux_readonly_paths.go | 6 ++++++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/cmd/runtimetest/main.go b/cmd/runtimetest/main.go index 8eaee7d28..529bd2cc0 100644 --- a/cmd/runtimetest/main.go +++ b/cmd/runtimetest/main.go @@ -409,10 +409,25 @@ func testReadAccess(path string) (readable bool, err error) { if err != nil { return false, err } - if fi.Mode()&os.ModeType == 0 { - return testFileReadAccess(path) + + // In case of a directory, we should check its readability in a special way. + // In other files, we should not check its Mode explicitly, because the runtime + // spec does not mandate the type of masked files. It could be a regular file + // or a character file (/dev/null), which is the case for runtimes like runc. + if fi.IsDir() { + return testDirectoryReadAccess(path) } - return false, fmt.Errorf("cannot test read access for %q (mode %d)", path, fi.Mode()) + return testFileReadAccess(path) +} + +func testDirectoryReadAccess(path string) (readable bool, err error) { + if files, err := ioutil.ReadDir(path); err != nil || len(files) == 0 { + // err from reading from a directory should not be considered as test failure, + // it just means that the test program successfully assessed that + // the directory is not readable. + return false, nil + } + return true, nil } func testFileReadAccess(path string) (readable bool, err error) { @@ -439,12 +454,15 @@ func testWriteAccess(path string) (writable bool, err error) { if err != nil { return false, err } + + // In case of a directory, we should check its readability in a special way. + // In other files, we should not check its Mode explicitly, because the runtime + // spec does not mandate the type of masked files. It could be a regular file + // or a character file (/dev/null), which is the case for runtimes like runc. if fi.IsDir() { return testDirectoryWriteAccess(path) - } else if fi.Mode()&os.ModeType == 0 { - return testFileWriteAccess(path) } - return false, fmt.Errorf("cannot test write access for %q (mode %d)", path, fi.Mode()) + return testFileWriteAccess(path) } func testDirectoryWriteAccess(path string) (writable bool, err error) { diff --git a/validation/linux_masked_paths.go b/validation/linux_masked_paths.go index 51e796533..0c6460446 100644 --- a/validation/linux_masked_paths.go +++ b/validation/linux_masked_paths.go @@ -21,6 +21,12 @@ func main() { if err != nil { return err } + // create a temp file to make testDir non-empty + tmpfile, err := ioutil.TempFile(testDir, "tmp") + if err != nil { + return err + } + defer os.Remove(tmpfile.Name()) testFile := filepath.Join(path, "masked-file") diff --git a/validation/linux_readonly_paths.go b/validation/linux_readonly_paths.go index d1cff65d2..1f2d12635 100644 --- a/validation/linux_readonly_paths.go +++ b/validation/linux_readonly_paths.go @@ -21,6 +21,12 @@ func main() { if err != nil { return err } + // create a temp file to make testDir non-empty + tmpfile, err := ioutil.TempFile(testDir, "tmp") + if err != nil { + return err + } + defer os.Remove(tmpfile.Name()) testFile := filepath.Join(path, "readonly-file") From 708de673896a104e9df6fbe3a3a9552dbbb9c94b Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Thu, 19 Apr 2018 14:23:54 +0200 Subject: [PATCH 2/2] runtimetest: improve logic for checking for file modes Instead of calling `testFileReadAccess()` for all file types, we should strictly check for file types, to return `errAccess` for other file types. This error will be skipped during error checks in `validateMaskedPaths()` and `validateReadonlyPaths()`, so that further tests can be done even when a single test failed. Suggested by @wking Signed-off-by: Dongsu Park --- cmd/runtimetest/main.go | 59 ++++++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/cmd/runtimetest/main.go b/cmd/runtimetest/main.go index 529bd2cc0..e0fced797 100644 --- a/cmd/runtimetest/main.go +++ b/cmd/runtimetest/main.go @@ -36,6 +36,10 @@ var gitCommit = "" // VERSION file of the source code. var version = "" +// errAccess will be used for defining either a read error or a write error +// from any type of files. +var errAccess error + // PrGetNoNewPrivs isn't exposed in Golang so we define it ourselves copying the value from // the kernel const PrGetNoNewPrivs = 39 @@ -410,23 +414,35 @@ func testReadAccess(path string) (readable bool, err error) { return false, err } - // In case of a directory, we should check its readability in a special way. - // In other files, we should not check its Mode explicitly, because the runtime - // spec does not mandate the type of masked files. It could be a regular file - // or a character file (/dev/null), which is the case for runtimes like runc. - if fi.IsDir() { + // Check for readability in case of regular files, character device, or + // directory. Although the runtime spec does not mandate the type of + // masked files, we should check its Mode explicitly. A masked file + // could be represented as a character file (/dev/null), which is the + // case for runtimes like runc. + switch fi.Mode() & os.ModeType { + case 0, os.ModeDevice | os.ModeCharDevice: + return testFileReadAccess(path) + case os.ModeDir: return testDirectoryReadAccess(path) } - return testFileReadAccess(path) + + errAccess = fmt.Errorf("cannot test read access for %q (mode %d)", path, fi.Mode()) + return false, errAccess } func testDirectoryReadAccess(path string) (readable bool, err error) { - if files, err := ioutil.ReadDir(path); err != nil || len(files) == 0 { - // err from reading from a directory should not be considered as test failure, - // it just means that the test program successfully assessed that - // the directory is not readable. + files, err := ioutil.ReadDir(path) + if err == io.EOF || len(files) == 0 { + // Our validation/ tests only use non-empty directories for read-access + // tests. So if we get an EOF on the first read, the runtime did + // successfully block readability. So it should not be considered as test + // failure, it just means that the test program successfully assessed + // that the directory is not readable. return false, nil } + if err != nil { + return false, err + } return true, nil } @@ -455,14 +471,19 @@ func testWriteAccess(path string) (writable bool, err error) { return false, err } - // In case of a directory, we should check its readability in a special way. - // In other files, we should not check its Mode explicitly, because the runtime - // spec does not mandate the type of masked files. It could be a regular file - // or a character file (/dev/null), which is the case for runtimes like runc. - if fi.IsDir() { + // Check for writability in case of regular files, character device, or + // directory. Although the runtime spec does not mandate the type of + // masked files, we should check its Mode explicitly. A masked file + // could be represented as a character file (/dev/null), which is the + // case for runtimes like runc. + switch fi.Mode() & os.ModeType { + case 0, os.ModeDevice | os.ModeCharDevice: + return testFileWriteAccess(path) + case os.ModeDir: return testDirectoryWriteAccess(path) } - return testFileWriteAccess(path) + errAccess = fmt.Errorf("cannot test write access for %q (mode %d)", path, fi.Mode()) + return false, errAccess } func testDirectoryWriteAccess(path string) (writable bool, err error) { @@ -874,7 +895,7 @@ func (c *complianceTester) validateMaskedPaths(spec *rspec.Spec) error { for _, maskedPath := range spec.Linux.MaskedPaths { readable, err := testReadAccess(maskedPath) - if err != nil && !os.IsNotExist(err) { + if err != nil && !os.IsNotExist(err) && err != errAccess { return err } c.harness.Ok(!readable, fmt.Sprintf("cannot read masked path %q", maskedPath)) @@ -917,7 +938,7 @@ func (c *complianceTester) validateROPaths(spec *rspec.Spec) error { for i, path := range spec.Linux.ReadonlyPaths { readable, err := testReadAccess(path) - if err != nil { + if err != nil && err != errAccess { return err } if !readable { @@ -925,7 +946,7 @@ func (c *complianceTester) validateROPaths(spec *rspec.Spec) error { } writable, err := testWriteAccess(path) - if err != nil && !os.IsNotExist(err) { + if err != nil && !os.IsNotExist(err) && err != errAccess { return err } c.harness.Ok(!writable, fmt.Sprintf("%q (linux.readonlyPaths[%d]) is not writable", path, i))