Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration files must not be writeable by other users #3544

Merged
merged 2 commits into from
Feb 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions dev-tools/jenkins_ci
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -140,4 +141,5 @@ main() {
fi
}

umask 022
main $*
13 changes: 9 additions & 4 deletions filebeat/fileset/modules_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion libbeat/cfgfile/cfgfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions libbeat/cfgfile/cfgfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
52 changes: 52 additions & 0 deletions libbeat/common/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably document that there is also an environment variable available for this setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we will need to update the documentation to explain the ownership and permission requirements. This info can go in there.

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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ignoring the error here ok? I'm mainly interested in the behaviour on windows. If I understand correctly, fileUID will be -1 in that case, right? Is euid also -1 on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should never get this far because of the first check to see if this is running on Windows.

On Windows FileInfo.UID() will always return an error. And syscall.Geteuid() always returns -1.

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
}
38 changes: 38 additions & 0 deletions libbeat/common/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package common

import (
"fmt"
"io/ioutil"
"os"
"runtime"
"strings"
"testing"

Expand Down Expand Up @@ -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")
}
}
49 changes: 49 additions & 0 deletions libbeat/common/file/fileinfo.go
Original file line number Diff line number Diff line change
@@ -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
}
73 changes: 73 additions & 0 deletions libbeat/common/file/fileinfo_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
25 changes: 25 additions & 0 deletions libbeat/common/file/fileinfo_unix.go
Original file line number Diff line number Diff line change
@@ -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
}
14 changes: 14 additions & 0 deletions libbeat/common/file/fileinfo_windows.go
Original file line number Diff line number Diff line change
@@ -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
}
3 changes: 2 additions & 1 deletion libbeat/scripts/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down