From 32d4285512bc34028e0606eebc34af158d8ac0da Mon Sep 17 00:00:00 2001 From: Andrew Kroh Date: Tue, 7 Feb 2017 11:13:20 -0500 Subject: [PATCH] Configuration files must not be writeable by other users (#3544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Configuration files must not be writeable by other users … This PR adds enforcement of ownership and file permissions on configuration files. Any configuration file must be owned by the same user that the Beat is running as and the file must not be writable by anyone other than the owner. This strict permission checking is limited to platforms with POSIX file permissions. The DACLs used by Windows are not checked at this time. The check can be disabled on the CLI with `-strict.perms=false` or by setting env var `BEAT_STRICT_PERMS=false`. * Update jenkins_ci to fix umask on git clone --- .travis.yml | 2 + CHANGELOG.asciidoc | 1 + dev-tools/jenkins_ci | 2 + filebeat/fileset/modules_integration_test.go | 13 ++-- libbeat/cfgfile/cfgfile.go | 2 +- libbeat/cfgfile/cfgfile_test.go | 5 +- libbeat/common/config.go | 52 ++++++++++++++ libbeat/common/config_test.go | 38 ++++++++++ libbeat/common/file/fileinfo.go | 49 +++++++++++++ libbeat/common/file/fileinfo_test.go | 73 ++++++++++++++++++++ libbeat/common/file/fileinfo_unix.go | 25 +++++++ libbeat/common/file/fileinfo_windows.go | 14 ++++ libbeat/scripts/Makefile | 3 +- 13 files changed, 271 insertions(+), 8 deletions(-) create mode 100644 libbeat/common/file/fileinfo.go create mode 100644 libbeat/common/file/fileinfo_test.go create mode 100644 libbeat/common/file/fileinfo_unix.go create mode 100644 libbeat/common/file/fileinfo_windows.go diff --git a/.travis.yml b/.travis.yml index f875a84ccc2..8206a90ebd6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -88,6 +88,8 @@ addons: - geoip-database before_install: + - umask 022 + - chmod -R go-w $GOPATH/src/github.com/elastic/beats # Docker-compose installation - sudo rm /usr/local/bin/docker-compose || true - curl -L https://github.com/docker/compose/releases/download/${DOCKER_COMPOSE_VERSION}/docker-compose-`uname -s`-`uname -m` > docker-compose diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 5919ae27c8f..59a40a438fa 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -15,6 +15,7 @@ https://github.com/elastic/beats/compare/v5.1.1...master[Check the HEAD diff] *Affecting all Beats* - Change beat generator. Use `$GOPATH/src/github.com/elastic/beats/script/generate.py` to generate a beat. {pull}3452[3452] +- Configuration files must not be writable by other users. {pull}3544[3544] *Filebeat* diff --git a/dev-tools/jenkins_ci b/dev-tools/jenkins_ci index f25a368a527..fb5afaa858b 100755 --- a/dev-tools/jenkins_ci +++ b/dev-tools/jenkins_ci @@ -131,6 +131,7 @@ main() { err "--build and --cleanup cannot be used together" exit 1 elif [ "$BUILD" == "true" ]; then + chmod -R go-w "${GOPATH}/src/github.com/elastic/beats" build elif [ "$CLEANUP" == "true" ]; then cleanup @@ -140,4 +141,5 @@ main() { fi } +umask 022 main $* diff --git a/filebeat/fileset/modules_integration_test.go b/filebeat/fileset/modules_integration_test.go index 8e98a2c848d..eb7237a07ad 100644 --- a/filebeat/fileset/modules_integration_test.go +++ b/filebeat/fileset/modules_integration_test.go @@ -45,8 +45,9 @@ func TestLoadPipeline(t *testing.T) { var res map[string]interface{} err = json.Unmarshal(response, &res) - assert.NoError(t, err) - assert.Equal(t, "describe pipeline", res["my-pipeline-id"].(map[string]interface{})["description"], string(response)) + if assert.NoError(t, err) { + assert.Equal(t, "describe pipeline", res["my-pipeline-id"].(map[string]interface{})["description"], string(response)) + } } func TestSetupNginx(t *testing.T) { @@ -62,10 +63,14 @@ func TestSetupNginx(t *testing.T) { } reg, err := newModuleRegistry(modulesPath, configs, nil, "5.2.0") - assert.NoError(t, err) + if err != nil { + t.Fatal(err) + } err = reg.LoadPipelines(client) - assert.NoError(t, err) + if err != nil { + t.Fatal(err) + } status, _, _ := client.Request("GET", "/_ingest/pipeline/filebeat-5.2.0-nginx-access-with_plugins", "", nil, nil) assert.Equal(t, 200, status) diff --git a/libbeat/cfgfile/cfgfile.go b/libbeat/cfgfile/cfgfile.go index cf8290846cf..d7eeaf1c0d4 100644 --- a/libbeat/cfgfile/cfgfile.go +++ b/libbeat/cfgfile/cfgfile.go @@ -91,7 +91,7 @@ func HandleFlags() error { func Read(out interface{}, path string) error { config, err := Load(path) if err != nil { - return nil + return err } return config.Unpack(out) diff --git a/libbeat/cfgfile/cfgfile_test.go b/libbeat/cfgfile/cfgfile_test.go index a8d1832345b..74d104df99a 100644 --- a/libbeat/cfgfile/cfgfile_test.go +++ b/libbeat/cfgfile/cfgfile_test.go @@ -34,8 +34,9 @@ func TestRead(t *testing.T) { config := &TestConfig{} - err = Read(config, absPath+"/config.yml") - assert.Nil(t, err) + if err = Read(config, absPath+"/config.yml"); err != nil { + t.Fatal(err) + } // validate assert.Equal(t, "localhost", config.Output.Elasticsearch.Host) diff --git a/libbeat/common/config.go b/libbeat/common/config.go index 593471acbdf..a35888da239 100644 --- a/libbeat/common/config.go +++ b/libbeat/common/config.go @@ -5,8 +5,11 @@ import ( "errors" "flag" "fmt" + "os" + "runtime" "strings" + "github.com/elastic/beats/libbeat/common/file" "github.com/elastic/beats/libbeat/logp" "github.com/elastic/go-ucfg" "github.com/elastic/go-ucfg/cfgutil" @@ -14,6 +17,17 @@ import ( "github.com/elastic/go-ucfg/yaml" ) +var flagStrictPerms = flag.Bool("strict.perms", true, "Strict permission checking on config files") + +// IsStrictPerms returns true if strict permission checking on config files is +// enabled. +func IsStrictPerms() bool { + if !*flagStrictPerms || os.Getenv("BEAT_STRICT_PERMS") == "false" { + return false + } + return true +} + // Config object to store hierarchical configurations into. // See https://godoc.org/github.com/elastic/go-ucfg#Config type Config ucfg.Config @@ -143,6 +157,12 @@ func NewFlagOverwrite( } func LoadFile(path string) (*Config, error) { + if IsStrictPerms() { + if err := ownerHasExclusiveWritePerms(path); err != nil { + return nil, err + } + } + c, err := yaml.NewConfigWithFile(path, configOpts...) if err != nil { return nil, err @@ -390,3 +410,35 @@ func filterDebugObject(c interface{}) { } } } + +// ownerHasExclusiveWritePerms asserts that the current user is the +// owner of the config file and that the config file is (at most) writable by +// the owner (e.g. group and other cannot have write access). +func ownerHasExclusiveWritePerms(name string) error { + if runtime.GOOS == "windows" { + return nil + } + + info, err := file.Stat(name) + if err != nil { + return err + } + + euid := os.Geteuid() + fileUID, _ := info.UID() + perm := info.Mode().Perm() + + if euid != fileUID { + return fmt.Errorf(`config file ("%v") must be owned by the beat user `+ + `(uid=%v)`, name, euid) + } + + // Test if group or other have write permissions. + if perm&0022 > 0 { + return fmt.Errorf(`config file ("%v") can only be writable by the `+ + `owner but the permissions are "%v"`, + name, perm) + } + + return nil +} diff --git a/libbeat/common/config_test.go b/libbeat/common/config_test.go index 1d4e9db7ca3..6e9f2a3cfae 100644 --- a/libbeat/common/config_test.go +++ b/libbeat/common/config_test.go @@ -2,6 +2,9 @@ package common import ( "fmt" + "io/ioutil" + "os" + "runtime" "strings" "testing" @@ -110,3 +113,38 @@ func TestConfigPrintDebug(t *testing.T) { assert.Equal(t, test.expected, buf) } } + +func TestConfigFilePermissions(t *testing.T) { + if !IsStrictPerms() { + t.Skip("Skipping test because strict.perms is disabled") + } + + f, err := ioutil.TempFile("", "writableConfig.yml") + if err != nil { + t.Fatal(err) + } + defer os.Remove(f.Name()) + defer f.Close() + + f.WriteString(`test.data: [1, 2, 3, 4]`) + f.Sync() + + if _, err = LoadFile(f.Name()); err != nil { + t.Fatal(err) + } + + // Permissions checking isn't implemented for Windows DACLs. + if runtime.GOOS == "windows" { + return + } + + if err = os.Chmod(f.Name(), 0460); err != nil { + t.Fatal(err) + } + + // Read will fail because config is group writable. + _, err = LoadFile(f.Name()) + if assert.Error(t, err, "expected writable error") { + assert.Contains(t, err.Error(), "writable") + } +} diff --git a/libbeat/common/file/fileinfo.go b/libbeat/common/file/fileinfo.go new file mode 100644 index 00000000000..05e9f2d2888 --- /dev/null +++ b/libbeat/common/file/fileinfo.go @@ -0,0 +1,49 @@ +package file + +import ( + "errors" + "os" +) + +// A FileInfo describes a file and is returned by Stat and Lstat. +type FileInfo interface { + os.FileInfo + UID() (int, error) // UID of the file owner. Returns an error on non-POSIX file systems. + GID() (int, error) // GID of the file owner. Returns an error on non-POSIX file systems. +} + +// Stat returns a FileInfo describing the named file. +// If there is an error, it will be of type *PathError. +func Stat(name string) (FileInfo, error) { + return stat(name, os.Stat) +} + +// Lstat returns a FileInfo describing the named file. +// If the file is a symbolic link, the returned FileInfo +// describes the symbolic link. Lstat makes no attempt to follow the link. +// If there is an error, it will be of type *PathError. +func Lstat(name string) (FileInfo, error) { + return stat(name, os.Lstat) +} + +type fileInfo struct { + os.FileInfo + uid *int + gid *int +} + +func (f fileInfo) UID() (int, error) { + if f.uid == nil { + return -1, errors.New("uid not implemented") + } + + return *f.uid, nil +} + +func (f fileInfo) GID() (int, error) { + if f.gid == nil { + return -1, errors.New("gid not implemented") + } + + return *f.gid, nil +} diff --git a/libbeat/common/file/fileinfo_test.go b/libbeat/common/file/fileinfo_test.go new file mode 100644 index 00000000000..53aecf6bb93 --- /dev/null +++ b/libbeat/common/file/fileinfo_test.go @@ -0,0 +1,73 @@ +// +build !windows + +package file_test + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" + + "github.com/elastic/beats/libbeat/common/file" + "github.com/stretchr/testify/assert" +) + +func TestStat(t *testing.T) { + f, err := ioutil.TempFile("", "teststat") + if err != nil { + t.Fatal(err) + } + defer os.Remove(f.Name()) + + link := filepath.Join(os.TempDir(), "teststat-link") + if err := os.Symlink(f.Name(), link); err != nil { + t.Fatal(err) + } + defer os.Remove(link) + + info, err := file.Stat(link) + if err != nil { + t.Fatal(err) + } + + assert.True(t, info.Mode().IsRegular()) + + uid, err := info.UID() + if err != nil { + t.Fatal(err) + } + assert.EqualValues(t, os.Geteuid(), uid) + + gid, err := info.GID() + if err != nil { + t.Fatal(err) + } + assert.EqualValues(t, os.Getegid(), gid) +} + +func TestLstat(t *testing.T) { + link := filepath.Join(os.TempDir(), "link") + if err := os.Symlink("dummy", link); err != nil { + t.Fatal(err) + } + defer os.Remove(link) + + info, err := file.Lstat(link) + if err != nil { + t.Fatal(err) + } + + assert.True(t, info.Mode()&os.ModeSymlink > 0) + + uid, err := info.UID() + if err != nil { + t.Fatal(err) + } + assert.EqualValues(t, os.Geteuid(), uid) + + gid, err := info.GID() + if err != nil { + t.Fatal(err) + } + assert.EqualValues(t, os.Getegid(), gid) +} diff --git a/libbeat/common/file/fileinfo_unix.go b/libbeat/common/file/fileinfo_unix.go new file mode 100644 index 00000000000..68d60b1cb83 --- /dev/null +++ b/libbeat/common/file/fileinfo_unix.go @@ -0,0 +1,25 @@ +// +build !windows + +package file + +import ( + "errors" + "os" + "syscall" +) + +func stat(name string, statFunc func(name string) (os.FileInfo, error)) (FileInfo, error) { + info, err := statFunc(name) + if err != nil { + return nil, err + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return nil, errors.New("failed to get uid/gid") + } + + uid := int(stat.Uid) + gid := int(stat.Gid) + return fileInfo{FileInfo: info, uid: &uid, gid: &gid}, nil +} diff --git a/libbeat/common/file/fileinfo_windows.go b/libbeat/common/file/fileinfo_windows.go new file mode 100644 index 00000000000..e6ae54ae35e --- /dev/null +++ b/libbeat/common/file/fileinfo_windows.go @@ -0,0 +1,14 @@ +package file + +import ( + "os" +) + +func stat(name string, statFunc func(name string) (os.FileInfo, error)) (FileInfo, error) { + info, err := statFunc(name) + if err != nil { + return nil, err + } + + return fileInfo{FileInfo: info}, nil +} diff --git a/libbeat/scripts/Makefile b/libbeat/scripts/Makefile index fdadc5d447a..37024f74544 100755 --- a/libbeat/scripts/Makefile +++ b/libbeat/scripts/Makefile @@ -301,7 +301,8 @@ stop-environment: .PHONY: write-environment write-environment: mkdir -p ${BUILD_DIR} - echo "ES_HOST=${ES_HOST}" > ${BUILD_DIR}/test.env + echo "BEAT_STRICT_PERMS=false" > ${BUILD_DIR}/test.env + echo "ES_HOST=${ES_HOST}" >> ${BUILD_DIR}/test.env echo "ES_PORT=9200" >> ${BUILD_DIR}/test.env echo "ES_USER=beats" >> ${BUILD_DIR}/test.env echo "ES_PASS=testing" >> ${BUILD_DIR}/test.env