From d5b0f9339d5b1ba3e72ba3f9b7d9890a06619180 Mon Sep 17 00:00:00 2001 From: Andrew Kroh <andrew.kroh@elastic.co> Date: Tue, 7 Feb 2017 01:06:09 -0500 Subject: [PATCH 1/2] =?UTF-8?q?Configuration=20files=20must=20not=20be=20w?= =?UTF-8?q?riteable=20by=20other=20users=20=20=E2=80=A6=20This=20PR=20adds?= =?UTF-8?q?=20enforcement=20of=20ownership=20and=20file=20permissions=20on?= =?UTF-8?q?=20configuration=20files.=20Any=20configuration=20file=20must?= =?UTF-8?q?=20be=20owned=20by=20the=20same=20user=20that=20the=20Beat=20is?= =?UTF-8?q?=20running=20as=20and=20the=20file=20must=20not=20be=20writable?= =?UTF-8?q?=20by=20anyone=20other=20than=20the=20owner.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`. --- .travis.yml | 2 + CHANGELOG.asciidoc | 1 + filebeat/fileset/modules_integration_test.go | 13 ++-- 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 +- 10 files changed, 265 insertions(+), 5 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 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/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/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 From f4292f3f72bf90c0b7daaec90980dd8f9bbe751e Mon Sep 17 00:00:00 2001 From: Andrew Kroh <andrew.kroh@elastic.co> Date: Tue, 7 Feb 2017 02:19:28 -0500 Subject: [PATCH 2/2] Update jenkins_ci to fix umask on git clone --- dev-tools/jenkins_ci | 2 ++ libbeat/cfgfile/cfgfile.go | 2 +- libbeat/cfgfile/cfgfile_test.go | 5 +++-- 3 files changed, 6 insertions(+), 3 deletions(-) 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/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)