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 3453fc6086a..ed5b80eb150 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] *Metricbeat* - Linux cgroup metrics are now enabled by default for the system process 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