Configuration files must not be writeable by other users (#3544)

* 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

@@ -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

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*
 

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 $*

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)

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)

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)

libbeat/common/config.go

@@ -5,15 +5,29 @@ 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"
 	cfgflag "github.com/elastic/go-ucfg/flag"
 	"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
+}

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")
+	}
+}

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
+}

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)
+}

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
+}

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
+}

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